If our code has obvious faults, we are very motivated to improve it. However, at some point we decide that our code is “good enough” and move on.
Typically this happens when we think that the benefits of improving our existing code are smaller than the required work. Of course, if we underestimate our return of investment, we can make the wrong call, and it can hurt us.
This is what happened to me, and I decided to write about it so that you can avoid making the same mistake.
Writing “Good” Unit Tests
If we want to write “good” unit tests, we have to write unit tests that:
- Test only one thing. A good unit test can fail for only one reason and can assert only one thing.
- Are named properly. The name of the test method must reveal what went wrong if the test fails.
- Mock external dependencies (and state). If a unit test fails, we know exactly where the problem is.
Additional Reading:
If we write unit tests that fulfil these conditions, we will write good unit tests. Right?
I used to think so. Now I doubt it.
The Road to Hell Is Paved with Good Intentions
I have never met a software developer who decided to write crappy unit tests. If a developer is writing unit tests, it is a lot more likely that he/she wants to write good unit tests. However, this doesn’t mean that the unit tests written by that developer are good.
I wanted to write unit tests that are both easy to read and maintain. I have even written a tutorial that describes how we can write clean tests. The problem is that the advice given in this tutorial is not good enough (yet). It helps us to get started, but it doesn’t show us how deep the rabbit hole really is.
The approach that is described in my tutorial has two major problems:
Naming Standards FTW?
If we use the “naming standard” that was introduced by Roy Osherove, we notice that it is surprisingly hard to describe the state under test and the expected behavior.
This naming standard works very well when we are writing tests for simple scenarios. The problem is that real software is not simple. Typically we end up naming our test methods by using one of these two options:
First, if we try to be as specific as possible, the method names of our test methods become way too looooooooong. In the end, we have to admit that we cannot be as specific as we would want to because the method names would take too much space.
Second, if we try to keep the method names as short as possible, the method names won’t really describe the tested state and the expected behavior.
It doesn’t really matter which option we choose because we will run into the following problem anyway:
- If a test fails, the method name won’t necessarily describe want went wrong. We can solve this problem by using custom assertions, but they aren’t free.
- It is hard to get a brief overview of the scenarios that are covered by our tests.
Here are the names of the test methods that we have written during the Writing Clean Tests tutorial:
- registerNewUserAccount_SocialSignInAndDuplicateEmail_ShouldThrowException()
- registerNewUserAccount_SocialSignInAndDuplicateEmail_ShouldNotSaveNewUserAccount()
- registerNewUserAccount_SocialSignInAndUniqueEmail_ShouldSaveNewUserAccountAndSetSignInProvider()
- registerNewUserAccount_SocialSignInAndUniqueEmail_ShouldReturnCreatedUserAccount()
- registerNewUserAccount_SocialSignInAnUniqueEmail_ShouldNotCreateEncodedPasswordForUser()
These method names aren’t very long, but we must remember that these unit tests are written to test a simple registration method. When I have used this naming convention for writing automated tests for a real life software project, the longest method names have been twice as long as our longest example.
That is not very clean or readable. We can do a lot better.
There Is No Common Config
We have made our unit tests a lot better during this tutorial. Nevertheless, they still suffer from the fact that there is no “natural” way to share configuration between different unit tests.
This means that our unit tests contain a lot of duplicate code which configures our mock objects and creates other objects that are used in our unit tests.
Also, since there is no “natural” way to indicate that some constants are relevant only for specific test methods, we have to add all constants to the beginning of the test class.
The source code of our test class looks as follows (the problematic code is highlighted):
1import org.junit.Before;
2import org.junit.Test;
3import org.junit.runner.RunWith;
4import org.mockito.ArgumentCaptor;
5import org.mockito.Mock;
6import org.mockito.invocation.InvocationOnMock;
7import org.mockito.runners.MockitoJUnitRunner;
8import org.mockito.stubbing.Answer;
9import org.springframework.security.crypto.password.PasswordEncoder;
10
11import static com.googlecode.catchexception.CatchException.catchException;
12import static com.googlecode.catchexception.CatchException.caughtException;
13import static org.assertj.core.api.Assertions.assertThat;
14import static org.mockito.Matchers.isA;
15import static org.mockito.Mockito.never;
16import static org.mockito.Mockito.times;
17import static org.mockito.Mockito.verify;
18import static org.mockito.Mockito.verifyZeroInteractions;
19import static org.mockito.Mockito.when;
20
21@RunWith(MockitoJUnitRunner.class)
22public class RepositoryUserServiceTest {
23
24 private static final String REGISTRATION_EMAIL_ADDRESS = "john.smith@gmail.com";
25 private static final String REGISTRATION_FIRST_NAME = "John";
26 private static final String REGISTRATION_LAST_NAME = "Smith";
27 private static final SocialMediaService SOCIAL_SIGN_IN_PROVIDER = SocialMediaService.TWITTER;
28
29 private RepositoryUserService registrationService;
30
31 @Mock
32 private PasswordEncoder passwordEncoder;
33
34 @Mock
35 private UserRepository repository;
36
37 @Before
38 public void setUp() {
39 registrationService = new RepositoryUserService(passwordEncoder, repository);
40 }
41
42 @Test
43 public void registerNewUserAccount_SocialSignInAndDuplicateEmail_ShouldThrowException() throws DuplicateEmailException {
44 RegistrationForm registration = new RegistrationFormBuilder()
45 .email(REGISTRATION_EMAIL_ADDRESS)
46 .firstName(REGISTRATION_FIRST_NAME)
47 .lastName(REGISTRATION_LAST_NAME)
48 .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER)
49 .build();
50
51 when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(new User());
52
53 catchException(registrationService).registerNewUserAccount(registration);
54
55 assertThat(caughtException()).isExactlyInstanceOf(DuplicateEmailException.class);
56 }
57
58 @Test
59 public void registerNewUserAccount_SocialSignInAndDuplicateEmail_ShouldNotSaveNewUserAccount() throws DuplicateEmailException {
60 RegistrationForm registration = new RegistrationFormBuilder()
61 .email(REGISTRATION_EMAIL_ADDRESS)
62 .firstName(REGISTRATION_FIRST_NAME)
63 .lastName(REGISTRATION_LAST_NAME)
64 .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER)
65 .build();
66
67 when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(new User());
68
69 catchException(registrationService).registerNewUserAccount(registration);
70
71 verify(repository, never()).save(isA(User.class));
72 }
73
74 @Test
75 public void registerNewUserAccount_SocialSignInAndUniqueEmail_ShouldSaveNewUserAccountAndSetSignInProvider() throws DuplicateEmailException {
76 RegistrationForm registration = new RegistrationFormBuilder()
77 .email(REGISTRATION_EMAIL_ADDRESS)
78 .firstName(REGISTRATION_FIRST_NAME)
79 .lastName(REGISTRATION_LAST_NAME)
80 .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER)
81 .build();
82
83 when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(null);
84
85 registrationService.registerNewUserAccount(registration);
86
87 ArgumentCaptor<User> userAccountArgument = ArgumentCaptor.forClass(User.class);
88 verify(repository, times(1)).save(userAccountArgument.capture());
89
90 User createdUserAccount = userAccountArgument.getValue();
91
92 assertThatUser(createdUserAccount)
93 .hasEmail(REGISTRATION_EMAIL_ADDRESS)
94 .hasFirstName(REGISTRATION_FIRST_NAME)
95 .hasLastName(REGISTRATION_LAST_NAME)
96 .isRegisteredUser()
97 .isRegisteredByUsingSignInProvider(SOCIAL_SIGN_IN_PROVIDER);
98 }
99
100 @Test
101 public void registerNewUserAccount_SocialSignInAndUniqueEmail_ShouldReturnCreatedUserAccount() throws DuplicateEmailException {
102 RegistrationForm registration = new RegistrationFormBuilder()
103 .email(REGISTRATION_EMAIL_ADDRESS)
104 .firstName(REGISTRATION_FIRST_NAME)
105 .lastName(REGISTRATION_LAST_NAME)
106 .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER)
107 .build();
108
109 when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(null);
110
111 when(repository.save(isA(User.class))).thenAnswer(new Answer<User>() {
112 @Override
113 public User answer(InvocationOnMock invocation) throws Throwable {
114 Object[] arguments = invocation.getArguments();
115 return (User) arguments[0];
116 }
117 });
118
119 User createdUserAccount = registrationService.registerNewUserAccount(registration);
120
121 assertThatUser(createdUserAccount)
122 .hasEmail(REGISTRATION_EMAIL_ADDRESS)
123 .hasFirstName(REGISTRATION_FIRST_NAME)
124 .hasLastName(REGISTRATION_LAST_NAME)
125 .isRegisteredUser()
126 .isRegisteredByUsingSignInProvider(SOCIAL_SIGN_IN_PROVIDER);
127 }
128
129 @Test
130 public void registerNewUserAccount_SocialSignInAnUniqueEmail_ShouldNotCreateEncodedPasswordForUser() throws DuplicateEmailException {
131 RegistrationForm registration = new RegistrationFormBuilder()
132 .email(REGISTRATION_EMAIL_ADDRESS)
133 .firstName(REGISTRATION_FIRST_NAME)
134 .lastName(REGISTRATION_LAST_NAME)
135 .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER)
136 .build();
137
138 when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(null);
139
140 registrationService.registerNewUserAccount(registration);
141
142 verifyZeroInteractions(passwordEncoder);
143 }
144}
Some developers would claim that unit tests that look like the above example are clean enough. I understand this sentiment because I used to be one of them. However, these unit tests have three problems:
- The essence of the case is not as clear as it could be. Because each test method configures itself before it invokes the tested method and verifies the expected outcome, our test methods become longer than necessary. This means that we cannot just take a quick peek at a random test method and figure out what it tests.
- Writing new unit tests is slow. Because every unit test has to configure itself, adding new unit tests to our test suite is a lot slower than it could be. Another “unexpected” downside is that this kind of unit tests encourage people to practice copy-and-paste programming.
- Maintaining these unit tests is a pain in the ass. We have to make changes to every unit test if we add a new mandatory field to the registration form or the change the implementation of the registerNewUserAccount() method. These unit tests are way too brittle.
In other words, these unit tests are hard to read, hard to write, and hard to maintain. We must do a better job.
Summary
This blog post has taught us four things:
- Even though we think that we are writing good unit tests, that is not necessarily true.
- If changing existing features is slow because we have to change many unit tests, we are not writing good unit tests.
- If adding new features is slow because we have to add so much duplicate code to our unit tests, we are not writing good unit tests.
- If we cannot see what situations are covered by our unit tests, we are not writing good unit tests.
The next part of this tutorial answers to this very relevant question:
If our existing unit tests suck, how can we fix them?
