Queue behaviour#58
Conversation
…/move_basic into queue-behaviour
|
goalCancelling test fixture is not working properly yet. |
| ASSERT_EQ(3.0, received_goal->target_pose.pose.position.x); | ||
|
|
||
| // Cancelling the first goal - PITFALL | ||
| resumeExecuting(); |
There was a problem hiding this comment.
You should resume after you sent the cancel, that way the code in the callback has a chance to check for preempts and stuff again.
| ASSERT_TRUE(got_goal); | ||
| ASSERT_EQ(7.0, received_goal->target_pose.pose.position.x); | ||
|
|
||
| // Third goal |
| finishExecuting(); // Finish the 3nd goal | ||
| /* | ||
| - if another goal is received add it to the queue (DONE) | ||
| - if the queue full, set the current goal as preempted - explicitly called! - so cancelling the current goal and start executing the next one |
There was a problem hiding this comment.
We decided to change this behavior. If the queue is full, the last goal in the queue will be canceled and replaced with the new goal.
rohbotics
left a comment
rohbotics
left a comment
There was a problem hiding this comment.
Thanks, overall the tests look good, but all the sleeps make me nervous, because it means the tests are timing sensitive and might randomly fail. I ran the tests 10 times, and I had at least 3 random failures.
I think with more signalling condition variables we can significantly increase the reliability of the tests.
-
We should have a signal that goes off at the beginning of
executeCallbackthen we can have a helper function that waits for that signal to go off instead of just doing a sleep. -
resumeExecuting needs to wait for one loop of the executeCallback to complete so that the test variables actually have a chance to update.
Can you implement that?
|
Yes, I will work on it. |
|
So despite removing the sleep functions and adding Helper function, unit testing succed in 7/10 cases. Before moving on to ASSERT-ing variables I want to make sure where am I missing things, that the test is so unreliable. |
|
The test fixtures are working consistently but lagging a lot due to sleep() that cancelGoal and everything else required to work properly. Tests are time independent, sleep are there just so the threads have enough time to close. |
| resumeExecuting(); | ||
| updateExecuting(); | ||
| sleepExecuting(); | ||
| EXPECT_TRUE(goal_preempted); |
There was a problem hiding this comment.
I tested this 10 times, this is the only line that failed (failed 3 times). Can you put a 0.5 second sleep before it as a quick fix?
There was a problem hiding this comment.
Weird, never failed in my case. Yes, sure.
dorkamotorka
force-pushed
the
queue-behaviour
branch
from
b3edd22 to
d663010
Compare
2 participants
No description provided.