3
\$\begingroup\$

This is my first attempt to do unit test for one of my controllers.

@RunWith(MockitoJUnitRunner.class)
public class AddressControllerUnitTests {

    private MockMvc mockMvc;

    @Mock
    SubmissionService submissionService;

    @Mock
    MessageSource messageSource;

    @InjectMocks
    AddressController addressController;

    @Before
    public void setup() {
        mockMvc = MockMvcBuilders.standaloneSetup(addressController).build();
    }

    @Test
    public void submitSuccess() throws Exception {
        when(submissionService.isEmailAvailable("email@email.com",0)).thenReturn(true);
        when(messageSource.getMessage("form.submitted.success", null, Locale.ENGLISH)).thenReturn("Form has been submitted successfully");
        this.mockMvc
                .perform(post("/address")
                        .param("address", "my address")
                        .param("phone", "08123456789")
                        .param("email", "email@email.com")
                        .param("userId", "14"))
                .andExpect(status().isFound())
                .andExpect(flash().attribute("message", "Form has been submitted successfully"))
                .andExpect(redirectedUrl("/address"));
    }
}

Do I really need to perform expect on the flashAttribute ? Or Should I just remove it completely ? It looks funny that I have to set

when(messageSource.getMessage("form.submitted.success", null, Locale.ENGLISH)).thenReturn("Form has been submitted successfully");

and then

.andExpect(flash().attribute("message", "Form has been submitted successfully"))
\$\endgroup\$
4
\$\begingroup\$

You've written 40 lines of code to test 2-3 assignment statements and possibly one if statement. You've also tighly coupled your test to the implementation. This does not seem like a good tradeoff to me.

Read this before you continue with the rest of the review.

Your test is very much in the solitary category and I think that's a problem.

Get rid of the service mocks. Use the real implementations of services and instead inject test doubles at the repository level. Then don't write any tests specifically for your services, unless you consider their implementation particulary tricky (and therefore need more direct feedback while implementing them).

In the article I linked Fowler did not really go into the benefits of sociable unit tests so let me expand upon that. A solitary test does only support refactorings inside the tested class. In my experience most problems with code do not come from the implementation of a single class but rather from poor choices when it comes to what constitutes a class and from the dependencies between classes. A sociable test allows you to perform much larger refactorings.

Note that a solitary test will not only not aid you in performing a larger refactoring, but it will actually get in your way as you will have to rewrite the test.

Also note that replacing remote services with test doubles is fine not only because those services are slow and introduce non-determinism, but also because refactorings across remote services are very rare (and probably shouldn't be called refactorings anymore).

Finally, I acknowledge that in this small piece of code this may not seem as a major issue. But in a large project this is the difference between an unmaintanable pile of shit and something I'd happily work on.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.