Writing Clean Tests – To Verify Or Not To Verify
When we write unit tests that use mock objects, we follow these steps:
- Configure the behavior of our mock objects.
- Invoke the tested method.
- Verify that the correct methods of our mock objects were invoked.
The description of the third step is actually a bit misleading, because often we end up verifying that the correct methods were invoked AND that the other methods of our mock objects were not invoked.
And everyone knows that if we want to write bug free software, we have to verify both of these things or bad things happen.
Right?
Let’s Verify Everything
Let’s start by taking a look at the implementation of a service method that is used to add new user accounts to the database.
The requirements of this service method are:
- If the email address of the registered user account is not unique, our service method must throw an exception.
- If the registered user account has a unique email address, our service method must add a new user account to the database.
- If the registered user account has a unique email address and it is created by using normal sign in, our service method must encode the user’s password before it is saved to the database.
- If the registered user account has a unique email address and it is created by using social sign in, our service method must save the used social sign in provider.
- A user account that was created by using social sign in must not have a password.
- Our service method must return the information of the created user account.
If you want learn how you can specify the requirements of a service method, you should read the following blog posts:
- From Top to Bottom: TDD for Web Applications
- From Idea to Code: The Lifecycle of Agile Specifications
This service method is implemented by following these steps:
- The service method checks that the email address given by user is not found from the database. It does this by invoking the findByEmail() method of the UserRepository interface.
- If the User object is found, the service method method throws a DuplicateEmailException.
- It creates a new User object. If the registration is made by using a normal sign in (the signInProvider property of the RegistrationForm class is not set), the service method encodes the password provided by user, and sets the encoded password to the created User object.
- The service methods saves the information of the created User object to the database and returns the saved User object.
The source code of the RepositoryUserService class looks as follows:
import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; @Service public class RepositoryUserService implements UserService { private PasswordEncoder passwordEncoder; private UserRepository repository; @Autowired public RepositoryUserService(PasswordEncoder passwordEncoder, UserRepository repository) { this.passwordEncoder = passwordEncoder; this.repository = repository; } @Transactional @Override public User registerNewUserAccount(RegistrationForm userAccountData) throws DuplicateEmailException { if (emailExist(userAccountData.getEmail())) { throw new DuplicateEmailException("The email address: " + userAccountData.getEmail() + " is already in use."); } String encodedPassword = encodePassword(userAccountData); User registered = User.getBuilder() .email(userAccountData.getEmail()) .firstName(userAccountData.getFirstName()) .lastName(userAccountData.getLastName()) .password(encodedPassword) .signInProvider(userAccountData.getSignInProvider()) .build(); return repository.save(registered); } private boolean emailExist(String email) { User user = repository.findByEmail(email); if (user != null) { return true; } return false; } private String encodePassword(RegistrationForm dto) { String encodedPassword = null; if (dto.isNormalRegistration()) { encodedPassword = passwordEncoder.encode(dto.getPassword()); } return encodedPassword; } }
If we want write unit tests which ensure that our service method is working correctly when the user is registering a new user account by using social sign in AND we want to verify every interaction between our service method and our mock objects, we have to write eight unit tests for it.
We have to ensure that:
- The service methods checks that email address is unique when a duplicate email address is given.
- The DuplicateEmailException is thrown when a duplicate email address is given.
- The service method doesn’t save a new account to the database when a duplicate email address is given.
- Our service method doesn’t encode the user’s password if a duplicate email address is given.
- Our service method checks that the email address is unique when a unique email address is given.
- When a unique email address is given, our service method creates a new User object that contains the correct information and saves the information of the created User object to the database.
- When a unique email address is given, our service method returns the information of the created user account.
- When a unique email address is given and a social sign in is used, our service method must not set the password of the created user account (or encode it).
The source code of our test class looks as follows:
import net.petrikainulainen.spring.social.signinmvc.user.dto.RegistrationForm; import net.petrikainulainen.spring.social.signinmvc.user.dto.RegistrationFormBuilder; import net.petrikainulainen.spring.social.signinmvc.user.model.SocialMediaService; import net.petrikainulainen.spring.social.signinmvc.user.model.User; import net.petrikainulainen.spring.social.signinmvc.user.repository.UserRepository; 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 net.petrikainulainen.spring.social.signinmvc.user.model.UserAssert.assertThatUser; 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_ShouldCheckThatEmailIsUnique() 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, times(1)).findByEmail(REGISTRATION_EMAIL_ADDRESS); } @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_SocialSignInAndDuplicateEmail_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(new User()); catchException(registrationService).registerNewUserAccount(registration); verifyZeroInteractions(passwordEncoder); } @Test public void registerNewUserAccount_SocialSignInAndUniqueEmail_ShouldCheckThatEmailIsUnique() 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); verify(repository, times(1)).findByEmail(REGISTRATION_EMAIL_ADDRESS); } @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); } }
These unit tests are written by following the instructions given in the previous parts of this tutorial.
That class has a lot of unit tests. Are we sure that every one of them is really necessary?
Or Maybe Not
The obvious problem is that we wrote two unit tests which both verify that our service method checks that the email address given by the user is unique. We could fix this by combining these tests into a single unit test. After all, one test should convince us that our service method verifies that the email address given by user is unique before it creates a new user account.
However, if we do this, we won’t find an answer to a much more interesting question. This question is:
Should we really verify every interaction between the tested code and our mock objects?
A few months I ago I ran into an article titled: Why Most Unit Testing is Waste by James Coplien. This article makes several good points but one of them was suits very well in this situation. James Coplien argued that we should ask one question about every test in our test suite:
If this test fails, what business requirement is compromised?
He also explains why this is such an important question:
Most of the time, the answer is, “I don’t know.” If you don’t know the value of the test, then the test theoretically could have zero business value. The test does have a cost: maintenance, computing time, administration, and so forth. That means the test could have net negative value. That is the fourth category of tests to remove.
Let’s find out what happens when we evaluate our unit tests by using this question.
Popping Up the Question
When the ask the question: “If this test fails, what business requirement is compromised?” about every unit test of our test class, we get the following answers:
- The service method checks that email address is unique when a duplicate email address is given.
- User must have a unique email address.
- The DuplicateEmailException is thrown when a duplicate email address is given.
- User must have a unique email address.
- The service method doesn’t save a new account to the database when a duplicate email address is given.
- User must have a unique email address.
- Our service method doesn’t encode the user’s password if a duplicate email address is given.
- –
- Our service method checks that the email address is unique when a unique email address is given.
- User must have a unique email address.
- When a unique email address is given, our service method creates a new User object that contains the correct information and saves the information of the created User object to the used database.
- If the registered user account has unique email address, it must be saved to the database.
- If the registered user account is created by using social sign in, our service method must save the used social sign in provider.
- When a unique email address is given, our service method returns the information of the created user account.
- Our service method must return the information of the created user account.
- When a unique email address is given and a social sign in is used, our service method must not set the password of the created user account (or encode it).
- User account that is created by using social sign in has no password.
At first it looks like our test class has only one unit test that has no business value (or which might have a negative net value). This unit test ensures that there are no interactions between our code and the PasswordEncoder mock when a user tries to create a new user account by using a duplicate email address.
It is clear that we must delete this unit test, but this is not the only unit test that must be deleted.
The Rabbit Hole Is Deeper than Expected
Earlier we noticed that our test class contains two unit tests that both verify that the findByEmail() method of the UserRepository interface is called. When we take a closer look at the implementation of the tested service method, we notice that:
- Our service method throws a DuplicateEmailException when the findByEmail() method of the UserRepository interface returns a User object.
- Our service method creates a new user account when the findByEmail() method of the UserRepository interface returns null.
The relevant part of the tested service method looks as follows:
public User registerNewUserAccount(RegistrationForm userAccountData) throws DuplicateEmailException { if (emailExist(userAccountData.getEmail())) { //If the PersonRepository returns a Person object, an exception is thrown. throw new DuplicateEmailException("The email address: " + userAccountData.getEmail() + " is already in use."); } //If the PersonRepository returns null, the execution of this method continues. } private boolean emailExist(String email) { User user = repository.findByEmail(email); if (user != null) { return true; } return false; }
I argue that we should remove both of these unit tests because of two reasons:
- As long as we have configured the PersonRepository mock correctly, we know that its findByEmail() method was called by using the correct method parameter. Although we can link these test cases to a business requirement (user’s email address must be unique), we don’t need them to verify that this business requirement isn’t compromised.
- These unit tests don’t document the API of our service method. They document its implementation. Tests like this are harmful because they litter our test suite with irrelevant tests and they make refactoring harder.
If we don’t configure our mock objects, they return “nice” values.
The Mockito FAQ states that:
In order to be transparent and unobtrusive all Mockito mocks by default return ‘nice’ values. For example: zeros, falseys, empty collections or nulls. Refer to javadocs about stubbing to see exactly what values are returned by default.
This why we should always configure the relevant mock objects! If we don’t do so, our tests might be useless.
Let’s move on and clean up this mess.
Cleaning Up the Mess
After we have removed these unit tests from our test class, its source code looks as follows:
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); } }
We removed three unit tests from our test class, and as a result, we can enjoy the following benefits:
- Our test class has less unit unit tests. This might seem like a strange benefit because often we are advised to write as many unit tests as possible. However, if we think about this, having less unit tests makes sense because we have less tests to maintain. This and the fact that each unit tests only one thing makes our code easier to maintain and refactor.
- We have improved the quality of our documentation. The removed unit tests didn’t document the public API of the tested service method. They documented its implementation. Because these tests were removed, it is easier to figure out the requirements of the tested service method.
Summary
This blog post has taught us three things:
- If we cannot identify the business requirement that is compromised if a unit test fails, we shouldn’t write that test.
- We should not write unit tests that don’t document the public API of the tested method because these tests make our code (and tests) harder to maintain and refactor.
- If we find existing unit tests that break these two rules, we should delete them.
We have achieved a lot during this tutorial. Do you think that it is possible to make these unit tests even better?
If you want to learn more about writing clean tests, read all parts of my Writing Clean Tests tutorial.
Reference: | Writing Clean Tests – To Verify Or Not To Verify from our JCG partner Petri Kainulainen at the Petri Kainulainen blog. |