Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make test_executor.spin_some_max_duration more reliable #430

Merged
merged 4 commits into from
May 6, 2020
Merged

Make test_executor.spin_some_max_duration more reliable #430

merged 4 commits into from
May 6, 2020

Conversation

ivanpauno
Copy link
Member

Fixes https://ci.ros2.org/view/nightly/job/nightly_win_rel/1537/testReport/junit/test_rclcpp/test_executor__rmw_cyclonedds_cpp/spin_some_max_duration/.

Considering that the Windows scheduler time slice is 20ms, the test is being too optimistic (same applies for macOS and Linux, though it was only failing on Windows).

I increased all durations, to ensure that the OS time slice doesn't represent a big fraction of it.

@ivanpauno
Make test_executor.spin_some_max_duration more reliable
bd4e975 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label May 1, 2020
@ivanpauno ivanpauno requested a review from sloretz May 1, 2020 19:32
@ivanpauno ivanpauno self-assigned this May 1, 2020
clalancette
clalancette approved these changes May 1, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change to add a bit of documentation, but I'll approve anyway.

test_rclcpp/test/test_executor.cpp
@@ -61,7 +61,7 @@ TEST(CLASSNAME(test_executor, RMW_IMPLEMENTATION), spin_some_max_duration) {
rclcpp::executors::SingleThreadedExecutor executor;
auto node = rclcpp::Node::make_shared("spin_some_max_duration");
auto lambda = []() {
std::this_thread::sleep_for(1ms);
std::this_thread::sleep_for(100ms);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change, and keeps the current semantics of the test; basically, that 20 threads of N ms sleep take < 20*N ms to complete. However, those semantics are a bit hard to discern here. Would you mind adding a comment explaining that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was already a comment, but I forgot to update it.
See 2d0e91c.

@ivanpauno ivanpauno mentioned this pull request May 1, 2020
Merged
@ivanpauno
Update comment
2d0e91c 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

ivanpauno commented May 1, 2020 •
edited
Loading

CI, testing only test_rclcpp test_executor test.
Using --retest-until-fail 10 to check if this actually makes the test more reliable.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Use bigger tolerance
2184dbc 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

The test seemed to be "less" flaky in the above run, but it still failed after a bunch of repetitions.
I incremented the time tolerance, to see if it doesn't fail anymore:

  • Windows Build Status

Based on internet speculations (as windows doesn't officially clarify anything), it seems that windows server distributions have a bigger time slice that desktop versions. That's (maybe) why time based tests started failing more frequently when we switched to containerized builds.
In windows performance options, there's even a Processor scheduling option, where you can choose between "adjust performance for Programs"/"adjust performance for Backgroud services".
The default in Windows Server installations is the latter, though in desktops is the former.

@ivanpauno ivanpauno requested a review from clalancette May 6, 2020 13:11
@ivanpauno
Copy link
Member Author

@clalancette let me know if you think this is ready.
I can further increase time tolerance to 500ms, as timing isn't really the point of the test.

clalancette
clalancette approved these changes May 6, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clalancette let me know if you think this is ready.
I can further increase time tolerance to 500ms, as timing isn't really the point of the test.

Yeah, I think that would be a good idea. As long as it completes in less than 2 seconds, I think we've proven what the test is trying to prove. I'll still approve.

@ivanpauno
Use bigger time tolerance
7d5cdaf 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

CI:

  • Windows Build Status

@ivanpauno ivanpauno merged commit 2f157db into master May 6, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/make-spin-some-max-duration-more-reliable branch May 6, 2020 16:01
@ivanpauno ivanpauno mentioned this pull request May 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@sloretz sloretz Awaiting requested review from sloretz

Assignees

@ivanpauno ivanpauno

Labels
in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ivanpauno @clalancette