Item 1: Avoid Coding to Coverage

By Ashley Waldron

All of the source code for this series can be found in this repository. Specifically, the code used throughout this item can be found here and its corresponding test classes found here.

One of the major pitfalls I see developers making when writing unit tests is using code coverage tools (such as Sonar) as a box to tick rather than as a guide to help you fill in the missing pieces. Sonar coverage becomes the goal and writing proper tests takes a back seat.

Developers should approach writing tests from the point of view of what the code does rather than what the current Sonar score is. Look through the method. What conditions does it contain? What exceptions does it catch? What’s it doing? What’s it supposed to be doing? How is it doing it?

Write unit test after unit test until you think that all scenarios and logical conditionals (and their combinations) have been covered. It’s better to not even use your code coverage tool until you think you’ve covered all scenarios you can think of and, in my opinion it’s safer not to use code coverage reports until you reach that point. Once you can’t think of any more scenarios to test then run your code coverage tool. At that point it might flag some lines that have not been reached. Then you can start writing tests from the point of view of the lines that have not been covered. Each line or combination of uncovered lines represents a unique scenario, so take note of those scenarios and write tests for each. Repeating this until you get to 100% code coverage.

Shifting your mindset

I would argue that “lines not covered” isn’t the correct way to think of it. What code coverage tools really flag is missing scenario coverage. Every unit test you write is a scenario that can occur in the code. Code coverage tools can only flag what lines (or combination of conditions) haven’t been exercised (they can’t understand your code or its intent) but a group of 1 or more uncovered lines (or conditions) really corresponds to an untested scenario.

Don’t use code coverage metrics as a game to win, a goal to beat or a hoop you need to jump through. Use them as a tool to help guide you on which scenarios you have not covered yet. Using this concept to write unit tests can bring a better understanding into how the code behaves, what it does and (usually) why it does it. Unit tests are written from the point of view of “what exactly is this code doing and why?” rather than “what lines have I yet to cover?”. By understanding the code, you’re more likely to spot when there’s a subtle flaw in it and subsequently fix it.

If we merely think about “what lines have I not covered yet?” then we run the risk of writing unit tests to the wrong design. The code could contain a logical mistake and since you’re coding to the line coverage rather than the logic that the code should enforce, you run the risk of writing tests to the wrong design. Whereas with the scenario approach outlined here, you’re forcing yourself to understand the code first, what it does and how it does it. Missing code coverage then simply represents scenarios that you’ve not yet thought of. This is where the real advantage of using Sonar comes in.

When you approach unit tests with this scenario centric thinking, your asserts are more likely to be very strong. The whole point of a unit test is to drive a scenario and carry out verifications that things happened the way they were supposed to. When developers focus on lines of coverage they often don’t think about assertions. They are more focused on getting the score high enough regardless of whether or not the code actually did what it was supposed to.

The dangers of coding to coverage

For an example of a real world experience I had with unit tests that were coded to coverage: I once joined a project which was already meeting the required Sonar line coverage score (it was at 93%), but when I inspected the unit tests I realized that they were extremely poor quality. Many of them didn’t really assert anything and the only asserts they had were assertNotNull (which is usually a proxy for “not actually testing anything”), an almost always useless assert.

As an exercise to show how bad these tests were, I picked a class at random and ripped out every single line of code from each method in the class. Any method which returned something just returned an empty object or value. I then ran the unit tests.

Not only did every single one of those tests still pass but the code coverage actually went up to 100%!

Not only are these tests useless, they’re worse than useless. These types of unit tests are extremely dangerous. If I see that a class has very high code coverage I will be very confident that if I break something the unit tests will let me know. This confidence is a key factor in being able to refactor and update code quickly and effectively. The problem here is that if I start refactoring or updating code I can break a bunch of stuff and I won’t know because the tests will all pass. It would actually be safer if there were no tests at all because in that case, I would know that I need to be extremely careful when updating any existing code. These types of tests aren’t tests at all, they’re code coverage box tickers which actually reduce code quality and make the code less safe to work on.

Some code smells to look out for when trying to spot unit tests that are “coded to coverage“

• assertNotNull is used extensively and is usually the only assert (or one of very few asserts) in the test – There is almost never any reason to use assertNotNull anyway, even as part of a large number of well written asserts. Sometimes developers will argue that the field being asserted is some random value like a timestamp which is generated by the code. In this case if you’re using Spring just @Autowire in a Clock, mock it in the unit test like everything else and tell it exactly what timestamp to generate. If this isn’t possible then you can still usually assert that the timestamp is within the last X [time interval] rather than simply asserting that it’s not null. Likewise, if the field is an automatically generated UUID (e.g. using Java’s static UUID.randomUUID() method), you can use regex to verify that the field contains a properly formatted UUID. Other similar challenges can be solved using these types of techniques that provide higher quality checks.
• Poor unit test method names – Over time as developers code to coverage rather than coding to scenarios, the method names tend to become completely unhelpful in describing what the test is trying to do. Method names should describe exactly what the scenario is. When coding to coverage the logic/business scenario is not really considered as the main reason to write the test and this often tends to lead to bad method names, especially as the code evolves over time.
• Mockito lenient is used throughout all unit test mock setup lines – Although this is as much a result of bad/lazy test coding than exclusively a coding to coverage smell. What tends to happen here is that developers haven’t understood exactly what the code is doing or what the full scenario really is, so they copy all the mock setups from one test to the next until they get it working. Nowadays Mockito throws errors if unnecessary mock setups are made but that is overridden with the lenient option. The lenient option only really exists for backwards compatibility reasons so it generally shouldn’t be used any more (I’ll write a separate item of its own to talk about this).
• ArgumentMatchers.any() method is used extensively in mock setups – As long as you’re not using the lenient option, Mockito setups very often double as verifications. If you use ArgumentMatchers.any() then you’re losing this benefit of the setups. The wrong parameter value could be passed to the mock but it would still behave correctly. Whereas if you use actual objects or the ArgumentMatchers.eq() method in the setups then the mock won’t work correctly and the test will usually blow up if the wrong value is passed (but that’s not always reliable as we’ll discuss in an advanced item). Also: this only works for mocked methods that don’t return void. We’ll talk more about that in a later item.

Beginning our Effective Unit Tests journey

Over the next few items we’ll walk through the creation and gradual improvement of a unit test to demonstrate the effectiveness of taking the points above into consideration. We’ll start with the following class [See Item1UserService.java]:

package com.effective.unit.tests.service.item1;
 
import com.effective.unit.tests.domain.CreateUserProfileRequest;
import com.effective.unit.tests.domain.CreateUserProfileResponse;
import com.effective.unit.tests.domain.UserEntity;
import com.effective.unit.tests.domain.enums.EventType;
import com.effective.unit.tests.repositories.UserRepository;
import com.effective.unit.tests.service.EmployeeNumberGenerator;
import com.effective.unit.tests.service.EventBroadcaster;
import com.effective.unit.tests.service.MarketingEmailService;
 
import java.util.UUID;
 
public class Item1UserService {
 
    private MarketingEmailService marketingEmailService;
    private UserRepository userRepository;
    private EventBroadcaster eventBroadcaster;
    private EmployeeNumberGenerator employeeNumberGenerator;
 
    public CreateUserProfileResponse create(CreateUserProfileRequest createUserProfileRequest) {
        validate(createUserProfileRequest);
        UserEntity userEntity = new UserEntity();
        userEntity.setId(UUID.randomUUID().toString());
        userEntity.setEmailAddress(createUserProfileRequest.getEmailAddress());
        userEntity.setRegion(createUserProfileRequest.getRegion());
        userEntity.setEmployeeNumber(employeeNumberGenerator.generate(createUserProfileRequest.getRegion()));
        UserEntity createdUserEntity = userRepository.save(userEntity);
 
        eventBroadcaster.broadcast(EventType.USER_REGISTRATION, createdUserEntity);
        if(createUserProfileRequest.shouldReceiveMarketingEmails()) {
            marketingEmailService.registerSubscriber(userEntity);
        }
 
        CreateUserProfileResponse createUserProfileResponse = new CreateUserProfileResponse();
        createUserProfileResponse.setId(userEntity.getId());
        createUserProfileResponse.setEmployeeNumber(userEntity.getEmployeeNumber());
        createUserProfileResponse.setEmailAddress(userEntity.getEmailAddress());
        createUserProfileResponse.setRegion(userEntity.getRegion());
        return createUserProfileResponse;
    }
 
    private void validate(CreateUserProfileRequest createUserProfileRequest) {
        if(createUserProfileRequest.getRegion() == null || createUserProfileRequest.getRegion().length() > 30) {
            throw new RuntimeException("Region must be less than 30 characters in length");
        }
    }
}

Looking through the code above we see the following trail of logic:

First the method validates the object, if the region is null an exception is thrown ^{scenario No. 1}, if the region is more than 30 characters long an exception is thrown ^{scenario No. 2} . If the region is less than 31 characters long the code execution continues ^{Happy Path Scenario} – We’ll call a scenario that runs through as much code as possible the ‘happy path scenario’ but that’s an arbitrary choice – After that the code creates a new UserEntity object, setting the various fields and then saves it. Then an event is broadcast. Then if the shouldReceiveMarketingEmails flag is set it will register the newly created UserEntity with the marketing email service ^{Happy Path Scenario} . If the shouldReceiveMarketingEmails flag is not set it will not register the newly created UserEntity with the marketing email service ^{scenario No. 3}. Then it creates a new CreateUserProfileResponse object, sets the correct fields and finally returns it.

So altogether that’s 3 separate scenarios plus the ‘happy path scenario’ giving us 4 scenarios in total. This means that we’ll have to write at least 4 tests for this method.

Happy Path Scenario

Let’s start with the happy path scenario [See Item1UserServiceTest.java]:

package net.theresnolimits.partialoverflow.effective.unit.tests.service.item1;

import net.theresnolimits.partialoverflow.effective.unit.tests.domain.CreateUserProfileRequest;
import net.theresnolimits.partialoverflow.effective.unit.tests.domain.CreateUserProfileResponse;
import net.theresnolimits.partialoverflow.effective.unit.tests.domain.UserEntity;
import net.theresnolimits.partialoverflow.effective.unit.tests.repositories.UserRepository;
import net.theresnolimits.partialoverflow.effective.unit.tests.service.EmployeeNumberGenerator;
import net.theresnolimits.partialoverflow.effective.unit.tests.service.EventBroadcaster;
import net.theresnolimits.partialoverflow.effective.unit.tests.service.MarketingEmailService;
import net.theresnolimits.partialoverflow.effective.unit.tests.service.item1.Item1UserService;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

import java.util.UUID;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.text.MatchesPattern.matchesPattern;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.BDDMockito.given;

@ExtendWith(MockitoExtension.class)
public class Item1UserServiceTest {

    private static final String UUID_PATTERN = "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$";

    @InjectMocks
    private Item1UserService item1UserService;

    @Mock
    private UserRepository userRepository;

    @Mock
    private EventBroadcaster eventBroadcaster;

    @Mock
    private MarketingEmailService emailService;

    @Mock
    private EmployeeNumberGenerator employeeNumberGenerator;

    private UserEntity mockSavedUserEntity = new UserEntity();

    @Test
    void createSuccessItem1() {
        mockSavedUserEntity.setEmployeeNumber("TestUsername");
        mockSavedUserEntity.setRegion("EUS1002");
        mockSavedUserEntity.setEmailAddress("TestEmailAddress@test.com");
        mockSavedUserEntity.setId(UUID.randomUUID().toString());
        given(userRepository.save(any())).willReturn(mockSavedUserEntity);
        given(employeeNumberGenerator.generate(anyString())).willReturn("EUS1002");
        CreateUserProfileRequest createUserProfileRequest = new CreateUserProfileRequest();
        createUserProfileRequest.setRegion("TestEmailAddress@test.com");
        createUserProfileRequest.setRegion("US-EAST");

        CreateUserProfileResponse createUserProfileResponse = item1UserService.create(createUserProfileRequest);

        assertThat(createUserProfileResponse.getEmployeeNumber(), is(equalTo("EUS1002")));
        assertThat(createUserProfileResponse.getRegion(), is(equalTo(createUserProfileRequest.getRegion())));
        assertThat(createUserProfileResponse.getEmailAddress(), is(equalTo(createUserProfileRequest.getEmailAddress())));
        assertThat(createUserProfileResponse.getId(), matchesPattern(UUID_PATTERN));
    }
}

The above test contains enough mock setup to get the code to run right to the end of the method and return something (there’s an obvious enough bug in it which you might have spotted and that we can ignore for now). It will cover 87% of the lines of code in the class so it might seem like we’re almost done with our unit testing. But we’re nowhere near finished, we’re barely even started. As mentioned above there are at least 3 more scenarios we need to cover but more importantly the code above contains only the bare minimum of asserts. So what’s left to assert?

1. We didn’t make sure that the new UserEntity object which was saved contained the correct values for each field. We’ve used the any() argument matcher on line 55, but that matcher should be used very infrequently, there’s only really one case where I consider using it to be acceptable. We have the same problem with the employeeNumberGenerator.generate() method.
2. We didn’t assert that EventBroadcaster.broadcast() was called with the correct values.
3. We didn’t assert that MarketingEmailService.registerSubscriber() was called with the correct values. 

We’ll correct these problems over the next several items and along the way we’ll uncover more and more issues with both the code and the test code until we’ve got a foolproof test that guarantees the correctness of the code.