From 18d3df197abf065b5defbbd91ad2a031460eeba1 Mon Sep 17 00:00:00 2001 From: williamckha Date: Wed, 25 Sep 2024 00:03:18 -0700 Subject: [PATCH 1/5] Fix observer test flakiness using FakeClock --- src/software/multithreading/BUILD | 1 + src/software/multithreading/observer.hpp | 25 +++++----- src/software/multithreading/observer_test.cpp | 19 ++++---- src/software/test_util/BUILD | 6 +++ src/software/test_util/fake_clock.cpp | 15 ++++++ src/software/test_util/fake_clock.h | 47 +++++++++++++++++++ 6 files changed, 92 insertions(+), 21 deletions(-) create mode 100644 src/software/test_util/fake_clock.cpp create mode 100644 src/software/test_util/fake_clock.h diff --git a/src/software/multithreading/BUILD b/src/software/multithreading/BUILD index e6b2193ef4..c051206e6c 100644 --- a/src/software/multithreading/BUILD +++ b/src/software/multithreading/BUILD @@ -64,6 +64,7 @@ cc_test( deps = [ ":observer", "//shared/test_util:tbots_gtest_main", + "//software/test_util:fake_clock", ], ) diff --git a/src/software/multithreading/observer.hpp b/src/software/multithreading/observer.hpp index 02cea7784f..8bf17142f2 100644 --- a/src/software/multithreading/observer.hpp +++ b/src/software/multithreading/observer.hpp @@ -8,8 +8,9 @@ * "Subject" to receive new instances of type T when they are available * * @tparam T The type of object this class is observing + * @tparam Clock A clock that satisfies the TrivialClock requirements */ -template +template class Observer { public: @@ -79,34 +80,34 @@ class Observer boost::circular_buffer receive_time_buffer; }; -template -Observer::Observer(size_t buffer_size, bool log_buffer_full) +template +Observer::Observer(size_t buffer_size, bool log_buffer_full) : buffer(buffer_size, log_buffer_full), receive_time_buffer(TIME_BUFFER_SIZE) { } -template -void Observer::receiveValue(T val) +template +void Observer::receiveValue(T val) { receive_time_buffer.push_back(std::chrono::duration_cast( - std::chrono::steady_clock::now().time_since_epoch())); + Clock::now().time_since_epoch())); buffer.push(std::move(val)); } -template -std::optional Observer::popMostRecentlyReceivedValue(Duration max_wait_time) +template +std::optional Observer::popMostRecentlyReceivedValue(Duration max_wait_time) { return buffer.popMostRecentlyAddedValue(max_wait_time); } -template -std::optional Observer::popLeastRecentlyReceivedValue(Duration max_wait_time) +template +std::optional Observer::popLeastRecentlyReceivedValue(Duration max_wait_time) { return buffer.popLeastRecentlyAddedValue(max_wait_time); } -template -double Observer::getDataReceivedPerSecond() +template +double Observer::getDataReceivedPerSecond() { if (receive_time_buffer.empty()) { diff --git a/src/software/multithreading/observer_test.cpp b/src/software/multithreading/observer_test.cpp index f75209c801..7117b44333 100644 --- a/src/software/multithreading/observer_test.cpp +++ b/src/software/multithreading/observer_test.cpp @@ -4,7 +4,9 @@ #include -class TestObserver : public Observer +#include "software/test_util/fake_clock.h" + +class TestObserver : public Observer { public: std::optional getMostRecentValueFromBufferWrapper() @@ -19,35 +21,34 @@ namespace TestUtil * Tests getDataReceivedPerSecond by filling the buffer * * @param test_observer The observer to test - * @param data_received_period_milliseconds The period between receiving data + * @param data_received_period_ms The period between receiving data in milliseconds * @param number_of_messages number of messages to send to the buffer * * @return AssertionSuccess the observer returns the correct data received per second */ ::testing::AssertionResult testGetDataReceivedPerSecondByFillingBuffer( - TestObserver test_observer, unsigned int data_received_period_milliseconds, + TestObserver test_observer, unsigned int data_received_period_ms, unsigned int number_of_messages) { - auto wall_time_start = std::chrono::steady_clock::now(); + auto wall_time_start = FakeClock::now(); for (unsigned int i = 0; i < number_of_messages; i++) { test_observer.receiveValue(i); - std::this_thread::sleep_for( - std::chrono::milliseconds(data_received_period_milliseconds)); + FakeClock::advance(std::chrono::milliseconds(data_received_period_ms)); } - auto wall_time_now = std::chrono::steady_clock::now(); + auto wall_time_now = FakeClock::now(); double test_duration_s = static_cast(std::chrono::duration_cast( wall_time_now - wall_time_start) .count()) * SECONDS_PER_MILLISECOND; double scaling_factor = - test_duration_s / (data_received_period_milliseconds * + test_duration_s / (data_received_period_ms * SECONDS_PER_MILLISECOND * number_of_messages); double expected_actual_difference = std::abs(test_observer.getDataReceivedPerSecond() - - 1 / (data_received_period_milliseconds * SECONDS_PER_MILLISECOND) * + 1 / (data_received_period_ms * SECONDS_PER_MILLISECOND) * scaling_factor); if (expected_actual_difference < 50) { diff --git a/src/software/test_util/BUILD b/src/software/test_util/BUILD index 5cd15e0620..20db356ece 100644 --- a/src/software/test_util/BUILD +++ b/src/software/test_util/BUILD @@ -33,6 +33,12 @@ cc_library( ], ) +cc_library( + name = "fake_clock", + srcs = ["fake_clock.cpp"], + hdrs = ["fake_clock.h"], +) + cc_test( name = "test_util_test", srcs = ["test_util_test.cpp"], diff --git a/src/software/test_util/fake_clock.cpp b/src/software/test_util/fake_clock.cpp new file mode 100644 index 0000000000..c96f7df3f4 --- /dev/null +++ b/src/software/test_util/fake_clock.cpp @@ -0,0 +1,15 @@ +#include "fake_clock.h" + +FakeClock::time_point FakeClock::now_; + +FakeClock::time_point FakeClock::now() noexcept { + return now_; +} + +void FakeClock::advance(duration time) noexcept { + now_ += time; +} + +void FakeClock::reset() noexcept { + now_ = FakeClock::time_point(); +} diff --git a/src/software/test_util/fake_clock.h b/src/software/test_util/fake_clock.h new file mode 100644 index 0000000000..1054fd200b --- /dev/null +++ b/src/software/test_util/fake_clock.h @@ -0,0 +1,47 @@ +#pragma once + +#include +#include +#include + +/** + * Fake clock convenient for testing purposes. + * + * Satisfies the TrivialClock requirements and thus can replace any standard clock + * in the chrono library (e.g. std::chrono::steady_clock). + */ +class FakeClock { + public: + typedef uint64_t rep; + typedef std::nano period; + typedef std::chrono::duration duration; + typedef std::chrono::time_point time_point; + inline static const bool is_steady = false; + + /** + * Returns the current time point. + * + * @return the current time point + */ + static time_point now() noexcept; + + /** + * Advances the current time point by the given amount of time. + * + * @param time the amount of time to advance the current time point by + */ + static void advance(duration time) noexcept; + + /** + * Resets the current time point to the clock's epoch + * (the origin of fake_clock::time_point) + */ + static void reset() noexcept; + + private: + FakeClock() = delete; + ~FakeClock() = delete; + FakeClock(FakeClock const&) = delete; + + static time_point now_; +}; \ No newline at end of file From 1f7c41f75870bf3ee199c7e3885d5f91fc07e4c6 Mon Sep 17 00:00:00 2001 From: williamckha Date: Wed, 25 Sep 2024 10:33:47 -0700 Subject: [PATCH 2/5] Add FakeClock unit tests --- src/software/test_util/BUILD | 9 +++++ src/software/test_util/fake_clock.h | 4 +++ src/software/test_util/fake_clock_test.cpp | 39 ++++++++++++++++++++++ 3 files changed, 52 insertions(+) create mode 100644 src/software/test_util/fake_clock_test.cpp diff --git a/src/software/test_util/BUILD b/src/software/test_util/BUILD index 20db356ece..5d417acb3c 100644 --- a/src/software/test_util/BUILD +++ b/src/software/test_util/BUILD @@ -56,3 +56,12 @@ cc_test( "//shared/test_util:tbots_gtest_main", ], ) + +cc_test( + name = "fake_clock_test", + srcs = ["fake_clock_test.cpp"], + deps = [ + ":fake_clock", + "//shared/test_util:tbots_gtest_main", + ] +) diff --git a/src/software/test_util/fake_clock.h b/src/software/test_util/fake_clock.h index 1054fd200b..f8b12fd09f 100644 --- a/src/software/test_util/fake_clock.h +++ b/src/software/test_util/fake_clock.h @@ -7,6 +7,10 @@ /** * Fake clock convenient for testing purposes. * + * Tests that rely on sleeping can be non-deterministic and flaky since they + * depend on real time to elapse. Use this clock instead, which can be explicitly + * advanced without sleeping. + * * Satisfies the TrivialClock requirements and thus can replace any standard clock * in the chrono library (e.g. std::chrono::steady_clock). */ diff --git a/src/software/test_util/fake_clock_test.cpp b/src/software/test_util/fake_clock_test.cpp new file mode 100644 index 0000000000..07aa4b16a4 --- /dev/null +++ b/src/software/test_util/fake_clock_test.cpp @@ -0,0 +1,39 @@ +#include "software/test_util/fake_clock.h" + +#include + +TEST(FakeClockTest, time_point_now_should_not_change_without_advancing) +{ + FakeClock::reset(); + FakeClock::time_point t0 = FakeClock::now(); + FakeClock::time_point t1 = FakeClock::now(); + EXPECT_EQ(std::chrono::milliseconds(0), t1 - t0); +} + +TEST(FakeClockTest, time_point_now_should_move_forward_after_advancing) +{ + FakeClock::reset(); + FakeClock::time_point t0 = FakeClock::now(); + + FakeClock::advance(std::chrono::milliseconds(100)); + FakeClock::time_point t1 = FakeClock::now(); + EXPECT_EQ(std::chrono::milliseconds(100), t1 - t0); + + FakeClock::advance(std::chrono::milliseconds(50)); + FakeClock::time_point t2 = FakeClock::now(); + EXPECT_EQ(std::chrono::milliseconds(150), t2 - t0); +} + +TEST(FakeClockTest, reset_should_set_time_point_now_to_clock_epoch) +{ + FakeClock::reset(); + FakeClock::time_point t0 = FakeClock::now(); + + FakeClock::advance(std::chrono::milliseconds(1000)); + FakeClock::time_point t1 = FakeClock::now(); + EXPECT_EQ(std::chrono::milliseconds(1000), t1 - t0); + + FakeClock::reset(); + FakeClock::time_point t2 = FakeClock::now(); + EXPECT_EQ(std::chrono::milliseconds(0), t2 - t0); +} From cedcfe6c5f172f0254f3afad38d21f5a7d4a4e75 Mon Sep 17 00:00:00 2001 From: williamckha Date: Wed, 25 Sep 2024 10:34:15 -0700 Subject: [PATCH 3/5] Run formatting --- src/software/multithreading/observer_test.cpp | 11 +++++------ src/software/test_util/BUILD | 2 +- src/software/test_util/fake_clock.cpp | 9 ++++++--- src/software/test_util/fake_clock.h | 9 +++++---- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/software/multithreading/observer_test.cpp b/src/software/multithreading/observer_test.cpp index 7117b44333..277b839689 100644 --- a/src/software/multithreading/observer_test.cpp +++ b/src/software/multithreading/observer_test.cpp @@ -44,12 +44,11 @@ namespace TestUtil .count()) * SECONDS_PER_MILLISECOND; double scaling_factor = - test_duration_s / (data_received_period_ms * - SECONDS_PER_MILLISECOND * number_of_messages); - double expected_actual_difference = - std::abs(test_observer.getDataReceivedPerSecond() - - 1 / (data_received_period_ms * SECONDS_PER_MILLISECOND) * - scaling_factor); + test_duration_s / + (data_received_period_ms * SECONDS_PER_MILLISECOND * number_of_messages); + double expected_actual_difference = std::abs( + test_observer.getDataReceivedPerSecond() - + 1 / (data_received_period_ms * SECONDS_PER_MILLISECOND) * scaling_factor); if (expected_actual_difference < 50) { return ::testing::AssertionSuccess(); diff --git a/src/software/test_util/BUILD b/src/software/test_util/BUILD index 5d417acb3c..f8d8d1c2ab 100644 --- a/src/software/test_util/BUILD +++ b/src/software/test_util/BUILD @@ -63,5 +63,5 @@ cc_test( deps = [ ":fake_clock", "//shared/test_util:tbots_gtest_main", - ] + ], ) diff --git a/src/software/test_util/fake_clock.cpp b/src/software/test_util/fake_clock.cpp index c96f7df3f4..e3f95068f4 100644 --- a/src/software/test_util/fake_clock.cpp +++ b/src/software/test_util/fake_clock.cpp @@ -2,14 +2,17 @@ FakeClock::time_point FakeClock::now_; -FakeClock::time_point FakeClock::now() noexcept { +FakeClock::time_point FakeClock::now() noexcept +{ return now_; } -void FakeClock::advance(duration time) noexcept { +void FakeClock::advance(duration time) noexcept +{ now_ += time; } -void FakeClock::reset() noexcept { +void FakeClock::reset() noexcept +{ now_ = FakeClock::time_point(); } diff --git a/src/software/test_util/fake_clock.h b/src/software/test_util/fake_clock.h index f8b12fd09f..13f68af7a6 100644 --- a/src/software/test_util/fake_clock.h +++ b/src/software/test_util/fake_clock.h @@ -14,7 +14,8 @@ * Satisfies the TrivialClock requirements and thus can replace any standard clock * in the chrono library (e.g. std::chrono::steady_clock). */ -class FakeClock { +class FakeClock +{ public: typedef uint64_t rep; typedef std::nano period; @@ -43,9 +44,9 @@ class FakeClock { static void reset() noexcept; private: - FakeClock() = delete; - ~FakeClock() = delete; + FakeClock() = delete; + ~FakeClock() = delete; FakeClock(FakeClock const&) = delete; static time_point now_; -}; \ No newline at end of file +}; From 9cd4fd3d7f57dfb4300e7c55f1fcd4472f65677d Mon Sep 17 00:00:00 2001 From: williamckha Date: Sun, 29 Sep 2024 11:00:22 -0700 Subject: [PATCH 4/5] Use using instead of typedef --- src/software/test_util/fake_clock.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/software/test_util/fake_clock.h b/src/software/test_util/fake_clock.h index 13f68af7a6..eda9cbb688 100644 --- a/src/software/test_util/fake_clock.h +++ b/src/software/test_util/fake_clock.h @@ -17,10 +17,10 @@ class FakeClock { public: - typedef uint64_t rep; - typedef std::nano period; - typedef std::chrono::duration duration; - typedef std::chrono::time_point time_point; + using rep = uint64_t; + using period = std::nano; + using duration = std::chrono::duration; + using time_point = std::chrono::time_point; inline static const bool is_steady = false; /** From e38c5b7f90a1189b82cb05a4747963190377d69f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci-lite[bot]" <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Date: Sun, 29 Sep 2024 18:08:58 +0000 Subject: [PATCH 5/5] [pre-commit.ci lite] apply automatic fixes --- src/software/test_util/fake_clock.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/software/test_util/fake_clock.h b/src/software/test_util/fake_clock.h index eda9cbb688..371cf1fde1 100644 --- a/src/software/test_util/fake_clock.h +++ b/src/software/test_util/fake_clock.h @@ -17,10 +17,10 @@ class FakeClock { public: - using rep = uint64_t; - using period = std::nano; - using duration = std::chrono::duration; - using time_point = std::chrono::time_point; + using rep = uint64_t; + using period = std::nano; + using duration = std::chrono::duration; + using time_point = std::chrono::time_point; inline static const bool is_steady = false; /**