4
\$\begingroup\$

We've been reviewing some legacy code and have found differences in the preferred approach to how to write tests using Mockito.

Both the following tests pass but which one is better and why?

@Test
public void verifyObjectSaved() throws PersistQueueException {
    final UserTokenVo userTokenVo = new UserTokenVo();

    String token = vodafoneSubscriptionService.processTokenRequest(userTokenVo);

    Mockito.verify(userTokenDao).savePayload(isA(String.class), eq(userTokenVo));

    assertTrue(StringUtils.isNotBlank(token));
}

and

@Test
public void verifyObjectSaved() throws PersistQueueException {
    final UserTokenVo userTokenVo = new UserTokenVo();

    doAnswer(new Answer() {
        public Object answer(InvocationOnMock invocation) {
            return "completed";
        }
    }).when(userTokenDao).savePayload(any(String.class), eq(userTokenVo));

    String token = vodafoneSubscriptionService.processTokenRequest(userTokenVo);

    assertTrue(StringUtils.isNotBlank(token));

    verify(userTokenDao, times(1)).savePayload(any(String.class), eq(userTokenVo));
}

Here's the production code it tests — it's not very complicated:

public String processTokenRequest(UserTokenVo userTokenVo) {
    String token;

    try {
        token = new String(TokenFactory.getToken()); 
        if (token != null) {
            userTokenDao.savePayload(token, userTokenVo);
        }
    } catch (Exception e) {
        LOGGER.error("failed to save token");
        token = null;
    }
    return token;
}
\$\endgroup\$
  • \$\begingroup\$ There are actually problems with the code itself rather than the tests :p \$\endgroup\$ – fge Apr 17 '15 at 11:21
  • \$\begingroup\$ @fge could you elaborate on that in an answer please? \$\endgroup\$ – ferada Apr 17 '15 at 11:30
  • \$\begingroup\$ @ferada well, I could, but wouldn't that be offtopic somewhat? Unless the OP wants it... \$\endgroup\$ – fge Apr 17 '15 at 15:26
2
\$\begingroup\$

Looking at your production code, my positive test case would check the getToken() and savePayload() methods were invoked; and the same string returned from getToken() was also returned by processTokenRequest().

Some points:

  • I wouldn't use the any matcher with verify(). Explicitly state what you want.
  • No need to return value from a method when you don't use the returned value i.e. doAnswer for savePayload() is unnecessary.
  • As mentioned, times(1) is the default for verify() and used in your first test.
  • Tests shouldn't throw exceptions.
  • Static imports often make short code easy to read.
  • Is isNotBlank() enough?
\$\endgroup\$
1
\$\begingroup\$

I'd say a combination of both would be best.

So first of all I'd favour less code, so static import all the things.

The times(1) is arguably good, I think the notes on the Mockito docs were saying that it depends on whether you really want to make sure that you only call it once, or whether you just want to test that "it happened" at all. Also, as @fge said, times(1) is the default, so leave it out.

any(String.class) is better than isA, because it clarifies intent; since String is final, you can't get a subclass there, so isA is in a sense too broad. Then again, anyString would be best, since it is shorter than the alternative and also checks for null.

The doAnswer is probably not necessary? I mean if you return a value, I'd probably check that it's passed through unharmed as well. Also, I think usually you don't need doAnswer, but you can use a simpler way of returning the value; doAnswer would be needed for throwing exceptions etc.

Why do the tests have a throws clause though?

\$\endgroup\$
  • 1
    \$\begingroup\$ times(1) is actually the default, I believe \$\endgroup\$ – fge Apr 17 '15 at 11:20
  • 1
    \$\begingroup\$ "any(String.class) is better than isA" Sure, but why? State your reason to OP. \$\endgroup\$ – abuzittin gillifirca Apr 20 '15 at 9:30
  • \$\begingroup\$ @abuzittingillifirca thanks, I've added a clarification. \$\endgroup\$ – ferada Apr 20 '15 at 21:43

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.