donderdag 17 januari 2013

When mocking builders... (using Mockito)

Most of you already know I'm not really keen about mock testing. It gets even worse when mocking builders. As a matter of fact, I'm not really crazy about builders either (so now I find myself looking for a better way to use two of the things I always try to avoid). At least not in production code. Builders in tests are great for quickly creating full-fledged domain objects. For production code however, always ask yourself whether you wouldn't be better of using a factory.

Anyway, in case you are using builders and you want to mock them out, don't do it this way (my example is overly simplistic, but that's not the point here):

  @Test
  public void createLogbookRecordIsNotReallyTested() {
    LogbookRecordBuilder builderMock = mock(LogbookRecordBuilder.class);
    when(builderMock.withType(any(LogbookRecordType.class)))
      .thenReturn(builderMock);
    when(builderMock.withPersonName(anyString()))
      .thenReturn(builderMock);
    when(builderMock.withAccountNumber(anyString()))
      .thenReturn(builderMock);
    when(builderMock.build()).thenReturn(LOG_RECORD);

    LogbookRecord result = client.createLogbookRecord(builderMock);
    assertThat(result).isEqualTo(LOG_RECORD);
  }

Of course we were testing this immensely vital production code:

  @Test
  public LogbookRecord createLogbookRecord(LogbookRecordBuilder builder) {
    return builder
      .withType(LogbookRecordType.DEPOSIT)
      .withPersonName(personName)
      .withAccountNumber(accountNumber)
      .build();
  }

What's wrong with the test? Well, it actually tests nothing. Ok, i'm exagerating a bit, it just tests the build method was called and the result is returned.
So you could change your production code into this:

  public LogbookRecord createLogbookRecord(LogbookRecordBuilder builder) {
    return builder.build();
  }

and your test would still work just fine. Reason is of course that you are stubbing your with-calls, the test is not forcing them to be called.
To fix that, we'll add verify methods to check whether the right methods are called on your builder:

  @Test
  public void createLogbookRecordIsTestedWithStubbingAndInOrderVerifications() {
    LogbookRecordBuilder builderMock = mock(LogbookRecordBuilder.class);
    when(builderMock.withType(any(LogbookRecordType.class)))
      .thenReturn(builderMock);
    when(builderMock.withPersonName(anyString()))
      .thenReturn(builderMock);
    when(builderMock.withAccountNumber(anyString()))
      .thenReturn(builderMock);
    when(builderMock.build()).thenReturn(LOG_RECORD);

    LogbookRecord result = client.createLogbookRecord(builderMock);
    assertThat(result).isEqualTo(LOG_RECORD);

    InOrder inOrder = Mockito.inOrder(builderMock);
    inOrder.verify(builderMock).withType(LogbookRecordType.DEPOSIT);
    inOrder.verify(builderMock).withPersonName(PERSONNAME);
    inOrder.verify(builderMock).withAccountNumber(ACCOUNTNUMBER);
    inOrder.verify(builderMock).build();
    verifyNoMoreInteractions(builderMock);
  }

I don't really like this approach because of the DRY principle (you're stubbing and verifying the same methods). Did you also notice the inOrder checking of the method calls? I'm not really proud about this, but that way I make sure the build method was called last.

One solution to stop repeating yourself might be using a default answer (ReturnSelfWhenTypesMatchAnswer) for your mocked builder which just returns the mock itself when the method's result type is some supertype of your mocked builder (so all your with-methods will return the mocked builder). That way your test looks like this:

  @Test
  public void createLogbookRecordIsTestedUsingDefaultAnswer() {
    LogbookRecordBuilder builderMock = 
      mock(LogbookRecordBuilder.class, new ReturnSelfWhenTypesMatchAnswer());
    when(builderMock.build()).thenReturn(LOG_RECORD);

    LogbookRecord result = client.createLogbookRecord(builderMock);
    assertThat(result).isEqualTo(LOG_RECORD);

    InOrder inOrder = Mockito.inOrder(builderMock);
    inOrder.verify(builderMock).withType(LogbookRecordType.DEPOSIT);
    inOrder.verify(builderMock).withPersonName(PERSONNAME);
    inOrder.verify(builderMock).withAccountNumber(ACCOUNTNUMBER);
    inOrder.verify(builderMock).build();
    verifyNoMoreInteractions(builderMock);
  }

The implementation of ReturnSelfWhenTypesMatchAnswer is quite simple:

public class ReturnSelfWhenTypesMatchAnswer implements Answer<Object> {

  @Override
  public Object answer(InvocationOnMock invocation) throws Throwable {
    Class<?> returnType = invocation.getMethod().getReturnType();
    if (returnType.isInstance(invocation.getMock())) {
      return invocation.getMock();
    }
    return null;
  }
}

Question I ask myself: why didn't anybody test the code like this?

  @Test
  public void createLogbookRecordIsTestedTheRightWayWithoutMocks() {
    LogbookRecordBuilder builder = new LogbookRecordBuilder();

    LogbookRecord result = client.createLogbookRecord(builder);

    assertThat(result.getType()).isEqualTo(LogbookRecordType.DEPOSIT);
    assertThat(result.getAccountNumber()).isEqualTo(ACCOUNTNUMBER);
    assertThat(result.getPersonName()).isEqualTo(PERSONNAME);
  }

For me, the only reasonable answer to the above question would be: because our builder is doing incredibly complex stuff which we don't want to re-test. Did you also notice the last test is the shortest and most readable version of all without copy-pasting any production code?

Ok, so I resulted back in discouraging you from writing mock tests. My original intention however was to show that using a default answer for mocks can sometimes be a more elegant solution than stubbing lots of stuff.

1 opmerking:

  1. I do like your solution of configuring a default answer for builders.

    However, every time I see a builder (be it in production code or test code), I ask myself why it's there in the first place. Sometimes I hear the argument that builders help avoiding long parameter lists. But do they really? In my opinion, a builder often hides the fact that the object being build has too many dependencies. Sure, using a builder is more readable than using a constructor with many unnamed parameters (Smalltalk, anyone?). But the dependencies are still there, and now you're not encouraged anymore to reduce the number of dependencies and splitting out smaller objects.
    The same holds when using builders for setting up complex test scenarios. A builder might increase readability superficially, by reducing the lines needed to set up objects in a desired state. But actually, these builders hide a lot of complexity and reduce the pressure that should lead you to smaller and less coupled objects. Again, a builder might look like a good solution, but it conceals a bigger problem: a large object with too many dependencies.

    Now, there are times when a builder is just the right tool for the job. When a large constellation of objects with many interdependencies needs to be built, a builder can express more clearly what those relations are. For example, a composite with a deep hierarchy can be built very expressive with a builder.
    When used in this way, a builder is easily tested without mocks of any kind. Is a test at that level even needed, as it will only duplicate what has already been stated in code?

    BeantwoordenVerwijderen