The binary coverage hypothesis

Everyone loves to hate code coverage. But I have started to notice one interesting consequence of using coverage metrics that I refer to as the binary coverage hypothesis:

A class should either have full unit test coverage or none at all.

Like other similar maxims (e.g. the zero-one-infinity rule), it may not always apply but seems to have general usefulness in hunting for design smells. It is probably best illustrated with a concrete example.

Imagine you have a relatively expensive API that might be called frequently inside your system. Ideally you want the call to be skipped sometimes but without burdening the call site with the logic to do the throttling. This problem might be solved by the following class which prevents an action from being invoked more than once in a given time interval — sort of a conditional null object pattern:

class ThrottledAction
{
    private readonly Action action;
    private readonly TimeSpan interval;

    private DateTime lastInvoked;

    public ThrottledAction(Action action, TimeSpan interval)
    {
        this.action = action;
        this.interval = interval;
    }

    public void Invoke()
    {
        DateTime now = DateTime.UtcNow;
        if ((now - lastInvoked) >= interval)
        {
            this.lastInvoked = now;
            this.action();
        }
    }
}

This formulation would be the “legacy code” example (using the Michael Feathers definition of legacy meaning “without tests”) — because it is very difficult to properly unit test a class that has a system clock dependency. But clearly there is logic here that deserves to be tested, given the potential boundary conditions and edge cases that could arise even in a small area like this.

Compelled by your TDD instincts, perhaps you would rework it like so and create Throttled Action 2: Test-Friendly Edition, while still ensuring compatibility with the original version:

class ThrottledAction
{
    private readonly Action action;
    private readonly TimeSpan interval;
    private readonly Func<DateTime> getTime;

    private DateTime lastInvoked;

    public ThrottledAction(Action action, TimeSpan interval, Func<DateTime> getTime)
    {
        this.action = action;
        this.interval = interval;
        this.getTime = getTime;
    }

    public ThrottledAction(Action action, TimeSpan interval)
        : this(action, interval, () => DateTime.UtcNow)
    {
    }

    public void Invoke()
    {
        DateTime now = this.getTime();
        if ((now - lastInvoked) >= interval)
        {
            this.lastInvoked = now;
            this.action();
        }
    }
}

Now you can write a full suite of unit tests such as these:

[TestMethod]
public void ShouldInvokeActionOnlyOnceWithinInterval()
{
    DateTime now = new DateTime(2001, 2, 3, 4, 5, 6);
    Func<DateTime> getTime = () => now;
    int count = 0;    
    Action inner = () => ++count;
    TimeSpan interval = TimeSpan.FromSeconds(5.0d);
    ThrottledAction action = new ThrottledAction(inner, interval, getTime);

    action.Invoke();
    now += TimeSpan.FromMilliseconds(1);
    action.Invoke();

    count.Should().Be(1);

    now += TimeSpan.FromSeconds(10.0d);
    action.Invoke();
    now += TimeSpan.FromMilliseconds(1);
    action.Invoke();

    count.Should().Be(2);
}

You could have as many tests as you want covering various timing situations. With the injectable clock function, the testing world is your oyster. Having gone this route, there is however one road you will most likely not take — using the alternate constructor that passes the real DateTime.UtcNow function. Because of this aversion to test-unfriendliness (and rightly so), you will now see an unsightly red bar in your code coverage graph, with that last 5% or so of coverage not achieved.

According to the binary coverage hypothesis, this “neither 0% nor 100%” half measure means we should rethink the approach. While it is possible to write a “unit test” that simply calls this alternate constructor using the real system clock and returns, that is merely a letter-but-not-the-spirit cop-out. What we are really seeing here is a sign that this constructor does not belong in this class anymore and should be deleted. For one, it unnecessarily couples the code to an implementation detail (albeit minor in this particular case). And, assuming all “legacy” call sites are eventually fixed up, it would likely be an error to allow instantiation of this class without providing the corresponding clock function within the context.

The binary coverage hypothesis is subtle and does not always lead to a “one true path” solution. Nevertheless, it is a useful guidepost to promote critical thinking when evaluating a system design.