-
Notifications
You must be signed in to change notification settings - Fork 417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add executor unit tests #1336
Add executor unit tests #1336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Executor::execute_any_executable(AnyExecutable & any_exec) |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: I added those in #1330, one of us will have to rebase it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, thanks for the heads up.
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>
8d86235
to
027a0f1
Compare
* 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>
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_subscription
method. 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.