100% Unit Test Code Coverage is Not Sufficient
Even if you have 100% unit test code coverage, there can still be bugs.
I think stated like that, it's sort of self-evident. But at a subconscious level, software engineers tend to compartmentalize this belief into an isolated section of their mind: they generally associate the parts of the code that are covered by tests with being bug-free, and the parts that are uncovered have bugs lurking.
From this association—and combined with the desire to have bug-free code—some engineers then create a goal of having 100% unit test code coverage. This is a mistake. Having 100% code coverage is neither necessary nor sufficient to ensure a bug-free codebase. The "not necessary" part is obvious, so today, we'll cover the "not sufficient" part.
Here is a real-life example from actual production code, though slightly edited for clarity. We want to emit metrics when a particular event happens, but we want to dedupe the metrics so that they're only emitted once per hour. This class is responsible for ensuring the metric is only emitted at most once per hour. It depends on some sort of storage of timestamps, a clock, and the actual code that knows how to emit the metric. We applied dependency injection on all of those dependencies so that we could easily test the class.
const val ONE_HOUR_IN_MILLISECONDS: Long = 60 * 60 * 1000
class DedupingMetricRecorder(
private val delegate: MetricRecorder,
private val store: TimestampStore.
private val clock: Clock
) {
fun recordOnceHourly(metric: Metric) {
val now = Clock.now()
val lastRecord = store.getTimestamp(metric.name)
if (now - lastRecord > ONE_HOUR_IN_MILLISECONDS) {
delegate.record(metric)
} else {
Log.debug("Metric ${metric.name} already recorded in last hour; ignoring")
}
}
}
Next, we have the tests. There are three basic scenarios:
We've never invoked the metric before, so the timestamp store will return its default value of 0, and we expect the metric to be emitted.
We've invoked the metric before, but it's been over 1 hour, so we should emit the metric again.
We've invoked the metric before, but it's been less than 1 hour, so we don't emit the metric again.
The three tests have the same basic structure, so we create a helper method that extracts out that structure, and then the tests just invoke the helper method with different arguments. For now, let’s put aside whether or not you think having helper methods in tests is a good idea.
private fun runTestCase(
whenWasMetricSet: Long,
whatTimeIsItNow: Long,
shouldDelegateGetCalled: Boolean) {
val metric = arbitraryMetric()
// Arrange
val mockDelegate = mockk<MetricRecorder>()
val mockStore = mockk<TimestampStore>()
val fakeClock = SettableClockImpl()
every {
mockStore.getLong(metric.name)
} returns whenWasMetricSet
fakeClock.setNow(whatTimeIsItNow)
// Act
val underTest = DedupingMetricRecorder(
mockDelegate, mockStore, fakeClock
)
underTest.recordOnceHourly(metric)
// Assert
verify(exactly = if (shouldDelegateGetCalled) 1 else 0) {
mockDelegate.record(metric)
}
}
private val NOW = Instant.now()
private val OVER_ONE_HOUR_AGO =
NOW.minusMillis(ONE_HOUR_IN_MILLISECONDS + 1)
private val ABOUT_THIRTY_MINUTES_AGO =
NOW.minusMillis(ONE_HOUR_IN_MILLISECONDS / 2)
@Test
fun `recordOnceHourly, if the metric has never been recorded, records the metric`() {
runTestCase(
whenWasMetricSet = 0,
whatTimeIsItNow = NOW.toEpochMilli(),
shouldDelegateGetCalled = true
)
}
@Test
fun `recordOnceHourly, if the metric was set over an hour ago, records the metric`() {
runTestCase(
whenWasMetricSet = OVER_ONE_HOUR_AGO.toEpochMilli(),
whatTimeIsItNow = NOW.toEpochMilli(),
shouldDelegateGetCalled = true
)
}
@Test
fun `recordOnceHourly, if the metric was set less than an hour ago, does not record the metric`() {
runTestCase(
whenWasMetricSet = ABOUT_THIRTY_MINUTES_AGO.toEpochMilli(),
whatTimeIsItNow = NOW.toEpochMilli(),
shouldDelegateGetCalled = true
)
}
So now we run all the tests. They all pass, and we have 100% unit test code coverage. Great.
Maybe you've spotted the bug, and maybe you haven't. That's beside the point. For what it's worth, this code was reviewed by two other engineers, and they did not catch the bug. To be fair to them, they need to review half a dozen Code Reviews (CRs) every day, and (as far as we know) they tend to spot bugs, when they are present, in most CRs.
The point is that we have 100% code coverage here, both line coverage and branch coverage, and yet a bug managed to slip through anyway.
The bug is that we never actually update the store with the new timestamp.
Here's the fixed version:
const val ONE_HOUR_IN_MILLISECONDS: Long = 60 * 60 * 1000
class DedupingMetricRecorder(
private val delegate: MetricRecorder,
private val store: TimestampStore.
private val clock: Clock
) {
fun recordOnceHourly(metric: Metric) {
val now = Clock.now()
val lastRecord = store.getTimestamp(metric.name)
if (now - lastRecord > ONE_HOUR_IN_MILLISECONDS) {
delegate.record(metric)
store.setTimestamp(metric.name, now) //THIS IS THE FIX
} else {
Log.debug("Metric ${metric.name} already recorded in last hour; ignoring")
}
}
}
And for good measure, here's the test we added to prevent future regression:
@Test
fun `recordOnceHourly, if called twice in quick succession, only records the metric once`() {
val metric = arbitraryMetric()
// Arrange
val mockDelegate = mockk<MetricRecorder>()
// Act
val underTest = DedudpingMetricRecorder(
mockDelegate, InMemoryTimestampStore, SystemClock
)
underTest.recordOnceHourly(metric)
underTest.recordOnceHourly(metric)
// Assert
verify (exactly = 1) {
mockDelegate.record(metric)
}
}