Fix flaky Observer tests using fake clock

src/software/multithreading/BUILD

@@ -64,6 +64,7 @@ cc_test(
     deps = [
         ":observer",
         "//shared/test_util:tbots_gtest_main",
+        "//software/test_util:fake_clock",
     ],
 )
 

src/software/multithreading/observer.hpp

@@ -8,8 +8,9 @@
  * "Subject<T>" 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 <typename T>
+template <typename T, typename Clock = std::chrono::steady_clock>
 class Observer
 {
    public:
@@ -79,34 +80,34 @@ class Observer
     boost::circular_buffer<std::chrono::milliseconds> receive_time_buffer;
 };
 
-template <typename T>
-Observer<T>::Observer(size_t buffer_size, bool log_buffer_full)
+template <typename T, typename Clock>
+Observer<T, Clock>::Observer(size_t buffer_size, bool log_buffer_full)
     : buffer(buffer_size, log_buffer_full), receive_time_buffer(TIME_BUFFER_SIZE)
 {
 }
 
-template <typename T>
-void Observer<T>::receiveValue(T val)
+template <typename T, typename Clock>
+void Observer<T, Clock>::receiveValue(T val)
 {
     receive_time_buffer.push_back(std::chrono::duration_cast<std::chrono::milliseconds>(
-        std::chrono::steady_clock::now().time_since_epoch()));
+        Clock::now().time_since_epoch()));
     buffer.push(std::move(val));
 }
 
-template <typename T>
-std::optional<T> Observer<T>::popMostRecentlyReceivedValue(Duration max_wait_time)
+template <typename T, typename Clock>
+std::optional<T> Observer<T, Clock>::popMostRecentlyReceivedValue(Duration max_wait_time)
 {
     return buffer.popMostRecentlyAddedValue(max_wait_time);
 }
 
-template <typename T>
-std::optional<T> Observer<T>::popLeastRecentlyReceivedValue(Duration max_wait_time)
+template <typename T, typename Clock>
+std::optional<T> Observer<T, Clock>::popLeastRecentlyReceivedValue(Duration max_wait_time)
 {
     return buffer.popLeastRecentlyAddedValue(max_wait_time);
 }
 
-template <typename T>
-double Observer<T>::getDataReceivedPerSecond()
+template <typename T, typename Clock>
+double Observer<T, Clock>::getDataReceivedPerSecond()
 {
     if (receive_time_buffer.empty())
     {

src/software/multithreading/observer_test.cpp

@@ -4,7 +4,9 @@
 
 #include <thread>
 
-class TestObserver : public Observer<int>
+#include "software/test_util/fake_clock.h"
+
+class TestObserver : public Observer<int, FakeClock>
 {
    public:
     std::optional<int> getMostRecentValueFromBufferWrapper()
@@ -19,36 +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<double>(std::chrono::duration_cast<std::chrono::milliseconds>(
                                     wall_time_now - wall_time_start)
                                     .count()) *
             SECONDS_PER_MILLISECOND;
         double scaling_factor =
-            test_duration_s / (data_received_period_milliseconds *
-                               SECONDS_PER_MILLISECOND * number_of_messages);
-        double expected_actual_difference =
-            std::abs(test_observer.getDataReceivedPerSecond() -
-                     1 / (data_received_period_milliseconds * 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();

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"],
@@ -50,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",
+    ],
+)

src/software/test_util/fake_clock.cpp

@@ -0,0 +1,18 @@
+#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();
+}

src/software/test_util/fake_clock.h

@@ -0,0 +1,52 @@
+#pragma once
+
+#include <chrono>
+#include <cstdint>
+#include <ratio>
+
+/**
+ * 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).
+ */
+class FakeClock
+{
+   public:
+    using rep                          = uint64_t;
+    using period                       = std::nano;
+    using duration                     = std::chrono::duration<rep, period>;
+    using time_point                   = std::chrono::time_point<FakeClock>;
+    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_;
+};

src/software/test_util/fake_clock_test.cpp

@@ -0,0 +1,39 @@
+#include "software/test_util/fake_clock.h"
+
+#include <gtest/gtest.h>
+
+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);
+}