Code Coverage and Defensive Programming (in private functions)

Question

Assuming you want to build automated tests for the following (very simple, if odd) class.

// Calculates the distance between neighboring values in a vector
// and provides functions to return the distance from one index to the following
// or to find the first occurrence of a specified distance
class NeighborDistanceFinder
{
  public:
    NeighborDistanceFinder(std::vector<int> inputVector)
        : m_neighborVector(inputVector) { };

    int getDistanceToNextIndex(int index) const
    {
        if (index >= 0 && index < m_neighborVector.size() - 1)
            return calculateDistance(index, index + 1);
        throw "Provided index is invalid";
    }

    int getIndexOfFirstOccurenceOfDistance(int distance) const
    {
        for (int index = 0; index < m_neighborVector.size() - 1; i++)
        {
            if (calculateDistance(index, index + 1) == distance)
                return index;
        }
        throw "Distance not found";
    }

  private:
    std::vector<int> m_neighborVector;

    int calculateDistance(int index, int otherIndex) const
    {
        const int lastIndex = m_neighborVector.size() - 1;
        if (index < 0 || otherIndex < 0 || index > lastIndex  || otherIndex > lastIndex)
            throw "Index of of bounds"; //This should never happen
        return m_neighborVector.at(index) - m_neighborVector.at(otherIndex);
    }
}

In such a simple class the bounds check for calculateDistance() is probably overkill, but imagine the condition to check being more complex than a simple bounds check and more functions calling calculateDistance(), functions mucking around with m_neighborVector and soon it looks more and more appealing...

In any case, with this masterwork at hand we suddendly realize we did not do TDD and don't have any unit tests. Well, no fear, they are simple enough to add (... a few hours later ...). Done.

But there is a gap in in our code coverage: we cannot reach throw "Index of of bounds"; (unless I made a mistake in writing this example...). Well, of course, we catch the out-of-bounds cases in the two public functions invoking it, and the function is private so we cannot invoke it directly (as we lack reflection in C++). I know, static code analysis should be able to tell us that this is essentially dead code, but again: Imagine the condition being more complex.

So what should be done concerning this very-defensive private function at this point?

1. The gap is fine - you don't need (and will never get) 100% coverage - just move on.
2. This is overkill defensive programming in a private function - get rid of the check, FFS.
3. You need to cover this - move the function to protected or make your test a friend class etc.
4. And now for something completely different?

Answer 1

This depends on your acceptable levels of risk.

For most code, it will be good to keep the check but turn it into an assertion. If the check fails it indicates a deep flaw in your logic; it makes no sense to use an exception that could be caught. You can annotate the assertion statement to exclude it from your code coverage metrics. This depends on your tool, e.g. with // LCOV_EXCL_LINE comments for lcov or gcovr.

If an error would be absolutely unacceptable, you must test that the check can fail correctly. In C++, using a friend class for the test is often the least insane solution.

Another technique is to separate your code into an internal, unencapsulated, easy to test base class, and then write the public API as a thin layer on top of it. The general design might look like:

namespace detail {
  class BaseNeighborDistanceFinder
  {
  public:
    ...
  };
}

class NeighborDistanceFinder
  : private detail::BaseNeighborDistanceFinder
{
  ...
};

Or equivalently, the public API can delegate to a private detail::BaseNeighborDistanceFinder member object. The point is that while unit test are generally just another client of the public API, it can sometimes be sensible to make different parts public for the actual API and for the tests. Use your professional judgement here, as the “make everything public!” approach does have drawbacks: your tests become more brittle, and your code could become less maintainable by accidentally starting to depend on public-for-test-only parts.

Answer 2

I always encourage developers to decompose code into testable functions with perform exactly one job.

Example:

namespace detail
{
    // this is a pure function - it's 100% testable
    int calculateDistance(std::vector<int> const& neighborVector, int index, int otherIndex)
    {
        const int lastIndex = neighborVector.size() - 1;
        if (index < 0 || otherIndex < 0 || index > lastIndex  || otherIndex > lastIndex)
            throw "Index of of bounds"; //This should never happen
        return neighborVector.at(index) - neighborVector.at(otherIndex);
    }
}


// impure function implemented in terms of pure function.
// now the only doubt is "did I supply the correct arguments?"    
int NeighborDistanceFinder::calculateDistance(int index, int otherIndex) const
{
    return detail::calculateDistance(m_neighborVector, index, otherIndex);
}

Answer 3

(I realize you have an answer already nevertheless I'm going to drop this here)
If you have no intention to use a function in another class, do not expose it.
If you cannot reach a statement the statement under any circumstance it is useless, drop it.

Don't expose functions solely for improving coverage. Unit-tests are meant to test your class's public interface. Adding more public functions to test your public interface makes no sense. You test the private function by testing the public function that uses it. Getting 100% coverage is not doable and in most cases anywhere between 80-100% should be more than enough.

Imho creating a base class which has no children other than your class just to test this single statement is overkill. You're just adding more bloat-code to your code-base. All that to test a private function that isn't exposed directly to any other classes.

Answer 4

So you have a test (in your code, not a unit test) that will never fail. And you handle the case that it does fail. But it can't fail. So it never fails. At least, that's what you think. But because it doesn't fail, you can't test what happens if it fails.

What can you do? You can remove the check. Because it can't fail. That's what you think. You also think there are no bugs in your code. At least in this code. Or you can leave the check in and pray that the error handling is correct when the test fails or that it never fails. Of you hack the code so it fails maybe on every tenth call, and find out how your software handles it.