Writing Clean Tests - Beware of Magic

Magic is the arch enemy of readable code, and one of the most common form of magic which can be found from our code is a magic number.

Magic numbers litters our source code, and transforms it into a pile of unreadable and unmaintainable rubbish.

That is why we should avoid magic numbers at all costs.

This blog post demonstrates what kind of an effect magic numbers have to our test cases, and describes how we can eliminate them by using constants.

Constants to the Rescue

We use constants in our code because without constants our code would be littered with magic numbers. Using magic numbers has two consequences:

  1. Our code is hard to read because magic numbers are just values without meaning.
  2. Our code is hard to maintain because if we have to change the value of a magic number, we have to find all occurrences of that magic number and update everyone of them.

In other words,

  • Constants help us to replace magic numbers with something that describes the reason of its existence.
  • Constants make our code easier to maintain because if the value of a constant changes, we have to make that change only to one place.

If we think about the magic numbers found from our test cases, we notice that they can be divided into two groups:

  1. Magic numbers which are relevant to a single test class. A typical example of this kind of magic number is the property value of an object which created in a test method. We should declare these constants in the test class.
  2. Magic numbers which are relevant to multiple test classes. A good example of this kind of magic number is the content type of a request processed by a Spring MVC controller. We should add these constants to a non-instantiable class.

Let’s take a closer look at both situations.

Declaring Constants in the Test Class

So, why should we declare some constants in our test class?

After all, if we think about the benefits of using constants, the first thing that comes to mind is that we should eliminate magic numbers from our tests by creating classes which contains the constants used in our tests. For example, we could create a TodoConstants class which contains the constants used in the TodoControllerTest, TodoCrudServiceTest, and TodoTest classes.

This is a bad idea.

Although it is sometimes wise to share data in this way, we shouldn’t make this decision lightly because most of the time our only motivation to introduce constants in our tests is to avoid typos and magic numbers.

Also, if the magic numbers are relevant only to the a single test class, it makes no sense to introduce this kind of dependency to our tests just because we want to minimize the number of created constants.

In my opinion, the simplest way to deal with this kind of situation is to declare constants in the test class.

Let’s find out how we can improve the unit test described in the previous part of this tutorial. That unit test is written to test the registerNewUserAccount() method of the RepositoryUserService class, and it verifies that this method is working correctly when a new user account is created by using a social sign provider and a unique email address.

The source code of the that test case looks as follows:

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.runners.MockitoJUnitRunner;
import org.mockito.stubbing.Answer;
import org.springframework.security.crypto.password.PasswordEncoder;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.mockito.Matchers.isA;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;


@RunWith(MockitoJUnitRunner.class)
public class RepositoryUserServiceTest {

    private RepositoryUserService registrationService;

    @Mock
    private PasswordEncoder passwordEncoder;

    @Mock
    private UserRepository repository;

    @Before
    public void setUp() {
        registrationService = new RepositoryUserService(passwordEncoder, repository);
    }


    @Test
    public void registerNewUserAccount_SocialSignInAndUniqueEmail_ShouldCreateNewUserAccountAndSetSignInProvider() throws DuplicateEmailException 		{
        RegistrationForm registration = new RegistrationForm();
        registration.setEmail("john.smith@gmail.com");
        registration.setFirstName("John");
        registration.setLastName("Smith");
        registration.setSignInProvider(SocialMediaService.TWITTER);

        when(repository.findByEmail("john.smith@gmail.com")).thenReturn(null);

        when(repository.save(isA(User.class))).thenAnswer(new Answer<User>() {
            @Override
            public User answer(InvocationOnMock invocation) throws Throwable {
                Object[] arguments = invocation.getArguments();
                return (User) arguments[0];
            }
        });

        User createdUserAccount = registrationService.registerNewUserAccount(registration);

        assertEquals("john.smith@gmail.com", createdUserAccount.getEmail());
        assertEquals("John", createdUserAccount.getFirstName());
        assertEquals("Smith", createdUserAccount.getLastName());
        assertEquals(SocialMediaService.TWITTER, createdUserAccount.getSignInProvider());
        assertEquals(Role.ROLE_USER, createdUserAccount.getRole());
        assertNull(createdUserAccount.getPassword());

        verify(repository, times(1)).findByEmail("john.smith@gmail.com");
        verify(repository, times(1)).save(createdUserAccount);
        verifyNoMoreInteractions(repository);
        verifyZeroInteractions(passwordEncoder);
    }
}

The problem is that this test case uses magic numbers when it creates a new RegistrationForm object, configures the behavior of the UserRepository mock, verifies that information of the returned User object is correct, and verifies that the correct method methods of the UserRepository mock are called in the tested service method.

After we have removed these magic numbers by declaring constants in our test class, the source code of our test looks as follows:

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.runners.MockitoJUnitRunner;
import org.mockito.stubbing.Answer;
import org.springframework.security.crypto.password.PasswordEncoder;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.mockito.Matchers.isA;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;


@RunWith(MockitoJUnitRunner.class)
public class RepositoryUserServiceTest {

    private static final String REGISTRATION_EMAIL_ADDRESS = "john.smith@gmail.com";
    private static final String REGISTRATION_FIRST_NAME = "John";
    private static final String REGISTRATION_LAST_NAME = "Smith";
    private static final Role ROLE_REGISTERED_USER = Role.ROLE_USER;
    private static final SocialMediaService SOCIAL_SIGN_IN_PROVIDER = SocialMediaService.TWITTER;

    private RepositoryUserService registrationService;

    @Mock
    private PasswordEncoder passwordEncoder;

    @Mock
    private UserRepository repository;

    @Before
    public void setUp() {
        registrationService = new RepositoryUserService(passwordEncoder, repository);
    }


    @Test
    public void registerNewUserAccount_SocialSignInAndUniqueEmail_ShouldCreateNewUserAccountAndSetSignInProvider() throws DuplicateEmailException 		{
        RegistrationForm registration = new RegistrationForm();
        registration.setEmail(REGISTRATION_EMAIL_ADDRESS);
        registration.setFirstName(REGISTRATION_FIRST_NAME);
        registration.setLastName(REGISTRATION_LAST_NAME);
        registration.setSignInProvider(SOCIAL_SIGN_IN_PROVIDER);

        when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(null);

        when(repository.save(isA(User.class))).thenAnswer(new Answer<User>() {
            @Override
            public User answer(InvocationOnMock invocation) throws Throwable {
                Object[] arguments = invocation.getArguments();
                return (User) arguments[0];
            }
        });

        User createdUserAccount = registrationService.registerNewUserAccount(registration);

        assertEquals(REGISTRATION_EMAIL_ADDRESS, createdUserAccount.getEmail());
        assertEquals(REGISTRATION_FIRST_NAME, createdUserAccount.getFirstName());
        assertEquals(REGISTRATION_LAST_NAME, createdUserAccount.getLastName());
        assertEquals(SOCIAL_SIGN_IN_PROVIDER, createdUserAccount.getSignInProvider());
        assertEquals(ROLE_REGISTERED_USER, createdUserAccount.getRole());
        assertNull(createdUserAccount.getPassword());

        verify(repository, times(1)).findByEmail(REGISTRATION_EMAIL_ADDRESS);
        verify(repository, times(1)).save(createdUserAccount);
        verifyNoMoreInteractions(repository);
        verifyZeroInteractions(passwordEncoder);
    }
}

This example demonstrates that declaring constants in the test class has three benefits:

  1. Our test case is easier to read because the magic numbers are replaced with constants which are named properly.
  2. Our test case is easier to maintain because we can change the values of constants without making any changes to the actual test case.
  3. It is easier to write new tests for the registerNewUserAccount() method of the RepositoryUserService class because we can use constants instead of magic numbers. This means that we don’t have to worry about typos.

However, sometimes our tests use magic numbers which are truly relevant to multiple test classes. Let’s find out how we can deal with this situation.

Adding Constants to a Non-Instantiable Class

If the constant is relevant for multiple test classes, it makes no sense to declare the constant in every test class which uses it. Let’s take a look at one situation where it makes sense to add constant to a non-instantiable class.

Let’s assume that we have to write two unit tests for a REST API:

  • The first unit test ensures that we cannot add an empty todo entry to the database.
  • The second unit test ensures that we cannot add an empty note to the database.
These unit tests use the Spring MVC Test framework. If you are not familiar with it, you might want to take a look at my Spring MVC Test tutorial.

The source code of the first unit test looks as follows:

import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.MediaType;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import org.springframework.test.context.web.WebAppConfiguration;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
import org.springframework.web.context.WebApplicationContext;

import java.nio.charset.Charset;

import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(classes = {WebUnitTestContext.class})
@WebAppConfiguration
public class TodoControllerTest {

    private static final MediaType APPLICATION_JSON_UTF8 = new MediaType(
            MediaType.APPLICATION_JSON.getType(), MediaType.APPLICATION_JSON.getSubtype(),
            Charset.forName("utf8")
    );

    private MockMvc mockMvc;

    @Autowired
    private ObjectMapper objectMapper;

    @Autowired
    private WebApplicationContext webAppContext;

    @Before
    public void setUp() {
        mockMvc = MockMvcBuilders.webAppContextSetup(webAppContext).build();
    }

    @Test
    public void add_EmptyTodoEntry_ShouldReturnHttpRequestStatusBadRequest() throws Exception {
        TodoDTO addedTodoEntry = new TodoDTO();

        mockMvc.perform(post("/api/todo")
                .contentType(APPLICATION_JSON_UTF8)
                .content(objectMapper.writeValueAsBytes(addedTodoEntry))
        )
                .andExpect(status().isBadRequest());
    }
}

The source code of the second unit test looks as follows:

import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.MediaType;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import org.springframework.test.context.web.WebAppConfiguration;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
import org.springframework.web.context.WebApplicationContext;

import java.nio.charset.Charset;

import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(classes = {WebUnitTestContext.class})
@WebAppConfiguration
public class NoteControllerTest {

    private static final MediaType APPLICATION_JSON_UTF8 = new MediaType(
            MediaType.APPLICATION_JSON.getType(), MediaType.APPLICATION_JSON.getSubtype(),
            Charset.forName("utf8")
    );

    private MockMvc mockMvc;

    @Autowired
    private ObjectMapper objectMapper;

    @Autowired
    private WebApplicationContext webAppContext;

    @Before
    public void setUp() {
        mockMvc = MockMvcBuilders.webAppContextSetup(webAppContext).build();
    }

    @Test
    public void add_EmptyNote_ShouldReturnHttpRequestStatusBadRequest() throws Exception {
        NoteDTO addedNote = new NoteDTO();

        mockMvc.perform(post("/api/note")
                .contentType(APPLICATION_JSON_UTF8)
                .content(objectMapper.writeValueAsBytes(addedNote))
        )
                .andExpect(status().isBadRequest());
    }
}

Both of these test classes declare a constant called APPLICATION_JSON_UTF8. This constant specifies the content type and the character set of the request. Also, it is clear that we need this constant in every test class which contains tests for our controller methods.

Does this mean that we should declare this constant in every such test class?

No!

We should move this constant to a non-instantiable class because of two reasons:

  1. It is relevant to multiple test classes.
  2. Moving it to a separate class makes it easier for us to write new tests for our controller methods and maintain our existing tests.

Let’s create a final WebTestConstants class, move the APPLICATION_JSON_UTF8 constant to that class, and add a private constructor to the created class.

The source code of the WebTestConstant class looks as follows:

import org.springframework.http.MediaType;

public final class WebTestConstants {
    public static final MediaType APPLICATION_JSON_UTF8 = new MediaType(
            MediaType.APPLICATION_JSON.getType(), MediaType.APPLICATION_JSON.getSubtype(), 
            Charset.forName("utf8")
    );
	
	private WebTestConstants() {
	}
}

After we have done this, we can remove the APPLICATION_JSON_UTF8 constants from our test classes. The source code of our new test looks as follows:

import com.fasterxml.jackson.databind.ObjectMapper;
import net.petrikainulainen.spring.jooq.config.WebUnitTestContext;
import net.petrikainulainen.spring.jooq.todo.dto.TodoDTO;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import org.springframework.test.context.web.WebAppConfiguration;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
import org.springframework.web.context.WebApplicationContext;

import java.nio.charset.Charset;

import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(classes = {WebUnitTestContext.class})
@WebAppConfiguration
public class TodoControllerTest {

    private MockMvc mockMvc;

    @Autowired
    private ObjectMapper objectMapper;

    @Autowired
    private WebApplicationContext webAppContext;

    @Before
    public void setUp() {
        mockMvc = MockMvcBuilders.webAppContextSetup(webAppContext).build();
    }

    @Test
    public void add_EmptyTodoEntry_ShouldReturnHttpRequestStatusBadRequest() throws Exception {
        TodoDTO addedTodoEntry = new TodoDTO();

        mockMvc.perform(post("/api/todo")
                        .contentType(WebTestConstants.APPLICATION_JSON_UTF8)
                        .content(objectMapper.writeValueAsBytes(addedTodoEntry))
        )
                .andExpect(status().isBadRequest());
    }
}

We have just removed duplicate code from our test classes and reduced the effort required to write new tests for our controllers. Pretty cool, huh?

If we change the value of a constant which is added to a constants class, this change effects to every test case which uses this constant. That is why we should minimize the number of constants which are added to a constants class.

Summary

We have now learned that constants can help us to write clean tests, and reduce the effort required to write new tests and maintain our existing tests. There are a couple of things which we should remember when we put the advice given in this blog post to practice:

  • We must give good names to constants and constants classes. If we don’t do that, we aren’t leveraging the full potential of these techniques.
  • We shouldn’t introduce new constants without figuring out what we want to achieve with that constant. The reality is often a lot more complex than the examples of this blog post. If we write code on autopilot, the odds are that we will miss the best solution to the problem at hand.
11 comments… add one
  • al0 Mar 25, 2015 @ 12:44

    The funny thing that examples do not contain any single magic number.
    And for good reasons - quite often there is no big sense to declare constants for numbers that are really magic (like 0 or 1).

    • Petri Mar 25, 2015 @ 13:22

      The funny thing that examples do not contain any single magic number.

      I disagree. Wikipedia gives one definition for magic number:

      Unique values with unexplained meaning or multiple occurrences which could (preferably) be replaced with named constants

      According to this definition, the example does contain magic numbers. I agree that declaring constant for the user role is a bit questionable though. The reason why I hide the used social sign in provider behind the constant is that the actual sign in provider (Twitter) is irrelevant, and using a constant emphasizes this.

      And for good reasons – quite often there is no big sense to declare constants for numbers that are really magic (like 0 or 1).

      Actually there is. They add meaning to the code that has none and prevent typos (like in this case). The second reason is probably the best argument for replacing magic numbers with constants in automated tests.

  • Michal Davidek Mar 30, 2016 @ 7:47

    Hi Petri, thanks for nice write up, I also like this approach to add meaning to numbers, strings and other objects reused in test cases. What I occasionally dislike on this solution (but the principle is right) is that after some time constant's class (or classes) is simply growing and often contains for example slight variations of strings with same meaning. This is hard to maintain and often leading to thinking about which constant to use in actual test case. It's the case mainly in big codebases where the refactoring (for example splitting constant class to several ones) of such things is not straightforward.

    • Petri Apr 1, 2016 @ 18:16

      Hi Michal,

      Thank you for your comment. I agree with you. I have noticed that managing constants is not nearly as easy as it sounds. The biggest problem is that it is quite easy to say that we should add meaning to magic numbers, but for some reason it is so damn hard to do so (at least properly).

  • Dinesh Mohan Jun 2, 2016 @ 15:04

    Why do we need @RunWith(MockitoJunitRunner.class). The code does run well without it. I am questioning whether we should use the Mockito test runner in the first place. Mocking and Test Running are two separate concerns - I'm not sure why we would combine them in this way. What if, for example, we come across an Assertion framework that has its own test runner. We can't use both.

    • Petri Jun 2, 2016 @ 16:06

      Hi,

      Why do we need @RunWith(MockitoJunitRunner.class)? The code does run well without it.

      I don't use the Mockito's test runner anymore because I want to use nested configuration (and I need to use a custom runner that supports this).

      When I wrote this blog post, I wanted to use the annotations that are provided by Mockito(@Mock, @Spy), and using the Mockito's test runner is the easiest way to do it (you can create the mocks by invoking the initMocks() method of the MockitoAnnotations class in your setup method). I don't use these annotations anymore because I want to create my mocks manually (i.e. by using the mock() method).

      What if, for example, we come across an Assertion framework that has its own test runner. We can’t use both.

      Well, life is full of trade-offs. Every time when you select a test runner, you basically make a choice that you value its features more than the features provided by the other test runners (this restriction is removed in JUnit 5). That is why I don't say that using Mockito's test runner is wrong. I just don't use it anymore because I value nested configure more than the features provided by the Mockito's test runner.

  • Gary Jan 3, 2019 @ 20:55

    Using spring in your unit tests is a far more dangerous practice.

    • Petri Jan 3, 2019 @ 22:13

      Actually, I don't typically load the Spring application context in my unit tests anymore (this post is over four years old), but I don't think that it always a bad idea. It always depends from the context, and it might make perfect sense if you want to write "unit tests" for legacy applications (the other option is often not to write "unit tests" at all).

      By the way, I think that the term unit test is a bit dangerous because don't seem to understand that a unit can be bigger than just a one class. That's why I think that the term fast test is a better name for tests which don't hit the database or use other external resources.

  • Anonymous... Nov 19, 2019 @ 17:18

    Hi,
    thanks for post in first place.
    I like property based testing. I use it instead of constants (if I have enough time and if given approach will pay of in terms of multiple usage). For example, instead of creating user like in your example, I would make some testing UserGenerator like this:

    User randomUserForTesting = UserGenerator
    .withProvider(SocialMediaService.TWITTER)
    .withRole(Role.USER_ROLE)
    .get();

    It will get me random user with random (through valid!) email address, name and so on. I will constrain only "fields" of generated "constant", which are relevant to test. So using this technique I can trigger more bugs than using hard-coded constants.

    What do you think about using generated "constants" instead of hard-coded ones?

    PS: In scala it is much more "user friendly" to use this technique for me and thus I have no great experience in java field.

Leave a Reply