Conversation
|
CI:
|
brawner
left a comment
brawner
left a comment
There was a problem hiding this comment.
Looks good to me, just a couple of questions
| // Wait for the wall timer to have expired. | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(50)); | ||
| EXPECT_FALSE(timer_fired); | ||
| dummy.spin_nanoseconds(node->get_node_base_interface()); |
There was a problem hiding this comment.
I'm not very familiar with spin_once, but is it possible that spin_once for different rmw implementations that there are other tasks that get taken care of here, or is expected that this timer will fire here?
There was a problem hiding this comment.
My understanding is that spin_once collects all of the entities that could possibly be executed, and then picks one to execute from a fixed-priority list:
rclcpp/rclcpp/src/rclcpp/executor.cpp
Line 488 in 148d295
Since timers are always the first thing in the priority list, then the only way we could not execute this is if some other entity added a timer to the executor before this one. But I don't think that can currently happen.
There is a lot of interest in removing that fixed-priority scheme and make it something else (FIFO, or whatever). If that happens, then there is a small possibility that this test won't work anymore, but I think it is pretty unlikely.
| node->create_wall_timer(std::chrono::milliseconds(1), [&timer_fired]() {timer_fired = true;}); | ||
|
|
||
| // Wait for the wall timer to have expired. | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(50)); |
There was a problem hiding this comment.
Just taking into account how long rclcpp already takes for tests to run, can these sleeps be shorter (like 2ms or 10ms)? Or do you think it would be flaky?
There was a problem hiding this comment.
I don't think that 2ms is going to be enough. 10ms is probably enough, but I really didn't want to introduce a flaky test here (particularly on Windows, where timers are really jittery).
Just as a datapoint, this entire file full of tests takes 842ms to run on my machine. If I were to change all of these 50ms sleeps to 10ms, then I'd save ~210ms. Its a good chunk, but I think the resistance to flakiness is more important.
There was a problem hiding this comment.
Very true, and 210ms is peanuts all told. I just thought it was not great practice to depend on sleeps for behavior tests, but I also couldn't think of a way to assert the proper behavior. FWIW on my machine, these tests all pass even removing this sleep.
| typename ArgT2, typename ArgT3, | ||
| typename ArgT4, typename ArgT5, | ||
| typename ArgT6, typename ArgT7> | ||
| struct PatchTraits<ID, ReturnT(ArgT0, ArgT1, ArgT2, ArgT3, ArgT4, ArgT5, ArgT6, ArgT7)> |
There was a problem hiding this comment.
FYI: I added those in #1330, one of us will have to rebase it
There was a problem hiding this comment.
Good to know, thanks for the heads up.
clalancette
force-pushed
the
clalancette/test-executor
branch
from
d0f3508 to
8d86235
Compare
In particular, make sure to use 'throw_from_rcl_error' as much as possible. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This will be needed by the executor tests. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
force-pushed
the
clalancette/test-executor
branch
from
8d86235 to
027a0f1
Compare
|
One more CI:
|
* Improve the error messages in the Executor class. In particular, make sure to use 'throw_from_rcl_error' as much as possible. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Allow mimick patching of methods with up to 9 arguments. This will be needed by the executor tests. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in unit tests for the Executor class. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Adjust test_executor for foxy Signed-off-by: Stephen Brawner <brawner@gmail.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
* Improve the error messages in the Executor class. In particular, make sure to use 'throw_from_rcl_error' as much as possible. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Allow mimick patching of methods with up to 9 arguments. This will be needed by the executor tests. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in unit tests for the Executor class. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Adjust test_executor for foxy Signed-off-by: Stephen Brawner <brawner@gmail.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
4 participants
This PR adds in unit tests for the Executor class. With this change in place, I'm seeing 91% line coverage of the executor.cpp file.
Besides a handful of single lines that aren't being covered, there is a big chunk of missing coverage in the
execute_subscriptionmethod. I think this is going to be a lot of work to cover, so I figured I would open what I have so far and we can iterate on that later.