search

osrf / capabilities

Implements the concept of capabilities as part of the robots-in-concert system.
Other
8 stars 26 forks source link

Fix typos in start_capability service #80

Closed jonbinney closed 9 years ago

jonbinney commented 9 years ago

My previous PR had two typos in it that I didn't notice because it only causes problems if a capability is started while a previous instance of it is in the process of stopping. This PR fixes those typos, and adds tests to catch this case.

wjwwood commented 9 years ago

The tests don't seem to pass. I think it might be a side effect of you modifying the minimal_pkg test fixture.

jonbinney commented 9 years ago

Oops. I made sure that the test I added passed, but forgot to check the other ones. I'll sort it out.

jonbinney commented 9 years ago

The discovery test wasn't expecting to find the new capability I added. I've updated the test to expect this. All tests pass on my computer now, we'll see what travis thinks.

jonbinney commented 9 years ago

Damn, a couple tests fail on travis but not on my machine.

wjwwood commented 9 years ago

Ok, I just turned travis over to see if it is one of those pesky race condition tests. If it is I can try to help tune it up so it doesn't spuriously fail.

jonbinney commented 9 years ago

Looks like it's still failing. I'll look a bit deeper at it. Its a bit frustrating because each of the tests in a file run in the same ros system; so if one test function runs a capability, but has a bug and fails to stop that capability, a later test could fail because it expects only a certain capability to be running. Sometimes the test that fails isn't the one with the bug.

jonbinney commented 9 years ago

ok i can reproduce this locally; it happens about 1 in 4 times i run the tests.

jonbinney commented 9 years ago

I think i'm beginning to understand what is going on. I wrote a test to start the SlowToDie capability, then ask it to stop, then ask it to start again immediately. I asserted that starting it up again would fail, because it should still be busy shutting down. BUT it looks like the stop_capability service actually blocks. So by the time stop_capability finally returns, it is often actually stopped (not necessarily, since there's still an asynchronous bit before the graph gets updated, but it's a spit second timing thing).

Moral of the story is that if you call stop_capability, it's gonna block until the capability is stopped, and because there's an asynchronous Event before the actual graph gets updated, you can't count on it being for sure started or for sure stopped. So there isn't any bug here, its just that we can't write a test that expects it to go one way or the other. I'll remove this particular test.

jonbinney commented 9 years ago

Looks like it is working now.

wjwwood commented 9 years ago

Cool, thanks @jonbinney.