Clean Unit Testing

It's easy to write "unit test" tests that use JUnit and some mocking library. They may produce code coverage, that keep some stakeholders happy, even though the tests aren't even unit tests and provide questionable value. It can also be very easy to write unit tests that are — in theory — units test but are more complex than the underlying code and hence just add to the total software entropy.

This particular type of software entropy has the unpleasant characteristic of making it even harder for the underlying software to be restructured or to surface new requirements. It is like the test has a negative value.

Doing unit testing properly is a lot harder than people think. In this article, I outline several tips that aim to improve the readability, maintainability and the quality of your unit tests.

Note: for the code snippets, Spock is used. For those who don't know Spock, consider it a very powerful DSL around JUnit which adds some nice features and cuts down on verbosity.

Reason for Failure 

The Unit Test should only fail if there is a problem with the code under test. A unit test for the class DBService should only fails if there is a bug with DBService not if there is a bug with any other class it depends on. Thus, in the unit test for DBService, the only instantiated object should be DBService. Every other object that DBService depends on should be stubbed or mocked.
Otherwise, you are testing code beyond DBService. While you might incorrectly think this is more bang for the buck, it means locating the root cause of problems will take longer. If the test fails, it could be because there is a problem with multiple classes but you don't know which one. Whereas, if it can only fail because the code under test is wrong, then you know exactly where the problem is.
Furthermore, thinking this way will improve the Object Orientated nature of your code. The tests will only test the responsibilities of the Class. If it's responsibilities aren't clear, or it can't do anything without another class, or the class is so trivial the test is pointless, it prompts the question that there something wrong with the class in terms of its responsibilities.
The only exception to not mocking or stubbing a dependent class is if you are using a well-known class from the Java library e.g. String. There is not much point stubbing or mocking that. Or, the dependent class is just a simple immutable POJO where there is not much value to stubbing or mocking it.

Stubbing and Mocking 

The terms mocking and stubbing can often be used interchangeably as if there were the same thing. They are not the same thing. In summary, if your code under test has a dependency on an object for which it never invokes a method on that object which has side effects, that object should be stubbed.
Whereas, if it has a dependency on an object for which it does invoke methods that have side effects then that should be mocked. Why is this important? Because your test should be checking for different things depending on the types of relationships it has with its dependencies.
Let's say your object under test is BusinessDelegate. BusinessDelegate receives requests to edit BusinessEntities. It performs some simple business logic and then invokes methods on a DBFacade (a facade class in front of a Database). So, the code under test looks like this:

public class BusinessDelegate {
     private DBFacade dbFacade;
     // ...

     public void edit(BusinessEntity businessEntity) {
         // Read some attributes on the business entity
         String newValue = businessEntity.getValue();
      
         // Some Business Logic, Data Mapping, and / or Validation

         //...  

         dbFacade.update(index, data)
    }
}

Regarding the BusinessDelegate class, we can see two relationships.

1. A read-only relationship with BusinessEntity. The BusinessDelegate calls a few getters() on it and never changes its state or never invokes any methods that have side effects. 
2. A relationship with DBFacade where it asks DBFacade to do something that we assume will have side effects. It is not the responsibility of BusinessDelegate to ensure the update happens, that is DBFacade's job. The responsibility of BusinessDelegate is to ensure the update method is invoked with the correct parameters — only.

So clearly, regarding the unit test for BusinessDelegate, BusinessEntity should be stubbed and DbFacade should be mocked. If we were using the Spock testing framework we could see this very clearly

class BusinessDelegateSpec {
    @Subject
    BusinessDelegate businessDelegate
    def dbFacade

    def setup() {
        dbFacade = Mock(DbFacade)
        businessDelegate =  new BusinessDelegate(dbFacade);
    }

    def "edit(BusinessEntity businessEntity)"() {
        given:
           def businessEntity = Stub(BusinessEntity)
           // ...
        when:
            businessDelegate.edit(businessEntity)
        then:
            1 * dbFacade.update(data)
    }
}

Having a good understanding of stub mock differentiation improves OO quality dramatically. Instead of just thinking about what the object does, the relationships and dependencies between them get much more focus. It is now possible for unit tests to help enforce design principles that would otherwise just get lost.

Stub and Mock in the Right Place 

The curious among you, might be wondering why in the above code sample, dbFacade declared at class level, while businessEntity was declared at method level? Well, the answer is, unit test code is much more readable the more it can mirror the code under test. In the actual BusinessDelegate class, the dependency on dbFacade is at the class level and the dependency on BusinessEntity at the method level.
In the real world when a BusinessDelegate is instantiated a DbFacade dependency will exist, anytime BusinessDelegate is instantiated for a unit test it is ok to have the DbFacade dependency also existing.
Sound reasonable? Hope so. There are two further advantages of doing this:

• A reduction in code verbosity. Even using Spock, unit tests can become verbose. If you move class-level dependencies out of the unit test, you will reduce test code verbosity. If your class has a dependency on four other classes at the class level that minimum four lines of code out of each test.
• Consistency. Developers tend to write unit tests their way. Fine if they are the only people reading their code; but this is rarely the case. Therefore, the more consistency we have across the tests the easier they are to maintain. So if you read a test you have never read before and at least see variables being stubbed and mocked in specific places for specific reasons, you will find the unit test code easier to read.

Variable Declaration Order 

This is a follow on from the last point. Declaring the variables in the right place is a great start, the next thing is to do in the same order they appear in the code. So, if we have something like below.

public class BusinessDelegate {

    private BusinessEntityValidator businessEntityValidator;
    private DbFacade dbFacade;
    private ExcepctionHandler exceptionHandler;

    @Inject
    BusinessDelegate(BusinessEntityValidator businessEntityValidator, DbFacade dbFacade, ExcepctionHandler exceptionHandler) {
        // ...
        // ...
    }
    public BusinessEntity read(Request request, Key key) {
         // ... 

    }
    
}

It is much easier to read the test code if they stubs and mocks are defined in the same order as the way the class declares them.

class BusinessDelegateSpec {
    @Subject BusinessDelegate businessDelegate
    //  class level dependencies in the same order
    def businessEntityValidator
    def dbFacade
    def exceptionHandler

    def setup() {
        businessEntityValidator = Stub(BusinessEntityValidator)
        dbFacade = Mock(DbFacade)
        exceptionHandler =  Mock(ExceptionHandler)
        businessDelegate = new BusinessDelegate(businessEntityValidator, dbFacade, exceptionHandler)
    }

    def "read(Request request, Key key)"() {
        given:
            def request = Stub(Request)
            def key = Stub(key)
        when:
            businessDelegate.read(request, key)
        then:
            // ...
    }
}

Variable Naming 

And if you thought the last point was pedantic, you'll be glad to know this one also is. The variable names used to represent the stubs and mocks should be the same names that are used in the actual code. Even better, if you can name the variable the same as the type in the code under test and not lose any business meaning then do that. In the last code sample, the parameter variables are named requestInfo and key and they corresponding stubs have the same names. This is much easier to follow than doing something like this:

//.. 
public void read(Request info, Key someKey) {
  // ...
}

// corresponding test code
def "read(Request request, Key key)"() {
    given:
        def aRequest = Stub(Request)
        def myKey = Stub(key)  // you ill get dizzy soon!
        // ... 

Avoid Over Stubbing 

Too much stubbing (or mocking) usually means something has gone wrong. Let's consider the Law of Demeter. Imagine some telescopic method call...

List queryBusinessEntities(Request request, Params params) {
    // check params are allowed
    Params paramsToUpdate =        queryService.getParamResolver().getParamMapper().getParamComparator().compareParams(params)
    // ...
    // ...
}

It is not enough to stub queryService. Now whatever is returned by resolveAllowableParams() has to be stubbed and that stub has to have mapToBusinessParamsstubbed() which then has to have mapToComparableParams() stubbed. Even with a nice framework like Spock which minimizes verbosity, you will have to four lines of stubbing for what is one line of Java code.

def "queryBusinessEntities()"() {
   given: 
      def params = Stub(Params)
      def paramResolver = Stub(ParamResolver)
      queryService.getParamResolver() = paramResolver
      def paramMapper = Stub(ParamMapper)
      paramResolver.getParamMapper() >> paramMapper
      def paramComparator = Stub (ParamComparator)
      paramMapper.getParamComparator() >> paramComparator
      Params paramsToUpdate = Stub(Params)
      paramComparator.comparaParams(params) >> paramsToUpdate
   when:
       // ...
   then: 
        // ...
}

Yuck! Look at how that one line of Java does to our unit test. It gets even worse if you are not using something like Spock. The solution is to avoid telescopic method calling and try to just use direct dependencies. In this case, just inject theParamComparator directly into our class. Then the code becomes...

List queryBusinessEntities(Request request, Params params) {
    // check params are allowed
    Params paramsToUpdate = paramComparator.compareParams(params)
    // ...
    // ...
}

and the test code becomes

setup() {
    // ...
    // ...
    paramComparator = Stub (ParamComparator)
    businessEntityDelegate = BusinessEntityDelegate(paramComparator) 
}

def "queryBusinessEntities()"() {
   given: 
      def params = Stub(Params)
      Params paramsToUpdate = Stub(Params)
      paramComparator.comparaParams(params) >> paramsToUpdate
   when:
       // ..
   then: 
        // ...
}

All of the sudden people should be thanking you for feeling less dizzy. 

Gherkin Syntax 

Bad unit tests have horrible things like asserts all over the place The top the middle and the bottom. It can very quickly get nauseating trying to figure out which ones are important, which ones are redundant and which ones require which bit of set up etc etc. Schematic things are easier to follow. That is the real advantage of the Gherkin syntax. The scenario is set up in the given: always, the when: is the test scenario and then: is what we expect. Even better using, something like Spock means you have a nice, neat DSL so that the given:, when:, and then: can all be co-located in the one test method.

Narrow When Wide Then 

If a unit test is testing four methods, is it a unit test? Consider the below test:

def "test several methods" {
    given: 
        // ...
    when:
        def name = personService.getname();
        def dateOfBirth = personService.getDateOfBirth();
        def country = personService.getCountry();
    then:
        name == "tony"
        dateOfBirth == "1970-04-04"
        country == "Ireland"
}

First up, if Jenkins tells you this failed, you are going to have to root around and figure out what part of the class is wrong. Because the test doesn't focus on a specific method you don't know immediately which method is failing. Second up, say if it is getName() that is failing, how do you know getDateOfBirth() and getCountry() are working? The test stops on the first failure. So when the test fails, you don't even know if you have one method not working or three methods not working. You can go around telling everyone you have 99% code coverage and one test failing. But — how much was that one test really doing?
Furthermore, what's easier to fix? A small test or a long test? Ideally, a test should check a single interaction with the thing you are testing. Now, this doesn't mean you can only have one asset, but you should have a narrow when and a wide then.
So let's take the narrow when first. Ideally, one line of code only. The one line of code matches the method you are unit testing.

def "getName()" {
    given: 
        // ...
    when:
        def name = personService.getname();
    then:
        name == "tony"
}

def "getDateOfBirth()" {
    given: 
        // ...
    when:
        def dateOfBirth = personService.getDateOfBirth();
    then:
        dateOfBirth == "1970-04-04"
}

def "getCountry()" {
    given: 
        // ...
    when:
        def country = personService.getCountry();
    then:
        country == "Ireland"
}

Now we could have the exact same code coverage, if getName() fails but getCountry() and getDateOfBirth() pass, but there is a problem with getName() and not getCountry() and getDateOfBirth(). Getting the granularity of a test is an entirely different stat to code coverage. It should be ideally one unit test minimum for every non-private method. It is more when you factor in negative tests etc. It is perfectly fine to have multiple asserts in a unit test. For example, suppose we had a method that delegated onto other classes.
Consider a method resyncCache() which in its implementation calls two other methods on a cacheService object, clear() and reload().

def "resyncCache()" {
    given: 
        // ...
    when:
        personService.resyncCache();
    then:
        1 * cacheService.clear()
        1 * cacheService.reload()
}

In this scenario, it would not make sense to have two separate tests. The "when" is the same and if either fails, you know immediately which method you have to look at. Having two separate tests just means twice the effort with little benefit. The subtle thing to get right here is to ensure your assets are in the right order. They should be in the same order as code execution. So, clear() is invoked before reload(). If the test fails at clear(), there is not much point going on to check to reload() anyway as the method is broken. If you don't follow the assertion order tip, and assert on reload() first and that is reported as failing, you won't know if clear() invocation which is supposed to happen first even happened. Thinking this way will make help you become a Test Ninja!
The ordering tip for mocking and stubbing, the same applies to assert. Assert in chronological order. It's pedantic but it will make test code much more maintainable.

Parameterization 

The parameterization is a very powerful capability that can greatly reduce test code verbosity and rapidly increase branch coverage in code paths. The Unit Test Ninja should be always able to spot when to use it!
An obvious indication that a number of tests could be grouped into one test and parameterized is that they have the same when blocks, except for different input parameters.
For example, consider the below.

def "addNumbers(), even numbers"() {
    given:
      // ...
    when:
      def answer = mathService.addNumbers(4, 4);
    then:
      // ...
}

def "addNumbers(), odd numbers"() {
    given:
      // ...
    when:
      def answer = mathService.addNumbers(5, 5);
    then:
      // ...
}

As we can see here the when is the same except the input parameters. This is a no-brainer for parameterization.

@Unroll("number1=#number1, number2=#number2")  // unroll will provide the exact values in test report
def "addNumbers()"(int number1, int number2) {
    given:
      // ...
    when:
      def answer = mathService.addNumbers(number1, number2);
    then:
      // ...
    where:
      number1   | number2   || answer
      4         | 4         || 8
      5         | 5         || 10
}

Immediately we get a 50% reduction in code. We have also made it much easier to add further permutations by just adding another row to the where table. So, while it may seem very obvious that these two tests should have been the one parameterized test, it is only obvious if the maxim of having a narrow when is adhered to. The narrow "when" coding style makes the exact scenario being tested much easier to see. If a wide when is used with lots of things happening it is not and therefore spotting tests to parameterize is harder.
Usually, the only time to not parameterize a test that has the same syntactic where: code block is when the expectations are a completely different structure. Expecting an int is the same structure, expecting an exception in one scenario and an int in another means you have two different structures. In such scenarios, it is better not to parameterize. A proverbial (and infamous) example of this is mixing a positive and negative test.
Suppose our addNumbers() method will throw an exception if it receives a float, that's a negative test and should be kept separate. A then: block should never contain an if statement. It is a sign a test is becoming too flexible and a separate test with no if statements would make more sense.

Summary 

Clean unit testing is very important if you want to have maintainable code, release regularly and enjoy your Software Engineering more.