23 May 2007

Test Smell: Too many expectations

Synaesthesia

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.

4 comments:

Debasish said...

I like your observation: "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."

In my team we are trying to make test methods as close to domain as possible, by separating out stubs and mocks from the meat of the test. I feel the test methods should also speak the domain. I have recently blogged about my experience with this. Would love to know more about your feedback on this.

Cheers.
- Debasish

Steve Freeman said...

I like your syntax for the CalculatorScaffold. Maybe it's suggesting a need for a Calculation object?

I haven't done much TestNG. I can't see the point in the data provider. Why don't you just call the method three times with different arguments?

One thing to consider is that I've found that my code style for tests is different from that for production. I'm in a different domain (testing), so I need to express different things.

Colin Jack said...

One thing that would make mocking/stubbing discussions clearer would be if it was made clearer in authoritive resources that these are things that you do at the method (or member) level rather than at the class level.

So I might create a test double and set expectations (so mock) one method but just stub another method. The resulting class isn't a mock or a stub, its just a test double that does multiple things.

Anyway I'm glad you guys are doing that! :)

Steve Freeman said...

@Colin. You know, I think we never made that point clear, that the distinction between stubs and mocks is /at the level of the interaction/, not the type. Thanks.