Unit tests that really do test

Unit tests are used in every project. Undeniably, they prove handy, but only if used in a correct way. In my career, I have seen many antipatterns in unit tests that made test suites unreadable, hard to maintain, or even failing to test functionalities at all. In this short article, I will list some of those and present simple ways to improve your unit tests.

Readability and maintainability

First things first, test cases should be as short and self-describing as possible. This approach not only improves readability but also allows easier maintenance. I believe that test methods that are too complex are the root cause of worthless test suites.

It’s common practice to make multiple when(mock.called()).then(...) statements, object initializations, etc. at the beginning of a test method. The same applies to a verification part where multiple assertions are done and mocks checked. This leads to test cases looking like the one here:

// all test cases are written in Groovy, but the same applies to pure Java
@Test
void "should commit configuration"() {
    def configuration = new Configuration()
    configuration.status = DRAFT
    // setting other fields
    when(requestProvider.getUsername()).thenReturn('username')
    when(configurationDao.get(123)).thenReturn(configuration)
    // other when calls, etc.

    configurationService.commitConfiguration(123)

    assert configuration.status == PENDING
    verify(configurationDao).merge(configuration)
    verify(someOtherServices).someMethodCalls(anyString())
    // other asserts and verifies
}

At first glance, it seems fine — setting all preconditions, calling tested method, verifying what was done. The problem arises with multiple conditions within the commitConfiguration() method. Most likely the above method would be cloned and have two or three lines changed, thus leading to many similar co-existing methods doing almost the same for no obvious reason.

According to the DRY principle, object preparation should be extracted to a utility method where common initialization is performed. When different test cases use similar data configurations, all of them should use that method and change just one parameter for a specific case. Optional Consumer can be used to configure input data.

@Test(expected = IllegalArgumentException)
void "commit configuration should fail for not a draft configuration"() {
    givenConfiguration({ configuration ->
        configuration.status = ACTIVE
    })

    configurationService.commitConfiguration(123)
}

def givenConfiguration(Consumer<Configuration> consumer = {}) {
    def configuration = new Configuration()
    configuration.status = DRAFT
    // setting other fields
    consumer.accept(configuration)
    when(configurationDao.get(123)).thenReturn(configuration)
    // other when calls, etc.
    return configuration
}

What’s more, with a test method checking too many cases, it’s tempting to add just one more check when adding new functionality. We should divide all these into separate test cases in a suite.

@Test
void "commit configuration should update status to pending"() {
    def configuration = givenConfiguration()

    configurationService.commitConfiguration(123)

    assert configuration.status == PENDING
    verify(configurationDao).merge(configuration)
}

@Test
void "commit configuration should call other services"() {
    givenConfiguration()

    configurationService.commitConfiguration(123)

    verify(someOtherServices).someMethodCalls(“some argument”)
}

We arrive at each test case being clear and self-describing. If any of the test methods fails, a developer immediately knows which functionality is broken. Adding new functionality to production code is easy from a test suite perspective — a new test method verifying the functionality needs to be added, without any changes to other test cases.

Refactoring is common when dealing with production code. It may break some tests as a result of, for example, adding or removing method parameters. Fixing these kinds of problems is quick but needs to be done in multiple test cases. The previously described solution enables the developer to fix only one method and make the whole test suite green.

If tests fail after fixing compilation errors, then most likely regression defects are present in the refactored code. Naming test methods properly helps a lot in fixing the production code. When a test suite contains only complex methods, it’s difficult to distinguish whether a bug in the refactored code exists or a test method needs fixing. That might lead to changing test method to pass even though it should not. If test suites cannot find regression bugs, then what’s the point of maintaining them at all?

Verification

Properly written unit tests should verify business logic rather than implementation details. This is not as hard to achieve as it may sound at first.

It’s common practise to create methods in DAOs, which builds complex criteria and query objects based on them. The idea to use Hibernate API is much better than a pure native query. Consider an example method checking hostname usage in any other configuration than the one specified:

public boolean isHostnameInUseExcludingConfiguration(String hostname, Integer configId) {
    Criteria criteria = getSession()
            .createCriteria(Configuration.class)
            .createAlias("configVersion", "configVersion")
            .add(Restrictions.eq("hostName", hostname))
            .add(Restrictions.ne("id", configId))
            .add(Restrictions.not(
                 Restrictions.in("configVersion.status",
                                 STATUSES_NOT_ALLOWED)))
            .setProjection(Projections.rowCount());

    Long count = (Long) criteria.uniqueResult();
    return count > 0;
}

There are two ways to verify that the method works correctly.

The first one is to use mocks and check every call like:

verify(criteria).createAlias(“configVersion”, “configVersion”)
verify(criteria).add(Restrictions.eq(“hostName”, hostname))

I’ve seen similar unit tests for complex DAO methods in multiple projects. They check nothing valuable:

• Misspells — should it say "hostname" or "hostName"?
• Conditions — this is an uncomplicated example, but it is not hard to miss the negation or mistake .lt() with .gt() or .le(), etc., especially when working with ranges of data;
• Aliases — frankly, I think that I have never written a query with aliases that worked at first try. Unit tests with mocks would not help with that for sure.

The second one is to simply check logic instead of implementation — use an in-memory DB like H2 and query real data. A few objects with various data should be created and saved in the database. The test case should call the tested method with different parameters to check if a result of the method is correct depending on the previously saved objects. This is what is important, not if an alias was made to "configVersion" or "cv".

Testing DAOs is clear as there is only one dependency (DB), which instead of mocking can be replaced with a simple implementation. It’s getting more complicated when dealing with the service layer of an application.

There is an approach to change mocked DAOs with simple implementations. You can use Map to store objects and test the whole service layer with end to end tests. I’m not overly enthusiastic about this this way, because of two reasons:

• test methods become complex, and it violates everything I have written in the first section;
• it requires implementing each DAO for test suits. Methods like save() or get() are easy, but many complex methods may require reimplementation.

I prefer a more traditional way of working with Junit and Mockito — mock every dependency of a service class.

The important thing to focus on is to check the correctness of output data for a given input. Not only for a happy path but also for edge cases and possible failures. Assertions using any() methods should be avoided, if possible, and replaced with expected values. That way unit tests will find regression if the output values are changed due to a bug introduced during development.

There is definitely no need to check every mock call. If an external service needs to be called in a specified condition two cases should be added to verify whether it was or wasn’t. On the other hand, verifying let’s say a repository’s get() method is useless as the method will not produce the expected output without it.

Going for 100% line coverage

Forcing developers to achieve 100% line coverage is a mistake. It requires testing getters, setters, constructors, equals/hashcode, proxy methods, etc. It’s time-consuming and fails to test valuable.

Even 100% line coverage does not guarantee that there are no bugs in production code. Consider an example of a badly written method that returns a cached object and refreshes the instance if requested:

private final Map<Integer, Configuration> configurations;

public Configuration getConfiguration(int id, boolean refresh) {
    if (refresh) {
        configurations.put(id, configurationRepository.get(id));
    }
    return configurations.get(id);
}

It is easy to see that no object will be added to the cache if a refresh is never requested. Yet a possible test case might look like this:

@Test
void "should return configuration"() {
    def configuration = new Configuration()
    when(configurationRepository.get(123)).thenReturn(configuration)
    def result = configurationService.getConfiguration(123, true)
    assert result == configuration
}

The above test case would provide 100% line coverage even though not all possible input parameters are passed into the method. It would give a false positive that the method is tested and works correctly.

To verify how good the test suite really is, branch coverage should be checked. For the above method, it would be 50% instead of 100% like in the line coverage, which is much more valuable. It’s not hard to imagine much more complex code with multiple if-statements, loops, calls to other private methods, etc. Branch coverage will tell us how many of all possible routes were covered.

Of course, branch coverage works along with correct assertions done at the ends of the test methods. Having 100% branch coverage without correct output verification is useless. The only check without output verification confirms that probably no NullPointerException will be thrown.

As I mentioned, this was a trivial example of a badly written method, but it’s supposed to be like that to clearly show the problem of line coverage. I expect these kinds of methods to be declined during code reviews.

Testing external dependencies

Logically, the more test cases and complex methods to verify, the longer test suites are running. in many cases it is possible to shorten the time of running all test suites.

Initialization of the Spring context takes a significant amount of time and is not required in most cases. Most classes should be tested without the Spring context with mocks injected in the test suite. One test checking if a context configuration is correct is more than enough.

The spring context is required for checking the API and DAO layers, but it’s not required to initialize the whole context. Spring Boot allows creating partial context using e.g. WebMvcTest or DataJpaTest annotations:

• WebMvcTest creates rest controllers and allows injecting mocks. An autowired MockMvc component can be used to perform rest calls and verify the API.
• DataJpaTest starts up an in-memory embedded database and initializes Repository and Entity classes only.

Changing full context initialization to partial one can significantly speed up test suites and artifact builds.

Summary

The point of this article is to show my perspective on writing unit tests. Both my team and I are developing test suites using the guidelines above. I hope you find some of them useful and valuable enough to start practicing in your projects.