Tuesday, April 29, 2014

Mock Objects are a Testing Smell

I like mock objects, at least in principle. I think that well-designed object-oriented programs consist of collaborating objects, and mocks do a good job of capturing that collaboration: the general idea is “push on A and verify something happens at B.”

But I feel uncomfortable when I see tests that use multiple mocks, or set many expectations. These tests have moved beyond testing behavior to testing implementation, and are tightly coupled to the mainline code. These tests are brittle, breaking at the slightest change to the mainline code. And on the flip side, the mainline code becomes much harder to refactor, because changes will break the tests.

This is a particular problem for service-level objects. To see what I mean, consider a simple service to manage bank accounts:

public interface BankService {
    BigDecimal getBalance(String accountId) throws BankException;
    List getTransactions(String accountId) throws BankException;

    void deposit(String accountId, BigDecimal amount) throws BankException;
    void withdraw(String accountId, BigDecimal amount) throws BankException;
}

To support this service, we need a data-access object, which can be equally simple:

public interface AccountRepository {
    Account find(String accountId);

    void update(Account account);
}

This seems to be a good use for mock objects: to test deposit(), you create a mock for AccountRepository and set expectations on find() and update(). Actually making this work, however, is not quite as easy as describing it. Here's an implementation using EasyMock:

public class TestAccountOperations {
    private static class AccountBalanceMatcher
    implements IArgumentMatcher {
        BigDecimal expected;

        public AccountBalanceMatcher(BigDecimal expected) {
            this.expected = expected;
        }

        @Override
        public boolean matches(Object argument) {
            Account account = (Account)argument;
            return account.getBalance().equals(expected);
        }

        @Override
        public void appendTo(StringBuffer sb) {
            sb.append(expected);
        }
    }

    public static Account eqBalance(BigDecimal expected) {
        EasyMock.reportMatcher(new AccountBalanceMatcher(expected));
        return null;
    }

    private final String accountId = "test-account-1234";
    
    private Account account = new Account(accountId);
    private AccountRepository mock = createMock(AccountRepository.class);
    private BankService service = new BankServiceImpl(mock);

    @Test
    public void testDepositHappyPath() throws Exception {
        final BigDecimal initialBalance = new BigDecimal("300.00");
        final BigDecimal depositAmount = new BigDecimal("200.00");
        final BigDecimal expectedBalance = initialBalance.add(depositAmount);
        
        account.setBalance(initialBalance);

        expect(mockAccountRepo.find(accountId)).andReturn(account);
        mockAccountRepo.update(EasyMockSupport.eqBalance(expectedBalance));
        replay(mockAccountRepo);

        service.deposit(accountId, depositAmount);
        
        verify(mockAccountRepo);
    }

That's a lot of code. More than half of it is infrastructure, needed so that EasyMock can validate the account balance in the call to update(). It is, however, relatively readable, and the mock object allows us to easily test “sad path” cases such as an invalid account number simply by replacing andReturn() with andThrow().

However, this test is missing something important: it doesn't validate that we update the transaction log. How do you discover this? You can't rely on a coverage report, because you're not actually exercising mainline code. You just have to know.

Fortunately, it's easy to add another mock (and it's also time to refactor the EasyMock extensions into their own class):

public class TestAccountOperations {
    private final String accountId = "test-account-1234";
    
    private Account account = new Account(accountId);
    private AccountRepository mockAccountRepo = createMock(AccountRepository.class);
    private TransactionRepository mockTxRepo = createMock(TransactionRepository.class);
    private BankService service = new BankServiceImpl(mockAccountRepo, mockTxRepo);

    @Test
    public void testDepositHappyPath() throws Exception {
        final BigDecimal initialBalance = new BigDecimal("300.00");
        final BigDecimal depositAmount = new BigDecimal("200.00");
        final BigDecimal expectedBalance = initialBalance.add(depositAmount);
        final Transaction expectedTx = new Transaction(accountId, TransactionType.DEPOSIT, depositAmount);
        
        account.setBalance(initialBalance);

        expect(mockAccountRepo.find(accountId)).andReturn(account);
        mockAccountRepo.update(EasyMockSupport.eqBalance(expectedBalance));
        mockTxRepo.store(EasyMockSupport.eqTransaction(expectedTx));
        replay(mockAccountRepo, mockTxRepo);

        service.deposit(accountId, depositAmount);
        
        verify(mockAccountRepo, mockTxRepo);
    }
}

We're done, although to my eye it's cluttered: you don't immediately see what is being tested, because you're distracted by how it's being tested.

Time marches on, and another department introduces a fraud-prevention service. We need to integrate with it, and verify that we reject transactions when the fraud service throws an exception. This is a case where mock-object testing is at its best: rather than understand the conditions that trigger a fraud exception, we just need to tell the mock to create one:

@Test(expected=BankException.class)
public void testWithdrawalFraudFailure() throws Exception {
    final BigDecimal initialBalance = new BigDecimal("300.00");
    final BigDecimal withdrawalAmount = new BigDecimal("200.00");
    
    account.setBalance(initialBalance);
    
    expect(mockAccountRepo.find(accountId)).andReturn(account);
    mockFraudService.check(accountId, FraudService.TransactionType.WITHDRAWAL, withdrawalAmount);
    expectLastCall().andThrow(new FraudException(accountId, "test of fraudulent transaction"));
    replay(mockAccountRepo, mockTxRepo, mockFraudService);

    service.withdraw(accountId, withdrawalAmount);
    
    verify(mockAccountRepo, mockTxRepo, mockFraudService);
}

As I said, this is one of the true benefits of mocks, but it comes at a cost: we need to update all of our tests to include an expectation on the service. When version 2.0 of the fraud service gets released, with an all-new API that doesn't use exceptions, the update to our mainline code is simple; our test library, now consisting of dozens of tests, not so much.

So here you are, spending hours updating your tests, while your manager is wondering why the team “wastes so much effort” on something that “clearly adds little value.” Perhaps you start to feel the same way, especially after you've spent a few hours trying to understand a test that hides its purpose within a mass of expectations.

But what's the alternative? Do you throw away the tests and hope that everything just works?

In my opinion, the only valid test for a service-layer object is an integration test, backed by a real (albeit preferably in-memory) database. Once you get rid of the mocks, not only do your tests become less brittle, but they clearly demonstrate the behavior that you're trying to test.

@Test
public void testDepositHappyPath() throws Exception {
    final BigDecimal initialBalance = new BigDecimal("300.00");
    final BigDecimal depositAmount = new BigDecimal("200.00");

    final BigDecimal expectedBalance = initialBalance.add(depositAmount);
    final Transaction expectedTx = new Transaction(accountId, TransactionType.DEPOSIT, depositAmount);
    
    // create account as part of @Before
    List initialTxs = service.getTransactions(accountId);
    service.deposit(accountId, depositAmount);
    List finalTxs = service.getTransactions(accountId);
     
    assertEquals("balance reflects deposit", expectedBalance, service.getBalance(accountId));
    assertEquals("transaction added by deposit", initialTxs.size() + 1, finalTxs.size());
    assertEquals("correct transaction", expectedTx, finalTxs.get(finalTxs.size() - 1));
}

That's not to say that you need to wire together the entire system for every test. As I said above, the fraud service is a good use for a stand-in. But that stand-in doesn't need to be an expectation-setting mock, it can be a behavior-delivering stub.