Test Argument

2 Comments October 2, 2014

So this post is more of an invitation for comment than anything else; a recent, and somewhat heated, difference of opinion arose during a code review. The specific argument was around a unit test that exercised a block of code that looked something like this simplified class:

package com.example;

...

public class FooBarPopulator implements Populator<FooModel, BarData>
{

    @Override
    public void populate(FooModel source, BarData target) throws ConversionException
    {
        target.setUid(source.getUid());
    }
}

I'd ended up with the following (again, slightly simplified) test to check the populate() method:

package com.example;

...

@RunWith(MockitoJUnitRunner.class)
public class SiteLinkPopulatorTest
{
    public static final String UID = "uid-123456";

    private final FooBarPopulator sut = new FooBarPopulator();

    @Mock
    private FooModel fooModel;

    @Before
    public void setUp() throws Exception
    {
        when(fooModel.getUid()).thenReturn(UID);
    }

    @Test
    public void testPopulate() throws Exception
    {
        final BarData barData = mock(BarData.class);

        sut.populate(fooModel, barData);

        verify(barData, times(1)).setUid(UID);
        verifyNoMoreInteractions(barData);
    }
}

Two mock objects are created, one to simulate FooModel and return values on request with the other to simulate BarData so that the setUid interaction could be verified. The only concrete class instance in the test is the system under test, FooBarPopulator.

The argument arose because the other party was adamant the test should look like something like this:

    ...

    @Test
    public void testPopulate() throws Exception
    {
        final FooModel fooModel = new FooModel();
        fooModel.setUid(UID);

        final BarData barData = new BarData();

        sut.populate(fooModel, barData);

        assertThat(barData.getUid(), equals(UID));
    }   

My assertion is that this is wrong, and for the following reasons.

The original test is asserting exactly what the class is doing; a value is being taken from fooModel and passed to barData. It is also important to remember that fooModel and especially barData are not owned by the populate method. Both objects are passed by reference to the populate method.

As only setters are accessed on fooModel and getters used on barData these are the only methods that should be tested, and therefore in this case confirmed by a verify() assertion. The other party however was arguing that as the intention of the populator is to set values on a POJO, the method should be tested by asserting the value returned by barData.getUid(). This to me seems completely wrong; from the perspective of the unit test, the fact that barData.getUid() returns the same value as set by barData.setUid("foo") is a coincidence and shouldn't be relied on.

In the end, we ended up at an impasse and agreed to disagree; the code got accepted, but it felt like a hollow victory. I'd be very interested in other opinions on this, so feel free to chime in below...


2 Comments

  • Gravatar Image
    Massimo Chericoni February 10, 2015 1:48 PM

    I share the point of view of the other party and I would prefer the test without the mocks.
    First of all, we shouldn't abuse mocks and use them even when it's perfectly possible to use the real object. The second reason is that the purpose of the test is to check that the Populator is correctly populating the barData instance, therefore we call the 'populate' method and verify the expectation that barData contains the given UID when we retrieve it through its getter.
    The test without mocks does this and it makes sense to me.
    The test with mocks, on the other hand, checks that there is one invocation of the setter method. This is an implementation detail of the 'populate' method. We're not interested in what this method does, it's a detail we don't need to know: we need to know that the 'populate' did it job correctly i.e. that when I next call the getter, I get the correct result.

  • Gravatar Image
    Ed Courtenay February 11, 2015 8:09 AM

    Thanks for the comment Massimo.

    My point is that the populator under test does not own either the source object (FooModel) or the destination object (BarData), and because of that you have no information on how FooModel or BarData have been implemented.

    The populator only uses the getter on the source object and the setter on the target object, and therefore these should be the only things tested. What's to say that Bar in this case represents a POJO? Why couldn't it be an interface?