Mock Objects

This Test Smell is about a weakness in the design of tests, rather than in the code to be tested.

It may sound obvious, but you don't have to mock every class in your system.

Don't mock value objects

There isn't much point in writing mocks for simple value objects (which should be immutable anyway), just create an instance and use it. For example, in this test:

@Test public void sumsTotalRunningTime() {
  Show show = new Show();
  Video video1 = context.mock(Video.class);
  Video video2 = context.mock(Video.class);
  
  context.checking(new Expectations(){{
    one(video1).getTime(); will(returnValue(40));
    one(video2).getTime(); will(returnValue(23));
  }});
  
  show.add(video1);
  show.add(video2);
  assertEqual(63, show.getRunningTime())
}

where Video holds details of a part of a show. It's not worth creating an interface / implementation pair to control which time values are returned, just create instances with the appropriate values and use them. There are a couple of heuristics for when a class is not worth mocking. First, it has only accessors or simple methods that act on values it holds, it doesn't have any interesting behaviour. Second, you can't think of a meaningful name for the class other than VideoImpl or some such vague term.

Don't mock third-party libraries

There are two problems when mocking a third-party library. First, you can't use the tests to drive the design, the API belongs to someone else. Mock-based tests for external libraries often end up contorted and messy to get through to the functionality that you want to exercise. These tests are giving off smells that highlight designs that are inappropriate for your system but, instead of fixing the problem and simplifying the code, you end up carrying the weight of the test complexity. The second issue is that you have to be sure that the behaviour you implement in a mock (or stub) matches the external library. How difficult this is depends on the API, which has to be specified (and implemented) well enough for you to be certain that the tests are meaningful.

To develop against an external API, first TDD your code to define interfaces for the services that your application needs in terms of your domain. Then write a thin layer using the library to implement these interfaces, with focussed integration tests to confirm your assumptions about how it works. How to make integration tests work effectively will depend on your environment, but you will need to do it anyway so you might as well start early and get the benefit. There will be relatively few integration tests, compared to the number of unit tests, so they should not get in the way of the build even if they're not as fast as the in-memory tests.

There are some exceptions when mocking third-party libraries can be helpful. You might use mocks to simulate behaviour that is hard to trigger with the real library, such as throwing exceptions. Similarly, you might use mocks to test a sequence of calls, for example making sure that a transaction is rolled back if there's a failure. There should not be many of these in a test suite.

Labels: design, listening to the tests, tip

Inspired by TestDox I discovered a trick when using Eclipse 3.2.2 with JUnit4. If you lay out your test methods like this

When you fold the test class, it looks like this

If you've written your method names appropriately, the result is a compact description of the features of the class under test.

Labels: eclipse, ide, junit4, tip

We often see tests that are hard to understand because they drive code that calls objects returned from other objects. Here's an example of code that calls a chain of objects.

public void allUnresolvedIssues(IssueHandler handler) throws Failure {
 TicketManager ticketManager = connections.getTicketManager();

 ticketManager.lock();
 try {
   IssueManager  issueManager = ticketManager.getIssueManager();
   Iterator issues = issueManager.allIssuesIterator();

   while (issues.hasNext()) {
     Issue issue = issues.next();
  
     if (issue.isUnresolved()) {
       handler.accept(issue);
     }
   }
 } finally {
   ticketManager.unlock();
 }
}

This is a minor offender, but even so it gets a list of Issues from an IssueManager, which it gets from a TicketManager, which it gets from a Connection. The important part of the method is the filtering of the issues, but to get there it has to navigate three objects, including locking and unlocking the TicketManager. We might have to do something like this to test it.

public void testFiltersUnresolvedIssuesFromIssueManager() throws Failure {
 final ArrayList allIssues = new ArrayList() {{
   add(issue1Unresolved);
   add(issue2);
   add(issue3Unresolved);
 }};

 expects(new InAnyOrder() {{
   allowing(connections).getTicketManager(); will(returnValue(ticketManager));

   expects(new InThisOrder() {{
     one(ticketManager).lock();
     one(ticketManager).getIssueManager(); will(returnValue(issueManager));
     one(issueManager).allIssuesIterator(); will(returnValue(allIssues.iterator()));
  
     one(issueHandler).accept(issue1Unresolved);
     one(issueHandler).accept(issue3Unresolved);
  
     one(ticketManager).unlock();
   }});
 }});

 moreThanImmediateNeighbours.allUnresolvedIssues(issueHandler);
}

I find this confusing to read. First, too much of the test code is for setting up state to get to the interesting feature. Second, the test is exercising several things at once: locking and unlocking the TicketManager, querying the issues within the lock, and filtering the issues. If I want to test more conditions, such as when there are no issues or when something fails, I'll have to duplicate lots of irrelevant test code. I can extract that duplication into setup methods, but I think that's missing what the test is telling me. Another problem is that the test is, more or less, just reproducing the logic of the code, which means it's not adding much value and also makes it brittle when I want to refactor later—especially if I have many similar tests to change. This is a common reason given for not using Mock Objects and a plausible one when dealing with code like this. Let's see if I can break things up a bit. My first thought is that I can separate out everything to do with the TicketManager by extracting a method.

  public void allUnresolvedIssues(IssueHandler handler) throws Failure {
   TicketManager ticketManager = connections.getTicketManager();
  
   ticketManager.lock();
   try {
     retrieveUnresolvedIssues(ticketManager.getIssueManager(), handler);
   } finally {
     ticketManager.unlock();
   }
 }

 private void retrieveUnresolvedIssues(IssueManager issueManager, IssueHandler handler) {
   Iterator issues = issueManager.allIssuesIterator();
  
   while (issues.hasNext()) {
     Issue issue = issues.next();
    
     if (issue.isUnresolved()) {
       handler.accept(issue);
     }
   }
 }

Now I can see a bit more clearly the two activities going on here: getting hold of the issue manager, and retrieving issues from it. I can break these up into different, focussed objects. I could start from the current design and refactor, but I want to begin again to show the design process...

Labels: design, tip