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.