From 1314104ddfac50e4984c440ed78e4ce7067c98c8 Mon Sep 17 00:00:00 2001 From: Kevin Schoedel <67607049+kpschoedel@users.noreply.github.com> Date: Wed, 8 Dec 2021 16:06:26 -0500 Subject: [PATCH] Add unit tests for SystemTimer.h (#12742) #### Problem Only the higher-level `System::Layer` timer operations had tests; the utility classes in `system/SystemTimer.h` had no unit tests, and a recent refactor (#12628) introduced a bug that a proper unit test would have caught. Fixes #12729 Add unit tests for SystemTimer.h #### Change overview What's in this PR - Add tests covering `TimerData`, `TimeList`, and `TimerPool`. - Changed these helpers to take a `Timestamp` rather than a `Timeout`. - Fixed `TimerList::Remove(Node*)` to allow an empty list or null argument (matching its description). #### Testing Quis custodiet ipsos custodes? --- src/system/SystemLayerImplLwIP.cpp | 4 +- src/system/SystemLayerImplSelect.cpp | 4 +- src/system/SystemTimer.cpp | 35 +++--- src/system/SystemTimer.h | 18 ++-- src/system/tests/TestSystemTimer.cpp | 153 +++++++++++++++++++++++++++ 5 files changed, 185 insertions(+), 29 deletions(-) diff --git a/src/system/SystemLayerImplLwIP.cpp b/src/system/SystemLayerImplLwIP.cpp index 7c03a18600be45..605f78daaa46bd 100644 --- a/src/system/SystemLayerImplLwIP.cpp +++ b/src/system/SystemLayerImplLwIP.cpp @@ -56,7 +56,7 @@ CHIP_ERROR LayerImplLwIP::StartTimer(Clock::Timeout delay, TimerCompleteCallback CancelTimer(onComplete, appState); - TimerList::Node * timer = mTimerPool.Create(*this, delay, onComplete, appState); + TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp() + delay, onComplete, appState); VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY); if (mTimerList.Add(timer) == timer) @@ -87,7 +87,7 @@ CHIP_ERROR LayerImplLwIP::ScheduleWork(TimerCompleteCallback onComplete, void * { VerifyOrReturnError(mLayerState.IsInitialized(), CHIP_ERROR_INCORRECT_STATE); - TimerList::Node * timer = mTimerPool.Create(*this, System::Clock::kZero, onComplete, appState); + TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp(), onComplete, appState); VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY); return ScheduleLambda([this, timer] { this->mTimerPool.Invoke(timer); }); diff --git a/src/system/SystemLayerImplSelect.cpp b/src/system/SystemLayerImplSelect.cpp index 88d915ca7d1ae7..0ac3bd3e7ad7bc 100644 --- a/src/system/SystemLayerImplSelect.cpp +++ b/src/system/SystemLayerImplSelect.cpp @@ -124,7 +124,7 @@ CHIP_ERROR LayerImplSelect::StartTimer(Clock::Timeout delay, TimerCompleteCallba CancelTimer(onComplete, appState); - TimerList::Node * timer = mTimerPool.Create(*this, delay, onComplete, appState); + TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp() + delay, onComplete, appState); VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY); #if CHIP_SYSTEM_CONFIG_USE_DISPATCH @@ -186,7 +186,7 @@ CHIP_ERROR LayerImplSelect::ScheduleWork(TimerCompleteCallback onComplete, void CancelTimer(onComplete, appState); - TimerList::Node * timer = mTimerPool.Create(*this, Clock::kZero, onComplete, appState); + TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp(), onComplete, appState); VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY); #if CHIP_SYSTEM_CONFIG_USE_DISPATCH diff --git a/src/system/SystemTimer.cpp b/src/system/SystemTimer.cpp index 5764228e844322..f966564e788418 100644 --- a/src/system/SystemTimer.cpp +++ b/src/system/SystemTimer.cpp @@ -67,29 +67,30 @@ TimerList::Node * TimerList::Add(TimerList::Node * add) TimerList::Node * TimerList::Remove(TimerList::Node * remove) { - VerifyOrDie(mEarliestTimer != nullptr); - - if (remove == mEarliestTimer) - { - mEarliestTimer = remove->mNextTimer; - } - else + if (mEarliestTimer != nullptr && remove != nullptr) { - TimerList::Node * lTimer = mEarliestTimer; - - while (lTimer->mNextTimer) + if (remove == mEarliestTimer) + { + mEarliestTimer = remove->mNextTimer; + } + else { - if (remove == lTimer->mNextTimer) + TimerList::Node * lTimer = mEarliestTimer; + + while (lTimer->mNextTimer) { - lTimer->mNextTimer = remove->mNextTimer; - break; - } + if (remove == lTimer->mNextTimer) + { + lTimer->mNextTimer = remove->mNextTimer; + break; + } - lTimer = lTimer->mNextTimer; + lTimer = lTimer->mNextTimer; + } } - } - remove->mNextTimer = nullptr; + remove->mNextTimer = nullptr; + } return mEarliestTimer; } diff --git a/src/system/SystemTimer.h b/src/system/SystemTimer.h index 59457d2c855c00..54b07f8c092f49 100644 --- a/src/system/SystemTimer.h +++ b/src/system/SystemTimer.h @@ -44,6 +44,7 @@ namespace chip { namespace System { class Layer; +class TestTimer; /** * Basic Timer information: time and callback. @@ -57,7 +58,7 @@ class DLL_EXPORT TimerData Callback(Layer & systemLayer, TimerCompleteCallback onComplete, void * appState) : mSystemLayer(&systemLayer), mOnComplete(onComplete), mAppState(appState) {} - void Invoke() { mOnComplete(mSystemLayer, mAppState); } + void Invoke() const { mOnComplete(mSystemLayer, mAppState); } const TimerCompleteCallback & GetOnComplete() const { return mOnComplete; } void * GetAppState() const { return mAppState; } Layer * GetSystemLayer() const { return mSystemLayer; } @@ -68,8 +69,8 @@ class DLL_EXPORT TimerData void * mAppState; }; - TimerData(Layer & systemLayer, System::Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState) : - mAwakenTime(SystemClock().GetMonotonicTimestamp() + delay), mCallback(systemLayer, onComplete, appState) + TimerData(Layer & systemLayer, System::Clock::Timestamp awakenTime, TimerCompleteCallback onComplete, void * appState) : + mAwakenTime(awakenTime), mCallback(systemLayer, onComplete, appState) {} ~TimerData() = default; @@ -106,8 +107,8 @@ class TimerList class Node : public TimerData { public: - Node(Layer & systemLayer, System::Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState) : - TimerData(systemLayer, delay, onComplete, appState), mNextTimer(nullptr) + Node(Layer & systemLayer, System::Clock::Timestamp awakenTime, TimerCompleteCallback onComplete, void * appState) : + TimerData(systemLayer, awakenTime, onComplete, appState), mNextTimer(nullptr) {} Node * mNextTimer; }; @@ -160,7 +161,7 @@ class TimerList /** * Test whether there are any timers. */ - bool Empty() const { return mEarliestTimer != nullptr; } + bool Empty() const { return mEarliestTimer == nullptr; } /** * Remove and return all timers that expire before the given time @a t. @@ -188,9 +189,9 @@ class TimerPool /** * Create a new timer from the pool. */ - Timer * Create(Layer & systemLayer, System::Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState) + Timer * Create(Layer & systemLayer, System::Clock::Timestamp awakenTime, TimerCompleteCallback onComplete, void * appState) { - Timer * timer = mTimerPool.CreateObject(systemLayer, delay, onComplete, appState); + Timer * timer = mTimerPool.CreateObject(systemLayer, awakenTime, onComplete, appState); SYSTEM_STATS_INCREMENT(Stats::kSystemLayer_NumTimers); return timer; } @@ -224,6 +225,7 @@ class TimerPool } private: + friend class TestTimer; ObjectPool mTimerPool; }; diff --git a/src/system/tests/TestSystemTimer.cpp b/src/system/tests/TestSystemTimer.cpp index 9b902f246ee0b6..6520be21959bfa 100644 --- a/src/system/tests/TestSystemTimer.cpp +++ b/src/system/tests/TestSystemTimer.cpp @@ -168,6 +168,158 @@ static void CheckStarvation(nlTestSuite * inSuite, void * aContext) ServiceEvents(lSys); } +// Test the implementation helper classes TimerPool, TimerList, and TimerData. +namespace chip { +namespace System { +class TestTimer +{ +public: + static void CheckTimerPool(nlTestSuite * inSuite, void * aContext); +}; +} // namespace System +} // namespace chip + +void chip::System::TestTimer::CheckTimerPool(nlTestSuite * inSuite, void * aContext) +{ + TestContext & testContext = *static_cast(aContext); + Layer & systemLayer = *testContext.mLayer; + nlTestSuite * const suite = testContext.mTestSuite; + + using Timer = TimerList::Node; + struct TestState + { + int count = 0; + static void Increment(Layer * layer, void * state) { ++static_cast(state)->count; } + static void Reset(Layer * layer, void * state) { static_cast(state)->count = 0; } + }; + TestState testState; + + using namespace Clock::Literals; + struct + { + Clock::Timestamp awakenTime; + TimerCompleteCallback onComplete; + Timer * timer; + } testTimer[] = { + { 111_ms, TestState::Increment }, // 0 + { 100_ms, TestState::Increment }, // 1 + { 202_ms, TestState::Reset }, // 2 + { 303_ms, TestState::Increment }, // 3 + }; + + TimerPool pool; + NL_TEST_ASSERT(suite, pool.mTimerPool.Allocated() == 0); + SYSTEM_STATS_RESET(Stats::kSystemLayer_NumTimers); + + // Test TimerPool::Create() and TimerData accessors. + + for (auto & timer : testTimer) + { + timer.timer = pool.Create(systemLayer, timer.awakenTime, timer.onComplete, &testState); + } +#if CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS + NL_TEST_ASSERT(suite, Stats::GetResourcesInUse()[Stats::kSystemLayer_NumTimers] == 4); +#endif // CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS + + for (auto & timer : testTimer) + { + NL_TEST_ASSERT(suite, timer.timer != nullptr); + NL_TEST_ASSERT(suite, timer.timer->AwakenTime() == timer.awakenTime); + NL_TEST_ASSERT(suite, timer.timer->GetCallback().GetOnComplete() == timer.onComplete); + NL_TEST_ASSERT(suite, timer.timer->GetCallback().GetAppState() == &testState); + NL_TEST_ASSERT(suite, timer.timer->GetCallback().GetSystemLayer() == &systemLayer); + } + + // Test TimerList operations. + + TimerList list; + NL_TEST_ASSERT(suite, list.Remove(nullptr) == nullptr); + NL_TEST_ASSERT(suite, list.Remove(nullptr, nullptr) == nullptr); + NL_TEST_ASSERT(suite, list.PopEarliest() == nullptr); + NL_TEST_ASSERT(suite, list.PopIfEarlier(500_ms) == nullptr); + NL_TEST_ASSERT(suite, list.Earliest() == nullptr); + NL_TEST_ASSERT(suite, list.Empty()); + + Timer * earliest = list.Add(testTimer[0].timer); // list: () → (0) returns: 0 + NL_TEST_ASSERT(suite, earliest == testTimer[0].timer); + NL_TEST_ASSERT(suite, list.PopIfEarlier(10_ms) == nullptr); + NL_TEST_ASSERT(suite, list.Earliest() == testTimer[0].timer); + NL_TEST_ASSERT(suite, !list.Empty()); + + earliest = list.Add(testTimer[1].timer); // list: (0) → (1 0) returns: 1 + NL_TEST_ASSERT(suite, earliest == testTimer[1].timer); + NL_TEST_ASSERT(suite, list.Earliest() == testTimer[1].timer); + + earliest = list.Add(testTimer[2].timer); // list: (1 0) → (1 0 2) returns: 1 + NL_TEST_ASSERT(suite, earliest == testTimer[1].timer); + NL_TEST_ASSERT(suite, list.Earliest() == testTimer[1].timer); + + earliest = list.Add(testTimer[3].timer); // list: (1 0 2) → (1 0 2 3) returns: 1 + NL_TEST_ASSERT(suite, earliest == testTimer[1].timer); + NL_TEST_ASSERT(suite, list.Earliest() == testTimer[1].timer); + + earliest = list.Remove(earliest); // list: (1 0 2 3) → (0 2 3) returns: 0 + NL_TEST_ASSERT(suite, earliest == testTimer[0].timer); + NL_TEST_ASSERT(suite, list.Earliest() == testTimer[0].timer); + + earliest = list.Remove(TestState::Reset, &testState); // list: (0 2 3) → (0 3) returns: 2 + NL_TEST_ASSERT(suite, earliest == testTimer[2].timer); + NL_TEST_ASSERT(suite, list.Earliest() == testTimer[0].timer); + + earliest = list.PopEarliest(); // list: (0 3) → (3) returns: 0 + NL_TEST_ASSERT(suite, earliest == testTimer[0].timer); + NL_TEST_ASSERT(suite, list.Earliest() == testTimer[3].timer); + + earliest = list.PopIfEarlier(10_ms); // list: (3) → (3) returns: nullptr + NL_TEST_ASSERT(suite, earliest == nullptr); + + earliest = list.PopIfEarlier(500_ms); // list: (3) → () returns: 3 + NL_TEST_ASSERT(suite, earliest == testTimer[3].timer); + NL_TEST_ASSERT(suite, list.Empty()); + + earliest = list.Add(testTimer[3].timer); // list: () → (3) returns: 3 + list.Clear(); // list: (3) → () + NL_TEST_ASSERT(suite, earliest == testTimer[3].timer); + NL_TEST_ASSERT(suite, list.Empty()); + + for (auto & timer : testTimer) + { + list.Add(timer.timer); + } + TimerList early = list.ExtractEarlier(200_ms); // list: (1 0 2 3) → (2 3) returns: (1 0) + NL_TEST_ASSERT(suite, list.PopEarliest() == testTimer[2].timer); + NL_TEST_ASSERT(suite, list.PopEarliest() == testTimer[3].timer); + NL_TEST_ASSERT(suite, list.PopEarliest() == nullptr); + NL_TEST_ASSERT(suite, early.PopEarliest() == testTimer[1].timer); + NL_TEST_ASSERT(suite, early.PopEarliest() == testTimer[0].timer); + NL_TEST_ASSERT(suite, early.PopEarliest() == nullptr); + + // Test TimerPool::Invoke() + NL_TEST_ASSERT(suite, testState.count == 0); + pool.Invoke(testTimer[0].timer); + testTimer[0].timer = nullptr; + NL_TEST_ASSERT(suite, testState.count == 1); + NL_TEST_ASSERT(suite, pool.mTimerPool.Allocated() == 3); +#if CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS + NL_TEST_ASSERT(suite, Stats::GetResourcesInUse()[Stats::kSystemLayer_NumTimers] == 3); +#endif // CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS + + // Test TimerPool::Release() + pool.Release(testTimer[1].timer); + testTimer[1].timer = nullptr; + NL_TEST_ASSERT(suite, testState.count == 1); + NL_TEST_ASSERT(suite, pool.mTimerPool.Allocated() == 2); +#if CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS + NL_TEST_ASSERT(suite, Stats::GetResourcesInUse()[Stats::kSystemLayer_NumTimers] == 2); +#endif // CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS + + pool.ReleaseAll(); + NL_TEST_ASSERT(suite, pool.mTimerPool.Allocated() == 0); +#if CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS + NL_TEST_ASSERT(suite, Stats::GetResourcesInUse()[Stats::kSystemLayer_NumTimers] == 0); +#endif // CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS +} + // Test Suite /** @@ -178,6 +330,7 @@ static const nlTest sTests[] = { NL_TEST_DEF("Timer::TestOverflow", CheckOverflow), NL_TEST_DEF("Timer::TestTimerStarvation", CheckStarvation), + NL_TEST_DEF("Timer::TestTimerPool", chip::System::TestTimer::CheckTimerPool), NL_TEST_SENTINEL() }; // clang-format on