From 9d5aaf5bae6d39f98446b928e274ffd5b986ab6e Mon Sep 17 00:00:00 2001 From: Alberto Soragna Date: Thu, 28 Mar 2024 06:23:32 -0700 Subject: [PATCH] fix flakiness in TestTimersManager unit-test (#2468) the previous version of the test was relying on the assumption that a timer with 1ms period gets called at least 6 times if the main thread waits 15ms. this is true most of the times, but it's not guaranteed, especially when running the test on windows CI servers. the new version of the test makes no assumptions on how much time it takes for the timers manager to invoke the timers, but rather focuses on ensuring that they are called the right amount of times, which is what's important for the purpose of the test Signed-off-by: Alberto Soragna --- rclcpp/test/rclcpp/test_timers_manager.cpp | 50 ++++++++++++++++++---- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/rclcpp/test/rclcpp/test_timers_manager.cpp b/rclcpp/test/rclcpp/test_timers_manager.cpp index 0e49da08e1..90d321c188 100644 --- a/rclcpp/test/rclcpp/test_timers_manager.cpp +++ b/rclcpp/test/rclcpp/test_timers_manager.cpp @@ -367,22 +367,23 @@ TEST_F(TestTimersManager, check_one_timer_cancel_doesnt_affect_other_timers) auto timers_manager = std::make_shared( rclcpp::contexts::get_global_default_context()); - size_t t1_runs = 0; + std::atomic t1_runs = 0; + const size_t cancel_iter = 5; std::shared_ptr t1; // After a while cancel t1. Don't remove it though. // Simulates typical usage in a Node where a timer is cancelled but not removed, // since typical users aren't going to mess around with the timer manager. t1 = TimerT::make_shared( 1ms, - [&t1_runs, &t1]() { + [&t1_runs, &t1, cancel_iter]() { t1_runs++; - if (t1_runs == 5) { + if (t1_runs == cancel_iter) { t1->cancel(); } }, rclcpp::contexts::get_global_default_context()); - size_t t2_runs = 0; + std::atomic t2_runs = 0; auto t2 = TimerT::make_shared( 1ms, [&t2_runs]() { @@ -397,11 +398,42 @@ TEST_F(TestTimersManager, check_one_timer_cancel_doesnt_affect_other_timers) // Start timers thread timers_manager->start(); - std::this_thread::sleep_for(15ms); + // Wait for t1 to be canceled + auto loop_start_time = std::chrono::high_resolution_clock::now(); + while (!t1->is_canceled()) { + auto now = std::chrono::high_resolution_clock::now(); + if (now - loop_start_time >= std::chrono::seconds(30)) { + FAIL() << "timeout waiting for t1 to be canceled"; + break; + } + std::this_thread::sleep_for(3ms); + } + + EXPECT_TRUE(t1->is_canceled()); + EXPECT_FALSE(t2->is_canceled()); + EXPECT_EQ(t1_runs, cancel_iter); + + // Verify that t2 is still being invoked + const size_t start_t2_runs = t2_runs; + const size_t num_t2_extra_runs = 6; + loop_start_time = std::chrono::high_resolution_clock::now(); + while (t2_runs < start_t2_runs + num_t2_extra_runs) { + auto now = std::chrono::high_resolution_clock::now(); + if (now - loop_start_time >= std::chrono::seconds(30)) { + FAIL() << "timeout waiting for t2 to do some runs"; + break; + } + std::this_thread::sleep_for(3ms); + } + + EXPECT_TRUE(t1->is_canceled()); + EXPECT_FALSE(t2->is_canceled()); + // t1 hasn't run since before + EXPECT_EQ(t1_runs, cancel_iter); + // t2 has run the expected additional number of times + EXPECT_GE(t2_runs, start_t2_runs + num_t2_extra_runs); + // the t2 runs are strictly more than the t1 runs + EXPECT_GT(t2_runs, t1_runs); - // t1 has stopped running - EXPECT_NE(t1_runs, t2_runs); - // Check that t2 has significantly more calls - EXPECT_LT(t1_runs + 5, t2_runs); timers_manager->stop(); }