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.
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):
import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; 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 com.googlecode.catchexception.CatchException.catchException; import static com.googlecode.catchexception.CatchException.caughtException; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Matchers.isA; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; 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 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_SocialSignInAndDuplicateEmail_ShouldThrowException() throws DuplicateEmailException { RegistrationForm registration = new RegistrationFormBuilder() .email(REGISTRATION_EMAIL_ADDRESS) .firstName(REGISTRATION_FIRST_NAME) .lastName(REGISTRATION_LAST_NAME) .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER) .build(); when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(new User()); catchException(registrationService).registerNewUserAccount(registration); assertThat(caughtException()).isExactlyInstanceOf(DuplicateEmailException.class); } @Test public void registerNewUserAccount_SocialSignInAndDuplicateEmail_ShouldNotSaveNewUserAccount() throws DuplicateEmailException { RegistrationForm registration = new RegistrationFormBuilder() .email(REGISTRATION_EMAIL_ADDRESS) .firstName(REGISTRATION_FIRST_NAME) .lastName(REGISTRATION_LAST_NAME) .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER) .build(); when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(new User()); catchException(registrationService).registerNewUserAccount(registration); verify(repository, never()).save(isA(User.class)); } @Test public void registerNewUserAccount_SocialSignInAndUniqueEmail_ShouldSaveNewUserAccountAndSetSignInProvider() throws DuplicateEmailException { RegistrationForm registration = new RegistrationFormBuilder() .email(REGISTRATION_EMAIL_ADDRESS) .firstName(REGISTRATION_FIRST_NAME) .lastName(REGISTRATION_LAST_NAME) .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER) .build(); when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(null); registrationService.registerNewUserAccount(registration); ArgumentCaptor<User> userAccountArgument = ArgumentCaptor.forClass(User.class); verify(repository, times(1)).save(userAccountArgument.capture()); User createdUserAccount = userAccountArgument.getValue(); assertThatUser(createdUserAccount) .hasEmail(REGISTRATION_EMAIL_ADDRESS) .hasFirstName(REGISTRATION_FIRST_NAME) .hasLastName(REGISTRATION_LAST_NAME) .isRegisteredUser() .isRegisteredByUsingSignInProvider(SOCIAL_SIGN_IN_PROVIDER); } @Test public void registerNewUserAccount_SocialSignInAndUniqueEmail_ShouldReturnCreatedUserAccount() throws DuplicateEmailException { RegistrationForm registration = new RegistrationFormBuilder() .email(REGISTRATION_EMAIL_ADDRESS) .firstName(REGISTRATION_FIRST_NAME) .lastName(REGISTRATION_LAST_NAME) .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER) .build(); 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); assertThatUser(createdUserAccount) .hasEmail(REGISTRATION_EMAIL_ADDRESS) .hasFirstName(REGISTRATION_FIRST_NAME) .hasLastName(REGISTRATION_LAST_NAME) .isRegisteredUser() .isRegisteredByUsingSignInProvider(SOCIAL_SIGN_IN_PROVIDER); } @Test public void registerNewUserAccount_SocialSignInAnUniqueEmail_ShouldNotCreateEncodedPasswordForUser() throws DuplicateEmailException { RegistrationForm registration = new RegistrationFormBuilder() .email(REGISTRATION_EMAIL_ADDRESS) .firstName(REGISTRATION_FIRST_NAME) .lastName(REGISTRATION_LAST_NAME) .isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER) .build(); when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(null); registrationService.registerNewUserAccount(registration); verifyZeroInteractions(passwordEncoder); } }
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?
Good points. I think the naming of test methods is one of the most difficult problems when testing the state. Sure, we can come up with ways like using the method object for testing specific tests related to scenarios for one of our state permutations but then we add complexity when we have multiple test classes for one single SUT class which makes it less readable.
Regarding your point about common config such as when testing the same setup, I really dislike duplicated code as well and I think it's fine to either just have a helper method in the test class if you need the same state in multiple tests or use the object mother pattern.
Obviously there is no perfect solution. We want our tests as clean as possible obviously but the tests classes are seldom as clean as the production code is IMHO.
I was also considering this approach, but I found a better a way before I started to use it.
I agree that there is no perfect solution. Nevertheless, it seems that I am constantly looking for ways to make my tests a bit cleaner. I guess that this is a never ending journey.
Why didn't you just put the setup code in each method in the setup() method? Would save all the copy and pasting and address your main gripes.
Because some tests might need slightly different configuration.
I tend to always put a happy path setup in the setup() method. And then tests that need a different, probably unhappy path setup can set that up in that unit test.
@Jason and @Anonymous:
Exactly.
On the other hand, moving some of the setup code to the
setup()
method would help us to clean up the source code of our test class. We could create theRegistrationForm
object in thesetup()
method because each test case uses the same information. This would definitely be an improvement, but we would still have to configure our mock objects in the test methods (because they need different configuration).The problem is that often our test classes contains unit tests for multiple methods that deal with same objects and / or mocks. If we put all initialization code to the common
setup()
method, it becomes long and (imo) messy.By the way, we can also create one test class per each tested method, but I think that there is a better way. I will describe this method in the next part of this tutorial.
@Adam
I have to admit that your approach didn't cross my mind. I want to understand your reasons for using this approach, and that is why I have a few questions to you:
setup()
method of the test class?Hey Petri, thanks for your posts. They are very useful. But now I am a bit confused about something..
Suppose that I need to test a "copyUser(User user)" method that copies all the properties from an User to a new User.
Should then I assert property by property in only one test. Or should I create a test for each property to be asserted?
Thanks.
Hi Fernando,
I have to admit I don't have a strict opinion about this. I try to ensure that a test method doesn't have too many assertions, but sometimes it doesn't make sense to write tests that have one assertion.
For example, if you want to verify that a user account is active, the odds are that you have add a few assertions into your test method.
In other words, add only one assertion per test method, but do not shoot yourself in the foot while doing it.
You could have created a helper-function right within RepositoryUserServiceTest to wrap the common stuff and then invoked that method from the setup() method or invoked it directly from specific tests that need to pass their own parameters.
Here's a possible implementation of such a helper function.
Also, sometimes people put such boilerplate stuff in a separate TestUtils class.
static Registration makeRegistration() {
RegistrationForm registration = new RegistrationFormBuilder()
.email(REGISTRATION_EMAIL_ADDRESS)
.firstName(REGISTRATION_FIRST_NAME)
.lastName(REGISTRATION_LAST_NAME)
.isSocialSignInViaSignInProvider(SOCIAL_SIGN_IN_PROVIDER)
.build();
when(repository.findByEmail(REGISTRATION_EMAIL_ADDRESS)).thenReturn(null);
return registration;
}