[iron] add test to check for execution order of entities in various executors #2561
Conversation
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
|
CI:
|
|
I messed up the CI, here's some hopefully fixed runs:
|
Signed-off-by: William Woodall <william@osrfoundation.org>
|
CI after uncrustify fix:
|
|
Unfortunately the order of execution for timers seems to be subtly out of the expected order on Windows 🤦 . I'll look into it more tonight. |
| for (size_t i = 0; i < number_of_entities; ++i) { | ||
| // "call order" for timers will be simulated by setting them at different | ||
| // periods, with the "first" ones having the smallest period. | ||
| auto period = 1ms + std::chrono::milliseconds(test_case.call_order[i]); |
There was a problem hiding this comment.
according to https://ci.ros2.org/job/ci_windows/22053/testReport/projectroot.test/rclcpp/test_executors/
values:
actual_order
Which is: { 18, 19, 17, 16, 14, 15, 13, 12, 10, 11, 9, 7, 8, 6, 5, 4, 2, 3, 1, 0 }
test_case.expected_execution_order
Which is: { 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 }means timers with period 1ms (in the end of the list of callbackgroup) and 2ms (2nd from the end) are ready when the 1st execution, then executor takes out the timer with 2ms since that is in front of timer with 1ms? it seems that this situation only happens 1st spin execution, that means probably when the executor spins and call `rcl_wait, it has been already 1ms passed?
if above assumption is right, maybe 1ms is too short for windows... (if that is either rolling or jazzy, we can create the timer with auto_start = false, and the reset the timer right before the spin. that would save some time for the 1st execution.)
|
Since this is targeting Iron, which is going EOL soon, I'm going to close this out. @wjwwood if you think it makes sense to retarget this to rolling, please feel free to reopen and retarget. |
This is the Iron version of #2536 and passes without changes to the actual source of rclcpp, which supports the idea that this test is actually testing a regression between iron and jazzy. The fix for jazzy/rolling has been proposed here: #2537