Skip to content

Reality Check: Legacy Code Kata with Mockito

by on August 23, 2013

I recently blogged about the Legacy Code Kata at Softwerkskammer Hamburg Dojo. Now it’s about time that the Kata passes the reality check. So I took some legacy code colleagues cursed about and started to write some tests strongly relying on mocking framework Mockito.

Mockito is great, …

Unlike the Coding Kata I relied on a mocking framework this time. It’s my favorite one: Mockito. So simply I mocked a page and some navigation object like this:

page = Mockito.mock(Page.class);
navigation = Mockito.mock(Navigation.class);
Mockito.when(page.getNavigation()).thenReturn(navigation);

This mocking was required deep in the callstack of the class under test.

but… The Problem

If this code will ever be refactored (actually I know that it soon will be) it might happen that the mocked call is actually not called anymore which makes the assumptions invalid and thus the result of the test is completely meaningless.

The Recommended Solution

If you are ever in need to mock some method calls you should verify that they actually got called. As this is a check of the assumptions made I recommend to place this in front of your test-assertions (as they are meaningless when these checks fail):

Mockito.verify(page, VerificationModeFactory.times(1))
  .getNavigation();

If this fails it is most likely that you have to adopt your test rather than adopt your code.

Note: times(1) could have been skipped here as this is the default. For this example atLeastOnce() would have been more appropriate as it is just important that the mocking got used – not how many times.

Side Note: AssertionError

What is bad (at least in this use case) is that Mockito.verify() raises an assertion error if the verification fails. But an assertion error is perceived as signal that something is wrong with the code under test. It would be better to have a “normal” exception here which would mark the test as “error” rather than “failure” which is more appropriate here as the test did not fail – it just did not match the assumptions.

Convenience

If you have a huge set of stubbed method calls as above you can easily convert a copy of it to a verification using this regular expression replacement:

From: when\(([^.]+)\.(.*(?=\)\.then)).*
To: verify($1, atLeastOnce()).$2;

The above example assumes static import for atLeastOnce().

From → Coding Dojo, Testing

2 Comments
  1. Your recommendation violates the DRY principle and the additional verification is not needed most of the time as experience shows. This is also what Mockito’s Javadoc [1] says

    > Although it is possible to verify a stubbed invocation, usually it’s just redundant
    > If your code cares what get(0) returns then something else breaks (often before even verify() gets executed).
    > If your code doesn’t care what get(0) returns then it should not be stubbed. Not convinced? See [2]

    I try to use verification only to ensure correctness of the code under test, for example to validate that the code invokes some callback methods. If used this way, the thrown AssertionError also makes sense.

    There are cases where it’s useful to verify stubbed invocations, but in general you should avoid that pattern.

    [1] http://docs.mockito.googlecode.com/hg/latest/org/mockito/Mockito.html#2
    [2] http://monkeyisland.pl/2008/04/26/asking-and-telling/

    • Yes, you are right, the DRY principle is clearly violated. And I would not recommend it for any other code than legacy code.

      The verification got added for another principle in testing: Fail Fast – which in this case means to fail if the assumptions of the test are no longer valid.

      Perhaps it’s a special case as I can foresee the future: a commit which not only refactors but completely rewrites the logic executed behind an API. This commit had to be reverted before a release because it contained a bug. Having this code I know that the tests will produce wrong results (not even sure if wrong failure or wrong pass) if the commit is re-established again.

      What was interesting about this reverted commit: Without having it at hand I would have written completely different tests – perhaps not even using the verification block in the test.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

Join 274 other followers