What approach do I take to unit testing a class which has a method that internally calls other methods?

Question

I have a class which has one method that is called from another class. This method internally calls several other methods to do its work. Those other methods are all public and can be called by the other class but I don't use them that way because I'm trying to follow the "Tell-Don't-Ask" principle. All these methods return something back and they don't mutate the state of the input parameters. They do all their processing logic using only the input parameters passed in.


Now when it comes to unit testing, I can do these things but I'm not sure which one to do


1. Test all methods by passing in input and asserting the actual output against an expected one.
2. Test just the internal "public" methods (instead of all methods) like I described above and also test that the method that is called by the other class invokes the internal methods with the correct parameters i.e. use a mocking framework to mock out those calls but still assert that the calls are made.

This is an example. The enhanceThingInformation method is the one that is called by the other class. The other methods are pretty specific to this class so I don't thing breaking them out to their own class is the right approach here (though it might be?).

class ThingInformationEnhancer {
    addErrorInformation(thing, someDictionary1, someDictionary2) {
        thing = /* some logic here */
        /* more logic here */
        return thing;
    }

    removeInvalidInformation(thing, someDictionary2) {
        thing = /* some logic here */
        /* more logic here */
        return thing;
    }

    addAdditionalInformation(thing, someDictionary1, someDictionary2) {
        thing = /* some logic here */
        /* more logic here */
        return thing
    }

    enhanceThingInformation(thing, someDictionary1, someDictionary2) {
        thing = this.addErrorInformation(thing, someDictionary1, someDictionary2);
        thing = this.removeInvalidInformation(thing, someDictionary2);
        thing = this.addAdditionalInformation(thing, someDictionary1, someDictionary2);
        return thing;
    }
}

I'm interested to hear how others handle this sort of situation.

Answer 1

In short, if you have a method that handles logic (i.e not just calling databases or other 3rd party libraries), you should test it.

For the longer version, there are 2 different things in there I believe:

First: The right amount of unit testing

You should strive to unit test all code that has logic in it, that's your point number 1 in the question there, pass the input and assert the expected outputs. This is going to run fast and make sure your code works as intended.

If a function just calls a database or a 3rd party library, you should skip those or just assert they are calling the 3rd party libraries they need to with the right parameters.

Second: Testing from the outer class (the consumer of this public method)

You said you only got one method that is supposed to be called from the outside and are unsure how to test it. You were right in the principle of just testing that you called that function and assert on the result.

The reason for that is that the consumer (or customer) of that class don't really care about the order of functions are called and with each parameter, just that it takes the right input and returns the right output. That is sometimes called the contract of a function and it is what you need to test there. This would be usually called an integration test.

Answer 2

Given that addErrorInformation, removeInvalidInformation, addAdditionalInformation are "side effect free"...

I would expect that either (a) those functions will have unit tests OR (b) some sort of tag/annotation indicating that the function is too simple to bother testing.

enhanceTheThing, as currently shown here, is necessarily side effect free. It's also simple enough that it might not need a test of its own (which is to say, given that you have the other three methods well tested, adding tests here mitigates very little current or future risk).

But given that we choose to test it - do we need to use Mocks? "It depends" -- the code is side effect free, so we don't have to worry about tests interfering with each other via shared mutable state. But the observable behavior of enhanceTheThing depends on the observable behavior of the other three methods.

Thus, an important question: are those other three behaviors stable? If the observed behavior of removeInvalidInformation were to change, then so too might change the observed behavior of enhanceTheThing. If the observed behavior of removeInvalidInformation changes frequently, then you are going to be wasting a lot of time fixing the enhanceTheThing tests to re-align the expectations with the new correct behaviors. In other words, you may end up investing a lot of time in "fixing the tests" even though the implementation of enhanceTheThing is unchanged.

One possibility is to add a counter to the test, to track how many times you have had to modify the test to eliminate a false positive. When that number becomes embarrassingly large, you either (a) remove the test or (b) replace the implementation with one that uses a mock.

If you already know that the counter is going to be embarrassing, then go ahead and skip to the test implementation with mocks.

One risk that mocks add is "drift"; that the thing being mocked may eventually become so different from how things work today that the test loses its effectiveness as a measure of the correctness of the system. JBrains talks about how to manage some of that risk in his discussion of Contract Tests.