0

Assume I have the following class:

class WeirdCalculator
{
  public:
    WeirdCalculator() { };

    int addWithOffset(int a, int b)
    {
        OffsetProvider provider;
        if (provider.applyOffset())
            return a + b + provider.getOffset();
        else
            return a + b;
    };
}

Further assume that OffsetProvider::applyOffset() and OffsetProvider::getOffset() not necessary always return the same values (applyOffset() is false on leap-years, and getOffset() normally returns 42 but yields a different value on Monday 13th).

Putting this class under unit-test (so you can refactor it), you will quickly notice that it is hard to cover the else path (but you can do it with some trickery). Looking closer you also notice that the result of addWithOffset() is dependent on the result of getOffset().

Should your test coverage include those possible divergence points (i.e. also simulate a test in leap years and on Monday 13th)? The impending refactorings might render those tests soon obsolete, but if you do not cover them you might not spot that you accidently hardcoded that 42 instead of using the value from getOffset().

CharonX
  • 2,130
  • 11
  • 33
  • Singletons have always been a pain when it comes to tests. The subject is (too?) broad, and well treated on the web... [and on SO](https://stackoverflow.com/a/2085988/5470596). – YSC Aug 22 '18 at 11:50
  • @YSC I saw those answers before I asked my question, and I don't think the question on **WHAT** to cover before refactoring has been answered in the duplicate - but since you and Martin Bonner already closed the question I won't bother SO with that anymore. – CharonX Aug 22 '18 at 12:14
  • @CharonX Feel free to edit your question and explain why it isn't a duplicate. If you ping me, I can reopen it. (Note that YSC actually voted to close as "too broad" - so please try to narrow the question down too.) – Martin Bonner supports Monica Aug 22 '18 at 12:57
  • @MartinBonner I've gutted the question and edited into something that still asks what I want to know (but asks in a different way) – CharonX Aug 22 '18 at 13:44
  • @CharonX - That's *much* better. The simplification is worthwhile, and not referring to singletons helps. I still wonder if it is going to get closed as "too broad" or "primarily opinion based" (and you might want to ask on the QA site too). – Martin Bonner supports Monica Aug 22 '18 at 13:49
  • The corner cases are the *most* important to test since those are the easiest to get wrong when initially implementing the class and also the easiest to break when refactoring. – Jesper Juhl Aug 22 '18 at 13:51
  • @JesperJuhl I feel that too, but in this specific case the corner case is further down the line - i.e. `OffsetProvider` **must** verify that the results of `applyOffset()` and `getOffset()` are correct in all cases (leap year or not, Monday 13th or not). The thing I am struggling with is if/when to rely in the result of a called function (if it does not create a new execution path). – CharonX Aug 22 '18 at 14:06
  • How much should you cover, you ask? as much as possible. And if that is easily done it is a no-brainer. Just do it. Write all test you can and then make the refactoring. I can recommend the book "working with legacy code". If your question is more philosophical then the answer all boils down to opinions... – Jocke Aug 22 '18 at 17:21

1 Answers1

0

Looking through the comments and sleeping over the issue I came to the following conclusion (at least for me):

If a function call has direct impact on your function under test (e.g. provides a return value), then you need to cover it. For example:

int indirectIsOdd(int a)
{
    //Please ignore the fact that I could just write "return Calculator::isOdd(a)" here
    if (Calculator::isOdd(a))
        return true;
    else
        return false;
}

It is obvious that you need to cover both possible results of Calculator::isOdd() - otherwise you don't cover your entire code.

int indirectAdd(int a, int b)
{
    int result = Calculator::add(a, b);
    return result;
}

You may want to plug in a few example values for a and b here, but - to be honest - what you REALLY want to do is mock Calculator::add() so you can verify that indirectAdd() both calls Calculator::add() and uses the value Calculator::add() returns.

CharonX
  • 2,130
  • 11
  • 33