Sometimes we find it difficult to write a test for some functionality we want to add to our code. In our experience, this is usually because our design can be improved, perhaps because the class is too tightly coupled to its environment or does not implement a clear abstraction. We try to use this as a opportunity to improve our code, rather than working around the design by introducing ever more sophisticated tools. We find that the qualities that make an object easy to test are those that make our code flexible and able to accommodate change.

The trick is to let our tests drive our design (that's a hint as to why it's called Test-Driven Development). TDD is about testing code, that is about verifying its externally visible qualities such as reliability, performance, and accuracy. TDD is also about feedback on the code's internal qualities such as the coupling and cohesion of its classes, dependencies that are explicit or hidden, and effective information hiding; these are the qualities that will keep the code maintainable. With practice, we’ve become more sensitive to the rough edges in our tests, so we can use them for rapid feedback about the design. Now when we find a feature that’s difficult to test we don’t just ask ourselves how to test it, but also why is it difficult to test.

In this chapter, we look at some common “Test Smells” that we’ve encountered and discuss what they might imply about design.

One approach to reducing complexity in code is to make commonly useful objects accessible through a global name, usually implemented with a Singleton to fool oneself into thinking that its not really a global variable. The idea is that any code that needs access to a feature can just refer to it by its global name instead of receiving it as a parameter. Here's a common example:

Date now = new Date();

Under the covers, the constructor calls the singleton System and sets the new instance to the current time using System.currentTimeMillis(). This is a convenient technique, but it comes at a cost. Let's say we want to write a test like this:

@Test public void 
rejectsRequestsNotWithinTheSameDay() {
  receiver.acceptRequest(FIRST_REQUEST);
  // the next day
  assertFalse("too late now", receiver.acceptRequest(SECOND_REQUEST));
}

The implementation looks like this:

public boolean acceptRequest(Request request) {
  final Date now = new Date();
  if (dateOfFirstRequest == null) {
    dateOfFirstRequest = now;
   } else if (firstDateIsDifferentFrom(now)) {
    return false;
  }
  // process the request
  return true;
}

where dateOfFirstRequest is a field and firstDateIsDifferentFrom is a helper method that hides the unpleasantness of working with the Java date library.

To test this timeout we must either make the test wait overnight or do something clever, perhaps with Aspects or byte-code manipulation, to intercept the constructor and return suitable Date values for the test. This difficulty in testing is a hint that we should change the code. To make the test easier, we need to control how Date objects are created, so we introduce a Clock and pass it into the Receiver. If we stub the Clock, the test might look like this:

@Test public void 
rejectsRequestsNotWithinTheSameDay() {
  Receiver receiver = new Receiver(stubClock);
  stubClock.setNextDate(TODAY);
  receiver.acceptRequest(FIRST_REQUEST);

  stubClock.setNextDate(TOMORROW);
  assertFalse("too late now", receiver.acceptRequest(SECOND_REQUEST));
}

and the implementation like this:

public boolean acceptRequest(Request request) {
  final Date now = clock.now();
  if (dateOfFirstRequest == null) {
   dateOfFirstRequest = now;
  } else if (firstDateIsDifferentFrom(now)) {
   return false;
  }
  // process the request
  return true;
}

Now we can test the Receiver without any special tricks. More importantly, however, we've made it obvious that a Receiver is dependent on time, we can't even create one without a Clock. One could argue that this is breaking encapsulation by exposing the internals of a Receiver, we should be able to just create an instance and not worry, but we want to know about this dependency—especially when the service is rolled out across timezones, and New York and London start complaining about different results.

firstDateIsDifferentFrom(now)

@Test public void 
rejectsRequestsNotWithinTheSameDay() {
  Receiver receiver = new Receiver(clock);
  context.checking(new Expectations() {{
   allowing(clock).now(); will(returnValue(NOW));
   one(clock).dayHasChangedFrom(NOW); will(returnValue(false));
  }});

  receiver.acceptRequest(FIRST_REQUEST);
  assertFalse("too late now", receiver.acceptRequest(SECOND_REQUEST));
}

public boolean acceptRequest(Request request) {
  if (dateOfFirstRequest == null) {
   dateOfFirstRequest = clock.now();
  } else if (clock.dayHasChangedFrom(dateOfFirstRequest)) {
   return false;
  }
  // process the request
  return true;
}

@Test public void 
rsejectsRequestsOutsideAllowedPeriod() {
  Receiver receiver = new Receiver(sameDayChecker);
  context.checking(new Expectations() {{
    allowing(sameDayChecker).hasExpired(); will(returnValue(false));
  }});

  assertFalse("too late now", receiver.acceptRequest(REQUEST));
}

public boolean acceptRequest(Request request) {
  if (sameDayChecker.hasExpired()) {
   return false;
  }
  // process the request
  return true;
}

We have a more contentious example of working with objects that are hard to replace: logging. Take a look at these two lines of code:

log.error("Lost touch with Reality after " + timeout + "seconds);
log.trace("Distance travelled in the wilderness: " + distance);

These are two separate features that happen to share an implementation. Let us explain.

Given this distinction, we should consider using different techniques to develop these two type of logging. Support logging should be Test-Driven from someone's requirements, such as auditing or responsiveness to failure. The tests will make sure that we've thought about what each message is for and that it works. The tests will also protect us from breaking any tools and scripts that other people write to analyse these log messages. Diagnostic logging, on the other hand, is driven by the programmers' need for fine-grained tracking of what's happening in the system. It's scaffolding so it probably doesn't need to be Test-Driven and the messages might not need to be as consistent as those for support logs. These messages are not to be used in production, right?

Location location = tracker.getCurrentLocation();
for (Filter filter : filters) {
  filter.selectFor(location);
  if (logger.isInfoEnabled()) {
    logger.info("Filter " + filter.getName() + ", " + filter.getDate()
                 + " selected for " + location.getName() 
                 + ", is current: " + tracker.isCurrent(location));
  }
}

Location location = tracker.getCurrentLocation();
for (Filter filter : filters) {
  filter.selectFor(location);
  support.notifyFiltering(tracker, location, filter);}

This Test Smell is more usually about weaknesses in the design of the tests, rather than in the code to be tested. It may sound obvious, but not every class in a system has to be mocked.

@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())
}

Don't mock third-party libraries

There are two problems when mocking a third-party library. First, we 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 we want to exercise. These tests are giving off smells that highlight designs that are inappropriate for our system but, instead of fixing the problem and simplifying the code, we end up carrying the weight of the test complexity. The second issue is that we have to be sure that the behaviour we 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 us to be certain that the tests are meaningful.

To develop against an external API, we first TDD the code to define interfaces for the services that our application needs in terms of our domain. Then we write a thin layer using the library to implement these interfaces, with focussed integration tests to confirm our assumptions about how it works, as in Figure 19.1, “An adaption layer above a third-party API”. How to make integration tests work effectively will depend on the environment, but we will need to do it anyway so we might as well start early and get some 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.

mockable-adaptor.svg

Figure 19.1. An adaption layer above a third-party API

There are some exceptions when mocking third-party libraries can be helpful. We might use mocks to simulate behaviour that is hard to trigger with the real library, such as throwing exceptions. Similarly, we 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.

One approach to Interaction Testing is to mock concrete classes rather than interfaces. The technique is to inherit from the class you want to mock and override the methods that will be called within the test, either manually or with any of the mocking frameworks. We think it's a technique that should be used only when you really have no other options.

Here's an example of mocking by hand, the test verifies that the music centre starts the CD player at the requested time. Assume that setting the schedule on a CdPlayer object involves triggering some behaviour we don't want in the test, so we override scheduleToStartAt() and verify afterwards that we've called it with the right argument.

public class MusicCentreTest {
  @Test public void 
  startsCdPlayerAtTimeRequested() {
    final MutableTime scheduledTime = new MutableTime();
    CdPlayer player = new CdPlayer() {
      @Override public void scheduleToStartAt(Time startTime) {
        scheduledTime.set(startTime);
      }
    }

    MusicCentre centre = new MusicCentre(player);
    centre.startMediaAt(LATER);

    assertEquals(LATER, scheduledTime.get());
  }
}

The problem with this approach is that it leaves the relationship between the objects implicit. We hope we've made clear by now that the intention of our approach to Test-Driven Development is to use Mock Objects to bring out relationships between objects. If we subclass, there's nothing in the domain code to make such a relationship visible, just methods on an object. This makes it harder to see if the service that supports this relationship might be relevant elsewhere and we'll have to do the analysis again next time we work with the class. To make the point, here's a possible implementation of CdPlayer:

public class CdPlayer {
  public void scheduleToStartAt(Time startTime) { …
  public void stop() { …
  public void gotoTrack(int trackNumber) { …
  public void spinUpDisk() { …
  public void eject() { …
}

It turns out that our MusicCentre object only uses the starting and stopping methods on the CdPlayer, the rest are used by some other part of the system. We would be over-specifying the MusicCentre by requiring it to talk to a CdPlayer, what it actually needs is a ScheduledDevice. Robert Martin made the point (back in 1996) in his Interface Segregation Principle that "Clients should not be forced to depend upon interfaces that they do not use", but that's exactly what we do when we mock a concrete class.

There's a more subtle but powerful reason for not mocking concrete classes. As part of the TDD with Mocks process, we have to think up names for the relationships we discover—in this example the ScheduledDevice. We find that this makes us think harder about the domain and teases out concepts that we might otherwise miss. Once something has a name, we can talk about it.

Sometimes, during the TDD process, we end up with a constructor that has a long, unwieldy list of arguments. Most likely, we got there by adding the object's peers (other objects it has to communicate with) one at a time, and it got out of hand. This is not dreadful, since the process helped us sort out the design of the object and its neighbours, but now it's time to clean up. The object must still be constructed in a valid state as required by the application. We will still need all the current constructor arguments, so we should see if there's any implicit structure there that we can tease out.

One possibility is that some of the arguments together define a concept that should be packaged up and replaced with a new object that represents it. Here's a small example.

public class MessageProcessor {
  public MessageProcessor(MessageUnpacker unpacker, 
                          AuditTrail auditer, 
                          CounterPartyFinder counterpartyFinder,
                          LocationFinder locationFinder,
                          DomesticNotifier domesticNotifier,
                          ImportedNotifier importedNotifier) 
  {
    // set the fields here
  }
  public void onMessage(Message rawMessage) {
    UnpackedMessage unpacked = 
       unpacker.unpack(rawMessage, counterpartyFinder);
    auditer.recordReceiptOf(unpacked);
    // some other activity here
    if (locationFinder.isDomestic(unpacked)) {
      domesticNotifier.notify(unpacked.asDomesticMessage());
    } else {
      importedNotifier.notify(unpacked.asImportedMessage())
    }
  }
}

Just the thought of writing expectations for all these objects makes us wilt, which suggests that things are too complicated. A first step is to notice that the unpacker and counterpartyFinder are always used together, they're fixed at construction and one calls the other. We can remove one argument by pushing the counterpartyFinder into the unpacker.

public class MessageProcessor {
  public MessageProcessor(MessageUnpacker unpacker, 
                          AuditTrail auditer, 
                          LocationFinder locationFinder,
                          DomesticNotifier domesticNotifier,
                          ImportedNotifier importedNotifier) {
  // etc.

  public void onMessage(Message rawMessage) {
    UnpackedMessage unpacked = unpacker.unpack(rawMessage);
    // etc.

Then there's the triple of locationFinder and the two notifiers, which seem to go together. It might make sense to package them together into a MessageDispatcher.

public class MessageProcessor {
  public MessageProcessor(MessageUnpacker unpacker, 
                          AuditTrail auditer, 
                          MessageDispatcher dispatcher) {
  // etc.

  public void onMessage(Message rawMessage) {
    UnpackedMessage unpacked = unpacker.unpack(rawMessage);
    auditer.recordReceiptOf(unpacked);
    // some other activity here
    dispatcher.dispatch(unpacked);
  }
}

Although we've forced this example to get it to fit within a section, it shows that being sensitive to complexity in the tests can help us clarify our designs. Now we have a message handling object that clearly performs the usual three stages: receive, process, and forward. We've pulled out the message routing code (the MessageDispatcher), so the MessageProcessor has fewer responsiblities and we know where to put routing decisions when things get more complicated. You might also notice that this code is easier to test.

When we're extracting implicit components, we look first for two conditions: arguments that are always used together in the class, and that have the same lifetime. That usually finds us the concept, then we have the harder task of finding a good name.

As an aside, one of the signs that the design is developing nicely is that this kind of change is easy to integrate. All we have to do is find where the MessageProcessor is created and change this

messageProceesor = 
  new MessageProcessor(new XmlMessageUnpacker(), 
                       auditer, counterpartyFinder, 
                       locationFinder, domesticNotifier,
                       importedNotifier);

to this

messageProceesor = 
  new MessageProcessor(new XmlMessageUnpacker(counterpartyFinder),
                       auditer,
                       new MessageDispatcher(
                         locationFinder, 
                         domesticNotifier, importedNotifier));

Later we can reduce the syntax noise by extracting out the creation of the MessageDispatcher.

Another diagnosis for a Bloated Constructor might be that the object itself is too large because it has too many responsibilities. For example,

public class Handset {
  public Handset(Network network, Camera camera, Display display, 
                DataNetwork dataNetwork, AddressBook addressBook,
                Storage storage, Tuner tuner, …) {
    // set the fields here
  }
  public void placeCallTo(DirectoryNumber number) { 
    network.openVoiceCallTo(number);
  }
  public void takePicture() { 
    Frame frame = storage.allocateNewFrame();
    camera.takePictureInto(frame);
    display.showPicture(frame);
  }
  public void showWebPage(URL url) {
    display.renderHtml(dataNetwork.retrievePage(url));
  }
  public void showAddress(SearchTerm searchTerm) {
    display.showAddress(addressBook.findAddress(searchTerm));
  } 
  public void playRadio(Frequency frequency) {
    tuner.tuneTo(frequency);
    tuner.play();
  }
  // and so on
}

Like our mobile phones, this class has several unrelated responsibilities which force it to pull in many dependencies. Like our phones, the class is confusing to use so we keep pressing the wrong button, and find that unrelated features interfere with each other. We're prepared to put up with these compromises in a handset because we don't have enough pockets for all the devices it implements, but that doesn't apply to code. This class should be broken up. The sort of fault lines we look for are fields that have no relationship with each other, particularly if the methods that reference them aren't used together either.

An associated smell for this kind of class is that its test suite will look confused too. The tests for its various features will have no relationship with each other, so we'll be able to make major changes in one area without touching any others. If we can break up the test class into slices that don't share anything, then the best thing might be to do just that and then slice up the object too.

Bring this up to date with only three stereotypes

A third diagnosis for a Bloated Constructor might be that not all of the arguments are dependencies, services that must be passed to the object before it can do its job. Other arguments might be more to do with structuring the implementation of the object or varying its behaviour to suit its neighbours. To help us think about this distinction, we've found it useful to categorise the peers of an object into stereotypes:

Dependencies must be passed in through the constructor. An object needs the system to provide implementations of its dependencies to function at all, so there's no point in creating an instance until they're available. Partially creating an object and then finishing it off by setting properties is risky and brittle because the programmer has to know whether all the dependencies have been set. As Yoda said, "New, or new not. There is no try."

Other types of peer can be passed to the constructor as a convenience but, if the list is becoming too long, they can also be initialised to safe defaults and overridden later (one way to distinguish the two is that there are no safe defaults for a Dependency). Policies and Parts can be initialised to commonly used cases, and collections of Parts can be initialised as empty. We can then add setters and add/remove methods to the class to allow the object's clients to configure it. Similarly, Notifications can be implemented as events. Unicast events can be initialised with a null implementation of the listener interface, and multicast events can be initialised with an empty collection of listeners. We always create a valid object, and we also have the hooks we need for testing.

public class RacingCar {
  private final Track track;
  private Tyres tyres;
  private Suspension suspension;
  private Wing frontWing;
  private Wing backWing;
  private double fuelLoad;
  private CarListener listener;
  private DrivingStrategy driver;

  public RacingCar(Track track, DrivingStrategy driver, 
                  Tyres tyres, Suspension suspension, 
                  Wing frontWing, Wing backWing, double fuelLoad,
                  CarListener listener)
  {
    this.track = track;
    this.driver = driver;
    this.tyres = tyres;
    this.suspension = suspension;
    this.frontWing = frontWing;
    this.backWing = backWing;
    this.fuelLoad = fuelLoad;
    this.listener = listener;
  }
}

public class RacingCar {
  private final Track track;

  private DrivingStrategy driver = 
                           DriverTypes.borderlineAggressiveDriving();
  private Tyres tyres = TyreTypes.mediumSlicks();
  private Suspension suspension = SuspensionTypes.mediumStiffness();
  private Wing frontWing = WingTypes.mediumDownforce();
  private Wing backWing = WingTypes.mediumDownforce();
  private double fuelLoad = 0.5;

  private CarListener listener = CarListener.NONE;

  public RacingCar(Track track) {
    this.track = track;
  }
    
  public void setSuspension(Suspension suspension) { […]
  public void setTyres(Tyres tyres) { […]
  public void setEngine(Engine engine) { […]
   
  public void setListener(CarListener listener) { […]
}

When a test has too many expectations it's hard to see what's important, what's really under test. For example, here's a test

@Test public void 
decidesCasesWhenFirstPartyIsReady() {
  context.checking(new Expectations(){{
    one(firstPart).isReady(); will(returnValue(true));
    one(organiser).getAdjudicator(); will(returnValue(adjudicator));
    one(adjudicator).findCase(firstParty, issue); will(returnValue(case));
    one(thirdParty).proceedWith(case);
  }});
  
  claimsProcessor.adjudicateIfReady(thirdParty, issue);
}

that might be implemented like this

public void adjudicateIfReady(ThirdParty thirdParty, Issue issue) {
  if (firstParty.isReady()) {
    Adjudicator adjudicator = organisation.getAdjudicator();
    Case case = adjudicator.findCase(firstParty, issue);
    thirdParty.proceedWith(case);
  } else{
    thirdParty.adjourn();
  }
}

What makes the test hard to read is that everything is an expectation, so everything seems to be equally important. We can't tell what's significant and what's just there to get through the test.

In fact, if we look at all the methods we call, there are only two that have any side effects outside this class: thirdParty.proceedWith() and thirdParty.adjourn(). It would be an error to call these more than once. All the other methods are queries, we can call organisation.getAdjudicator() repeatedly without breaking any behaviour. adjudicator.findCase() might go either way, but it happens to be a lookup so it has no side effects.

We can make our intentions clearer by distinguishing between Stubs, simulations of real behaviour that help me get our test to pass, and Expectations, assertions we want to make about how an object interacts with its neighbours. Reworking the test, we get

@Test public void 
decidesCasesWhenFirstPartyIsReady() {
  context.checking(new Expectations(){{
    allowing(firstPart).isReady(); will(returnValue(true));
    allowing(organiser).getAdjudicator(); will(returnValue(adjudicator));
    allowing(adjudicator).findCase(firstParty, issue); will(returnValue(case));
    
    one(thirdParty).proceedWith(case);
  }});
  
  claimsProcessor.adjudicateIfReady(thirdParty, issue);
}

which is more explicit about how we expect the object to change the world around it.

@Test public void 
decidesCasesWhenFirstPartyIsReady() {
  context.checking(new Expectations(){{
    allowing(firstParty).isReady(); will(returnValue(true));
    
    one(thirdParty).proceedWith(case);
  }});
  
  claimsProcessor.adjudicateIfReady(thirdParty, issue);
}

public void adjudicateIfReady(ThirdParty thirdParty, Issue issue) {
  if (firstParty.isReady()) {
    organisation.adjudicateBetween(
        firstParty, thirdParty, issue);
  } else {
    thirdParty.adjourn();
  }
}

public void adjudicateIfReady(ThirdParty thirdParty, Issue issue) {
  if (firstParty.isReady()) {
    thirdParty.startAdjudication(
        organisation, firstParty, issue);
  } else{
    thirdParty.adjourn();
  }
}

We had one posting to the jMock user list that included this comment

We've seen this failure mode on other projects we've been helping with, so we think there might be a common pattern.

Unit test code shouldn't get that large, except for unusual circumstances. Unit tests are supposed to focus on at most a few classes and shouldn't need a large amount of set up. What we've seen on projects was enormous amounts of preparation and positioning to get the objects into a state where the expected feature could be exercised. Of course it's hard to understand the point of a test when there's just so much code. If you see this pattern, then we suggest that the code (or at least the test code) needs breaking. Integration and acceptance tests that happen to be written using a unit test framework, on the other hand, might have to be large.

It's not yet clear whether the naive use of interaction-testing is particularly susceptible to this failing, or whether it happens all the time and we're the only ones who get complained to. We are, however, convinced that an emphasis on using Mock Objects mainly to substitute external systems (some of which we perpetrated in the early days) is a deeply bad idea which leads teams towards this sort of problem.

Link smells to possible solutions: Test Smell -> Code Smell -> Refactoring. Maybe as an appendix?

We've found these benefits from learning to Listen to Test Smells

Keeping the code clean as we go along minimizes the risk of nasty surprises later when a new requirement triggers significant changes to the design and the developers end up being pressured into botching the code to get the job done. Just a few of these episodes can make further changes increasingly difficult and dissipate the good intentions of the team.