The best thing coverage can tell you is that code is unused, and should therefore be deleted.

Crossposted from https://technology.lmax.com

A brief history of coverage

There are countless gigabytes of posts talking about coverage all over the internet.

100% is the only target!

Coverage is necessary, but not sufficient!

Coverage is pointless!

Primarily, my feelings about coverage are reducible to the following example:

    public static void doThing(String input)
    {
        if (input.length() > 3) {
            doThingOne(input);
        }

        if (input.length() < 7) {
            doThingTwo(input);
        }
    }

I can cover this function with two tests and trivially achieve 100% coverage. My inputs could be, e.g, “bananarama” and “oo”. That’s not great, because there is a code path we haven’t covered: “moose” - this fires both if branches, rather than just a single one, and that’s a scenario we’ve never represented in our test code.

This brings me down solidly in the “coverage is necessary, but not sufficient” crowd - that 100% figure is a good start, but we still have to think. Boo.

Enter TDD

When using TDD we’re only supposed to write code in response to a failing test. That should basically get us that 100% coverage as we go. If we haven’t got 100% coverage, we broke the rules, and wrote code the test didn’t need.

Why run coverage at all in this case? It can’t tell you what tests you still need to write (a hard problem). We already answered this question really - it can tell us if we’ve broken the rules.

What should we do in this case? Delete the uncovered code! It doesn’t have an excuse to exist, and the tests, given they don’t use it, will all still pass.

A more archaeological example

Yes, fine, you say, this is a fun technique to introduce to the TDD cycle - perhaps, on green, this can show me some code I don’t need to refactor, but I can just be rid of altogether. Indeed - but this technique can also help shine a light on existing code.

Here’s a toyified example of something I came across recently where I was able to use this technique to convince myself that some code was dead.

In this example, we’re aggregating up some mysterious ‘state’ by instrument id and then instruction id. At some point, we want to remove any state attached to a given account id.

    private final Map<Long, Map<InstructionId, AggregatedState>> instrumentIdMap = new HashMap<>();
    // implementation of InstructionId, AggregatedState elided

    public void removeState(final long accountId)
    {
        final List<AggregatedState> completedStates = new ArrayList<>();
        for (final Iterator<Map<InstructionId, AggregatedState>> iterator = instrumentIdMap.values().iterator(); iterator.hasNext(); )
        {
            final Map<InstructionId, AggregatedState> map = iterator.next();

            for (final AggregatedState aggregate : map.values())
            {
                aggregate.removeStateForAccount(accountId);
                if (aggregate.isComplete())
                {
                    completedStates.add(aggregate);
                }
            }

            if (map.isEmpty())
            {
                iterator.remove();
            }
        }
        completedStates.forEach(this::removeAggregate);
    }

    private void removeAggregate(AggregatedState aggregatedState)
    {
        final Map<InstructionId, AggregatedState> map = instrumentIdMap.get(aggregatedState.getInstrumentId());
        map.remove(aggregatedState.getOriginalInstructionId());
        if (map.isEmpty())
        {
            instrumentIdMap.remove(aggregatedState.getInstrumentId());
        }
    }

Now - that’s quite a lot of code. I needed to rewrite it, because, in the non-toyified example, the resulting events it generated were not emitted in a deterministic order. Before changing it, though, I wanted to understand it, and, additionally, check the test coverage was sufficient to give me confidence in my change.

In particular, one area stuck out for me as being tricky to test - the removal of entries for the various levels of map. This object doesn’t permit access to its internals, and I was loath to open it up just in order to test it. Instead, I added an assertion to the end of the method:

        assert instrumentIdMap.values().stream().noneMatch(Map::isEmpty);

I then went to look at the tests, found a couple that I thought were missing, and added them in. They duly passed. Well - that’s no good - I want to see them fail, so I know they do something.

//            if (map.isEmpty())
//            {
//                iterator.remove();
//            }

That should break things…but it doesn’t. If you’re reading along at home and fancy a challenge, can you see why? Take a moment to review the code to see if you can spot it.

Inspection didn’t do it for me (I wonder: would chatgpt?), so eventually I came around to the idea of uncommenting those lines and running the test with coverage, to see if any test ever triggered them. Turns out not! But that assertion still didn’t fail. Why not? Well, because removeAggregate is the place doing the actual tidyup, and if we look carefully at the inner loop, we can see we never remove anything from map, so it can never be empty. Those three lines, it turned out, had been vestigial for several years, and the whole need for using iterator rather than enhanced for loops was totally bogus.

Or, shorter: coverage would have told us the code was dead, if we’d only have thought to run it.

Conclusion

Coverage - it can’t give you confidence in what you have. But it can help you identify what to take away.

• Are you TDDing the hell out of something? Good on you. Run coverage to see if you’ve broken the rules.
• Do you have code that’s well tested, but hard to fathom? Try running the tests with coverage for some ideas of what you could delete.

How do you know if code is well tested? Well, that’s the real toughie. As we’ve seen here (for even the most trivial code, regrettably), you’re going to have to think; coverage might be a part of that process, but it certainly isn’t the end.