One of the key metrics I’m looking to achieve with Edible is 100% code coverage. I won’t get into the more general debate about whether this is a good metric or not. There are an incredible amount of articles in that arena already. What I want to do is share an anecdote and maybe draw a conclusion or two.
The bisect operation in Edible works on sets of elements that have a total ordering amongst them. Because the types of elements you can feed into the algorithm are dynamic, it’s not guaranteed that they will have total ordering if their comparison functions are incorrect. Here’s the stretch of code I’m talking about:
if lte(value_at_left_index, value) and lte(value, value_at_current_index) then return 0 elseif gt(value_at_current_index, value) then return -1 elseif lt(value_at_current_index, value) then return 1 end
You will notice there’s no else condition here. That’s dangerous because in the case where none of the conditions are true (if the ordering functions are implemented wrong for instance), this will fail silently and cause some kind of issues downstream that may be difficult to trace back. To that end, we might want to put a line in like
-- Conditions copied from above if lte(value_at_left_index, value) and lte(value, value_at_current_index) then return 0 elseif gt(value_at_current_index, value) then return -1 elseif lt(value_at_current_index, value) then return 1 -- Handle the "impossible" case else assert(false, "Programmer error, this condition occurs when there is not " .. "a total ordering of all elements with the searched array. " .. "It indicates a deficiency in the ordering algorithms.") end
Here the else statement should never run. As the commentary explains, the assertion only trips if there is a bug in the ordering functions. The issue here is that if you have the else statement, it will never run. You’ll be left with 99.999% code coverage forever because there’s no way to test this “impossible” case.
So what can we do
Well there are a couple of options. One possibility is to refactor this to allow mocking of the ordering functions so that you can introduce a bug and trigger the assertion thus getting back to 100% code coverage. This sacrifices readability of both the code and that test case since you’ll be bending over backwards for the test.
Another option is to remove the assertion and the else block entirely. As explained above, this is dangerous if the ordering functions are changing since you can get weird failure cases that are hard to diagnose.
The last option is to sacrifice 100% code coverage. Besides being aesthetically displeasing, there’s a practical reason to avoid this option. Having exactly 100% coverage is a fast way to know that every statement has at least been executed once. It’s no guarantee that every possible combination of cases has been tested, but it provides confidence that most cases have been considered, including all branches.
None of these options are good, but in the end I elected to remove the else statement. My reason was to avoid sacrificing code readability or coverage since those metrics are permanent. This code is only risky if the ordering functions change. Since they are stable, I can paste the else statement back in to handle the impossible case before starting to work on those functions. Then I can just remove it before committing when I know the ordering functions are stable again. While this solution works in my specific situations, I don’t think it’s generally applicable. Whether or not you agree that 100% code coverage is desirable, it’s certainly not attainable in all cases without sacrifices.