3
\$\begingroup\$

I'm using the time for e.g. ping and timeout calculation as well as animations in a game. So my (big) codebase uses std::chrono::steady_clock::now() in many places. For testing I'd like to test those without having to resort to sleep and friends (timeout of >=5mins will be hard to test this way...)

For this I created a custom Clock class that uses a singleton-like instance which can be changed to provide a custom clock. A default of steady_clock is used for production code. This Clock can be used as a drop-in replacement of std::chrono::steady_clock and can be mocked in test code.

I'd like a review with suggestions how to improve this or if it has any flaws. Pointers to similar/better implementations are also welcome. I'm mostly concerned about the multiple indirections (and related performance penalties) this causes: Lookup of a "constructed" flag, the impl and the vtable (3 pointers) although I only have 1 function. See godbold

#include <chrono>
#include <memory>

struct BaseClock
{
    using Clock = std::chrono::steady_clock;
    using time_point = Clock::time_point;
    virtual ~BaseClock() = default;
    virtual time_point now(){ return std::chrono::steady_clock::now(); }
};

class Clock
{
    static std::unique_ptr<BaseClock>& inst(){
        static std::unique_ptr<BaseClock> clock(new BaseClock);
        return clock;
    }
public:
    using rep = BaseClock::Clock::rep;
    using duration = BaseClock::Clock::duration;
    using time_point = std::chrono::time_point<Clock>;

    static time_point now(){ return time_point(inst()->now().time_since_epoch()); }
    static void set(std::unique_ptr<BaseClock> clock){inst() = std::move(clock);}
};

int main()
{
    return Clock::now().time_since_epoch().count();
}

Example usage in test code:

struct MockClock: public BaseClock{
    static time_point current;
    time_point now() override { return current; }
};

void test_method(){
    Clock::set(new MockClock);
    MockClock::current = MockClock::time_point(100);
    testClass.methodUsingClock();
    MockClock::current += std::chrono::seconds(10);
    REQUIRE(testClass.checkTimeout());
}
\$\endgroup\$
  • \$\begingroup\$ What's the point of BaseClock ? What's advantage against using directly std::chrono ? Avoid naked new, use std::make_unique instead. \$\endgroup\$ – Calak Nov 23 '18 at 10:06
  • 1
    \$\begingroup\$ @Calak: IIUC the point of BaseClock is to provide a baseline clock interface without direct dependencies. A user expecting a const BaseClock& or similiar can now write code independent of the underlying clock type. This allows for switching the underlying clock on a higher level without affecting the dependent code (kind of "dependency injection for clocks"). As a side effect, this allows for easier test setups (especially for weird corner cases). // I'm not a fan of the singleton in Clock, though. \$\endgroup\$ – hoffmale Nov 23 '18 at 10:54
  • 1
    \$\begingroup\$ @Calak In test code you need to mock the time, that's the whole purpose that I stated in the intro. Hence you need some injection point which the virtual BaseClock::now() provides while defaulting to std::chrono. I added a usage example to make this clearer. \$\endgroup\$ – Flamefire Nov 23 '18 at 12:20
  • \$\begingroup\$ @hoffmale You are correct, although the code does not get BaseClock& but simply uses Clock as a drop-in replacement for std::chrono::*clock (I edited the question to highlight this) So yes this is exactly dependency injection but without passing it as (template or regular) parameters which would require massive changes to the codebase. As clocks are static classes a singleton is required, I don't see a way avoiding this. \$\endgroup\$ – Flamefire Nov 23 '18 at 12:22
5
\$\begingroup\$

While I think the underlying idea is quite nice, the actual implementation and design has some issues.

Wrong Abstraction Level

Let's start with the less obvious one: How would you actually use Clock or BaseClock with std::chrono::high_resolution_clock or std::chrono::system_clock?

The simplest approach would be something akin to this:

struct HighResClock : BaseClock {
    time_point now() override { return std::chrono::high_resolution_clock::now(); }
};

It seems so clean, so simple, and so easy. Except that it doesn't compile! That's because std::chrono::high_resolution_clock::time_point is not the same as BaseClock::time_point (and cannot easily be converted, if at all possible).

But: Do we actually need time_points in the public interface?

The only reason to expose time_point values is to extract time differences between them. But that only matters if arbitrary time_points are to be compared.

Technically, the time_point could be stored in some form. However, for many clocks, like std::chrono::steady_clock or std::chrono::high_resolution_clock, the epoch from when the clock are measuring their time offset can change between different executions of the same program (e.g. because the computer got rebooted).

This makes storing time_points, especially those not obtained from std::chrono::system_clock, rather useless. In that case, you'll likely need a calendar library (or similar) to get points of time in a storable format.

But in most cases, a simple Timer abstraction can fulfill all clock needs (comparing some time_points with some relation). A simple Timer interface could look like this:

struct timer {
    // choose an appropriate duration type
    using duration = std::chrono::duration<double>;

    virtual ~timer() = default;

    virtual void start() = 0;
    virtual duration stop() = 0;
    virtual duration tick() = 0; // to obtain multiple measurements from the same baseline
};

class steady_timer : public timer {
    using clock = std::chrono::steady_clock;
    using time_point = clock::time_point;

    time_point start_time;
    bool running;

public:
    steady_timer() = default;

    void start() override
    {
        start_time = clock::now();
        running = true;
    }

    duration tick() override
    {
        return duration(clock::now() - start_time);
    }

    duration stop() override
    {
        auto elapsed = tick();
        running = false;
        return elapsed;
    }
};

Now the only exposed part of the interface is the duration. And it is easily extensible to other time sources (e.g. Win32 QueryPerformanceCounter) or mockable.

Singleton

I really don't like the Clock singleton. Yes, it is easy to just ask a global clock. Yes, it is also easy to screw all code depending on this clock by changing the underlying instance.

For example, a test setting Clock to a mock but not restoring the original clock breaks all other tests that assume the default clock implementation - making test failure dependent on test execution order.

Instead, take a reference or pointer to a timer as parameter. This allows you to pass in a clock where needed, without changing (or corrupting) everyone elses timer.

Rewriting your test case:

class mock_timer : public timer {
    std::vector<duration> measurements;
    int counter = 0;

public:
    mock_timer(std::initializer_list<duration> il) : measurements(il) {}

    void start() override {}

    duration tick() override
    {
        if(counter < measurements.size()) return measurements[counter++];
        return measurements.back();
    }

    duration stop() override
    {
        return measurements.back(); // just example
    }
};

void test_method(){
    using namespace std::literals::chrono_literals;
    mock_timer my_timer{ 10s };

    testClass.methodUsingClock(my_timer);

    // or, more likely:
    testclass inst{my_timer};
    inst.methodUsingClock();

    REQUIRE(testClass.checkTimeout());
}
\$\endgroup\$
  • \$\begingroup\$ I understand your arguments But I see problems with that: Time is something global, so it makes sense for it being a singleton/global. Passing it to every class would require huge refactoring and passing things down in aggregates etc. which shows another problem: Your timer class cannot be reused! So e.g. a class that needs 2 timers cannot reuse the timer as it would restart. BUT: What if I use my design but change BaseClass::now to return a duration? Then the underlying clock does not matter as long is it returns "time since epoch" and is not changed while a Clock::time_point is held. \$\endgroup\$ – Flamefire Nov 23 '18 at 14:58
  • 1
    \$\begingroup\$ @Flamefire: I'm not sure if I am getting your whole argument. I don't think time is something global. Instead, everything has its own perspective on time. For example, a game might be using one clock for rendering and a different clock for logic updates. Why? Because if gameplay is paused (e.g. when entering a menu), the graphics should still continue to be rendered because otherwise the screen would freeze. // RE "time since epoch": When is epoch? What if the epoch changes because the clock is changed? Having mutable global state is just waiting for accidents to happen. \$\endgroup\$ – hoffmale Nov 23 '18 at 16:52
  • \$\begingroup\$ Even your timer needs a clock. And all instances of that (at least in the same logical unit) need to use the same clock. Otherwise the relative time won't match (or at least it is not ensured) For your example: Yes if paused the timer needs to be paused too, but the time does not. So you need different timers, but I don't see the need for different clocks. // "mutable global state": I actually don't want nor need it to be mutable. It should be set once: Start of program or start of test. Then the epoch stays the same ->hence my "is not changed" restriction, but I'm unsure how to enforce it \$\endgroup\$ – Flamefire Nov 24 '18 at 16:44
1
\$\begingroup\$

The use of a singleton will limit your possibilities, as hoffmale's review points out. It completely prevents concurrent testing. However, you'll find that getting the Clock instance to the code that needs it can easily add lots of "tramp data" to intermediated method signatures. I try to limit that by passing a factory/registry interface that allows classes to access the system interactions they need - time, filesystem, network, etc. In the tests, populate a mock-factory with the necessary mock objects, and in production, pass a concrete-factory that gives out objects with real system access.

One aspect that's missing is that this interface only gives access to now() - it doesn't handle the other clock-related actions that can cause slow tests. In particular, you'll want sleeps and network- or mutux-related timeouts to respect the mock clock's idea of time. To achieve that, you'll need to redirect those timeouts to mockable methods. That's a bigger job, but will give you a much more useful test regime.

I think that when I made something like that (many years ago, and in a different language), I ended up with the MockTime storing an ordered list of future events; whenever control entered its code, it could advance the time to match the next event (which could be the end of a sleep, release of a mutex, or an interrupt from an external (mocked) thread, for example).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.