Test Smell: Too many expectations

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. I can't tell what's significant and what's just there to get me through the test.

In fact, if I look at all the methods I 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, I 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.

I can make my intentions clearer by distinguishing between Stubs, simulations of real behaviour that help me get my test to pass, and Expectations, assertions I want to make about how my object interacts with its neighbours. Reworking my test, I 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 explicit about how I expect my object to change the world.

Stub queries, mock actions

For the more formally minded, you might want to think (very, very loosely) of Stubs as pre-conditions and Expectations as post-conditions. The test now says that given the state described in the Stubs, the events described by the Expectations should have happened. Once I've sorted out which calls are Stubs, I can usually move some of them into common setup code, making the tests clearer still:

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

It is, of course, possible that all the methods calls are actions. In which case, either I just have a complicated domain and there's not much I can do to simplify it or, more likely, there are some intermediate concepts in there that need to be extracted into objects.

Clearly, when I'm thinking about the whole system, I can't ignore implementation issues such as the cost of database queries, but that's not relevant at this point. What matters now is that I have tests and code that clearly express my intentions.

On a side note, this is why I'm not keen on an expectation syntax like this: expect(firstParty.isReady()).stubReturn(true). I find the leading "expect" confusing, when my intention is to stub.

Special bonus prize

I always have problems coming up with good examples. There's actually a better improvement to this code, which is to notice that that I've pulled out a chain of objects to get to the case object, exposing dependencies that aren't relevant here. What I should have done is told the nearest object to do the work for me, like this:

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

or, possibly,

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

which looks more balanced. If you spotted this then I award you a Moment of Smugness™ to be exercised at your convenience.

Apache Camel

The Apache ActiveMQ project has recently released Camel, a rule based routing and mediation engine for building enterprise integration systems. Camel provides a DSL-style Java API, inspired by jMock and LiFT, to specify rules for routing, transforming and monitoring messages and you can use mock message receivers to describe and verify expected message flows in tests.

Testability vs. Design (again)

I'm sure TypeMock is a fine piece of software, but I just cannot agree with their notion that API design and testability are in conflict. Their new case study includes the statement:

The key benefit we get from TypeMock is having the ability to fully unit-test the code without impacting the API design. [...] For us, the API is part of the deliverable. We need to make it fairly easy to consume and can't have the architecture of the solution overshadow the usability of the API.

In the other corner we have Michael Feathers who has been running a tutorial called API Design as if Unit Testing mattered. Michael is one of the most interesting people I know on the Agile circuit, although he doesn't make a fuss about it. To quote him:

Have you ever tried to write a unit test for a class that uses an external API? An API that is not mockable? Testing seems to be the last thing on our minds when we design APIs. It isn't that we don't test our APIs, we just don't make it easy to test code that uses our APIs.

His point is that, for library designers, even writing tests for your API is not enough to produce something that's really useable, you have to try writing tests for the code that will use your API. Or, as Michael Puleio wrote,

If your API is hard to unit test, how in the heck are your users going to be able to use it?

What the tests will tell you

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

Keeping knowledge local

Some of the test smells we've identified, such as needing magic to create mocks, are to do with knowledge leaking between components. If I can keep knowledge local to an object, either internal or passed in, then its implementation is independent of its context. I can safely move it wherever I like. Do this consistently and I can build an application as a web of communicating objects.

If it's explicit I can name it

This is why I don't like mocking concrete classes, I like to have names for the relationships between objects as well the objects themselves. As the legends say, if I have something's true name then I can control it. If I can see it, I have more chance of finding other uses and so reducing duplication.

More names mean more domain information

I find that when we emphasise how objects communicate, rather than what they are, I end up with types and roles defined more in terms of the domain than of the implementation. I think this might be because I have more, smaller abstractions which gets me a further away from the underlying language. Somehow I seem to get more domain vocabulary into the code.

Pass behaviour rather than data

I find that TDD with mocks encourages me to write objects that tell each other what to do, rather then requesting and manipulating values. Applied consistently, I end up with a coding style where I pass behaviour (in listeners and callbacks) from the higher levels of the code to the data, rather than pulling data up through the stack. It's an unusual style in the Java/C# world, but I find that we get better encapsulation and clearer abstractions because I have to clarify what the pieces will do, not just what values they hold.

Object Collaboration Stereotypes

Another 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 me think about this distinction, I've found it useful to categorise the peers of an object into four stereotypes:

• Dependencies are services that the object needs from its environment so that it can fulfil its responsibilities.
• Notifications are other parts of the system that need to know when the object changes state or performs an action.
• Policies are objects that tweak or adapt the object's behaviour to the needs of the system.
• Parts are components in the implementation that are not controlled from outside the object after being set.

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. I 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. I always create a valid object, and I also have the hooks I need for testing.

A high speed example

Here's an example, it's probably not quite bad enough to need fixing, but it'll do to make the point. The application is a Formula 1 racing game, players can try out different configurations of car and driving style to see which one wins. A RacingCar represents a competitor within a race.

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;
  }
}

It turns out that the track is the only Dependency of a RacingCar, the hint is that it's the only field that's final. The driver is a Policy, the listener is a Notification, and the rest are Parts; all of these can be modified by the user before or during the race. Here's a reworked constructor:

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) {  …
  …
}

Now I've initialised the peers to most common defaults, the user can then configure them through the user interface. I've initialised the listener to a null implementation, again this can be changed later by the object's environment.

A matter of taste

These stereotypes are only heuristics to help me think about the design, not hard rules, so I don't get obsessed with finding just the right classification of an object's peers. What matters most is the context in which I'm developing the object, my mental model of the domain. For example, an auditing log might be a Dependency, since it's a legal requirement for the business, or a Notification, since the object will otherwise function without it. Policies belong to the environment and can usually be constants, since they're often immutable and so don't need to be instantiated more than once. Parts belong to the object and usually cannot be stateless, so they need a new instance. For example, in my Racing Car, tyres might be a Policy if TyreTypes.mediumSlicks() describes only how the choice of tyre affects the car's handling, but a Part if it stores mutable values such as air pressure and wear.

Mocking classes challenge

I'm in a communcation quandry. I'm either missing a point or can't explain something, or both.

I really don't like mocking classes, except where I'm trying to cut a seam in some legacy code, but other people disagree.

Can someone post or send me an example of code where interaction-testing with interfaces doesn't work well? Thanks

Contact me at mockexample theAtSymbol m3p theDotCharacter co anotherDotHere uk

Cross-referencing Code Smells

While we've been writing up our Test Smells, here's a good summary of some Code Smells

Test Smell: Confused object

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 my mobile phone, this class has several unrelated responsibilities which force it to pull in many dependencies. Like my phone, the class is confusing to use so I keep pressing the wrong button, and I find that unrelated features interfere with each other. I'm prepared to put up with these compromises in my handset because I don't have enough pockets for all the devices it implements, but that doesn't apply to my code. This class should be broken up. The sort of fault lines I 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 I'll be able to make major changes in one area without touching any others. If I 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.