From bdba2b578dd1c240625789607a911ff7a68571b1 Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Wed, 5 Jul 2023 17:15:12 -0700 Subject: [PATCH] Fix flaky recurrent timer test. Do not rely on sleep in tests, but wait for the events to happen before timeout. Set the timeout to be large enough (5s) so that the tests are more stable. Test: atest RecurrentTimerTest Bug: 289759453 Change-Id: Iff271ca440426871ed3a798bab50b9532f9256f0 Merged-In: Iff271ca440426871ed3a798bab50b9532f9256f0 (cherry picked from commit 76d7fbf6a1eac23361510301c0bb8e2f857c66de) --- .../utils/common/test/RecurrentTimerTest.cpp | 62 ++++++++++++------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/automotive/vehicle/aidl/impl/utils/common/test/RecurrentTimerTest.cpp b/automotive/vehicle/aidl/impl/utils/common/test/RecurrentTimerTest.cpp index 141efc135b..62046f38dc 100644 --- a/automotive/vehicle/aidl/impl/utils/common/test/RecurrentTimerTest.cpp +++ b/automotive/vehicle/aidl/impl/utils/common/test/RecurrentTimerTest.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include @@ -28,6 +29,8 @@ namespace hardware { namespace automotive { namespace vehicle { +using ::android::base::ScopedLockAssertion; + class RecurrentTimerTest : public testing::Test { public: std::shared_ptr getCallback(size_t token) { @@ -35,6 +38,15 @@ class RecurrentTimerTest : public testing::Test { std::scoped_lock lockGuard(mLock); mCallbacks.push_back(token); + mCond.notify_all(); + }); + } + + bool waitForCalledCallbacks(size_t count, size_t timeoutInMs) { + std::unique_lock uniqueLock(mLock); + return mCond.wait_for(uniqueLock, std::chrono::milliseconds(timeoutInMs), [this, count] { + ScopedLockAssertion lockAssertion(mLock); + return mCallbacks.size() >= count; }); } @@ -54,6 +66,7 @@ class RecurrentTimerTest : public testing::Test { } private: + std::condition_variable mCond; std::mutex mLock; std::vector mCallbacks GUARDED_BY(mLock); }; @@ -66,12 +79,11 @@ TEST_F(RecurrentTimerTest, testRegisterCallback) { auto action = getCallback(0); timer.registerTimerCallback(interval, action); - std::this_thread::sleep_for(std::chrono::seconds(1)); + // Should only takes 1s, use 5s as timeout to be safe. + ASSERT_TRUE(waitForCalledCallbacks(/* count= */ 10u, /* timeoutInMs= */ 5000)) + << "Not enough callbacks called before timeout"; timer.unregisterTimerCallback(action); - - // Theoretically trigger 10 times, but check for at least 9 times to be stable. - ASSERT_GE(getCalledCallbacks().size(), static_cast(9)); } TEST_F(RecurrentTimerTest, testRegisterUnregisterRegister) { @@ -92,10 +104,11 @@ TEST_F(RecurrentTimerTest, testRegisterUnregisterRegister) { timer.registerTimerCallback(interval, action); - std::this_thread::sleep_for(std::chrono::seconds(1)); + // Should only takes 1s, use 5s as timeout to be safe. + ASSERT_TRUE(waitForCalledCallbacks(/* count= */ 10u, /* timeoutInMs= */ 5000)) + << "Not enough callbacks called before timeout"; - // Theoretically trigger 10 times, but check for at least 9 times to be stable. - ASSERT_GE(getCalledCallbacks().size(), static_cast(9)); + timer.unregisterTimerCallback(action); } TEST_F(RecurrentTimerTest, testDestroyTimerWithCallback) { @@ -114,7 +127,9 @@ TEST_F(RecurrentTimerTest, testDestroyTimerWithCallback) { std::this_thread::sleep_for(std::chrono::milliseconds(200)); - ASSERT_TRUE(getCalledCallbacks().empty()); + // Should be 0, but in rare cases there might be 1 events in the queue while the timer is + // being destroyed. + ASSERT_LE(getCalledCallbacks().size(), 1u); } TEST_F(RecurrentTimerTest, testRegisterMultipleCallbacks) { @@ -132,7 +147,11 @@ TEST_F(RecurrentTimerTest, testRegisterMultipleCallbacks) { auto action3 = getCallback(3); timer.registerTimerCallback(interval3, action3); - std::this_thread::sleep_for(std::chrono::seconds(1)); + // In 1s, we should generate 10 + 20 + 33 = 63 events. + // Here we are waiting for more events to make sure we receive enough events for each actions. + // Use 5s as timeout to be safe. + ASSERT_TRUE(waitForCalledCallbacks(/* count= */ 70u, /* timeoutInMs= */ 5000)) + << "Not enough callbacks called before timeout"; timer.unregisterTimerCallback(action1); timer.unregisterTimerCallback(action2); @@ -152,20 +171,18 @@ TEST_F(RecurrentTimerTest, testRegisterMultipleCallbacks) { action3Count++; } } - // Theoretically trigger 10 times, but check for at least 9 times to be stable. - ASSERT_GE(action1Count, static_cast(9)); - // Theoretically trigger 20 times, but check for at least 15 times to be stable. - ASSERT_GE(action2Count, static_cast(15)); - // Theoretically trigger 33 times, but check for at least 25 times to be stable. - ASSERT_GE(action3Count, static_cast(25)); + + ASSERT_GE(action1Count, static_cast(10)); + ASSERT_GE(action2Count, static_cast(20)); + ASSERT_GE(action3Count, static_cast(33)); } TEST_F(RecurrentTimerTest, testRegisterSameCallbackMultipleTimes) { RecurrentTimer timer; - // 0.02s - int64_t interval1 = 20000000; - // 0.01s - int64_t interval2 = 10000000; + // 0.2s + int64_t interval1 = 200'000'000; + // 0.1s + int64_t interval2 = 100'000'000; auto action = getCallback(0); for (int i = 0; i < 10; i++) { @@ -175,10 +192,9 @@ TEST_F(RecurrentTimerTest, testRegisterSameCallbackMultipleTimes) { clearCalledCallbacks(); - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - - // Theoretically trigger 10 times, but check for at least 9 times to be stable. - ASSERT_GE(getCalledCallbacks().size(), static_cast(9)); + // Should only takes 1s, use 5s as timeout to be safe. + ASSERT_TRUE(waitForCalledCallbacks(/* count= */ 10u, /* timeoutInMs= */ 5000)) + << "Not enough callbacks called before timeout"; timer.unregisterTimerCallback(action);