0

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(firstIndex) - m_neighborVector.at(secondIndex);
    }
}

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?
  • 1
    Note that vector::at already throws std::out_of_range, so I would say both "Provided index is invalid" and "Index of of bounds" are superfluous – Caleth Jun 27 '18 at 12:44
  • Good point. Still, this is only a simple example I stitched together on the fly. Feel free mentally subsititute a random vector type that does no bounds checking... – CharonX Jun 28 '18 at 7:42
3

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.

0

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);
}
  • Is NeighborDistanceFinder::calculateDistance() still a private function? If so, how can I test it or detail::calculateDistance(). I do not have access to reflection in C++. Or are you suggesting to make NeighborDistanceFinder::calculateDistance() public? – CharonX Jul 2 '18 at 8:06
  • @CharonX detail::calculateDistance would be a public pure function, so it's easily testable. This means your private function is almost 100% covered, apart from argument assignment. – Richard Hodges Jul 2 '18 at 8:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.