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.

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

Improve test_time_controller test #1012

Merged
merged 3 commits into from
May 14, 2022

Conversation

Blast545
Copy link
Contributor

Should close #992

It's a simple fix for a test that was heavily relying on the the OS not waking up as explained here:
#992 (comment)

This test right before it makes the same assumption:

EXPECT_TRUE(clock.sleep_until(clock.now() + 100));

But as it haven't failed I decided not to change it with this PR.

Signed-off-by: Jorge Perez jjperez@ekumenlabs.com

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 requested a review from a team as a code owner May 12, 2022 20:52
@Blast545 Blast545 requested review from MichaelOrlov and hidmic and removed request for a team May 12, 2022 20:52
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 changed the title Improve test time controller test Improve test_time_controller test May 12, 2022
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@Blast545 Thank you for the PR.
CI build fails due to forgotten

#include <rclcpp/utilities.hpp>

Please add it.

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545
Copy link
Contributor Author

Blast545 commented May 13, 2022

Checks are passing, running full CI:

  • Linux Build Status
  • Linux-aarch64 Build Status -> Known to be failing infra issue.
  • Windows Build Status

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

Approve!

@MichaelOrlov MichaelOrlov merged commit a0de6f8 into master May 14, 2022
@delete-merged-branch delete-merged-branch bot deleted the blast545/improve_test_time_controller_992 branch May 14, 2022 02:31
@emersonknapp
Copy link
Collaborator

emersonknapp commented Jun 10, 2023

I know this is over a year old, but I'm adding some context here for future historians: This PR didn't actually do anything to the test - since rclcpp is not used in the test, the loop never ran, which means that any spurious wakeup still caused the test to fail. The original 100 nanosecond sleep request test likely never failed because it was very unlikely or maybe even impossible for a spurious wakeup to happen in that time frame. The new loop added in this test, on the other hand, slept for a full second (10 million times longer), which made a spurious wakeup far more likely (which is not handled because the loop is never entered), and therefore actually made this test far more likely to fail, which is why we've seen it so flaky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rosbag2_cpp.TimeControllerClockTest.unpaused_sleep_returns_true test fails on Windows
3 participants