Tuesday, November 14, 2006

Is the clutter in your test trying to tell you something?

We’ve all encountered it at some point or another. Your unit test is doing the job, but the test is so bogged down with setup code that it is impossible to see the wood for the trees. The interesting test code is in there somewhere, but where exactly?

The common causes of clogs in your test code are:

  • Complex object creation code.
  • Preceding interactions with the test subject.

Complex object creation is simple to deal with. You can create test fixtures to create the object for you, or better still you can refactor the code to try and simplify things a little.

But what about the interaction clutter? This is more subtle in the way that it insinuates itself into the test code. The clutter sneaks up on you. It demands more and more code to be added due to 'unavoidable' interactions with one object after another. Soon the useful to useless LOC ratio in the test case has tipped. Suddenly it becomes the norm that you should have N interactions in the test case to simulate calls to the database and N more calls out to other supporting services, even though the aspect of the code which you want to test has no real need to go through these extra steps.

For a particularly contrived example, lets imagine that we are building an online Pie ordering service for Weebl & Bob's Pie Delivery Service…and we are implementing it using WebWork.

We are in the final stages of implementing our pie ordering action. We want to verify that on execution the action invokes a number of support services:


public void testShouldOrderBobAYummyPieOnSubmit() {
PieOrder yummyPieOrder = new PieOrder(“yummypie”, 1, “for Bob”);
PieOrderingAction pieAction = new PieOrderingAction(orderService);
pieAction.setAction(Action.SUBMIT);
pieAction.execute();
assertEquals(yummyPieOrder, orderService.getLastPieOrdered())
}

But our test fails to run - it never reaches our orderService throwing nasty null object exceptions instead. But why? Looking more carefully at our execute method we make a number of discoveries:
  1. When our action executes, it actually loads our order from the database using its repository and the supplied pie-ID. This test doesn’t supply a repository or a pie-ID. So we buckle down and add it into the test.
  2. Weebl and Bob’s Pie Delivery Service often runs out of stock. So before we can allow our customer to order our pie we need to call out to our pieInventoryService to ensure that the yummy pie actually exists and is ready to be cooked for our customer. Grrrr. We add more support code into the test.
Now our test has become a bit of a monster:

public void testShouldOrderBobAYummyPieOnSubmit () {
PieOrder yummyPieOrder = new PieOrder(“yummypie”, 1, “for Bob”);
PieOrderingAction pieAction = new PieOrderingAction(orderService);
pieAction.setPieOrderRepository(pieOrderRepositoryMock);
pieOrderRepository.expects(once()).method(loadOrder)
.will(returnValue(yummyPieOrder));
pieAction.setPieInventoryService(pieAlwaysInStockStub);
pieAction.setAction(Action.SUBMIT);
pieAction.setPieID(yummyPieOrder.getPieD());

pieAction.execute();

assertEquals(yummyPie, orderService.getLastPieOrdered())
}
OK, so this isn’t the end of the world. But it isn’t so hot either in pastry-free, real world scenarios, this could be much worse. The problem here is that our implementation of execute has 3 phases:
  1. Load PieOrder
  2. Verify PieOrder
  3. Submit PieOrder

public class PieOrderPacingAction…{
public void execute(){
//Load pie order…
//validate pie order…
//submit pie order…
}
}
We really only want to test phase 3 in isolation. Someone has already implemented tests and production code for steps 1 and 2 – we don’t want to repeat ourselves.

Time to listen to the test. Our test code is suggesting that this is not an optimal implementation. We could choose to hide all this away in obscure setup code, or we could try to find a better way of implementing our code. So we pick the best option and look into the API of our actions and discover that we could implement this differently. WebWork supports actions which implement prepare() and validate() methods. Woot! We can move our code into these calls instead and simply our production code.


public class PieOrderPacingAction…{
public void prepare(){
//Load pie order…
}
public void validate (){
//validate pie order…
}
public void execute(){
//submit pie order…
}
}


The beauty of this is that, in theory we can chop up our tests.
  1. Our first set of tests check that the action loads the PieOrder during prepare().
  2. Our second set of tests prove that the action validates its order during validate()
    Given that the action has already loaded the order..
  3. Our third set of tests verify that the action will submit the pie order during execute()
    Given that the action has already loaded and validated the order.

So our test case looks like this now:



public void testShouldOrderBobAYummyPieOnSubmit () {
PieOrder yummyPieOrder = new PieOrder(“yummypie”, 1, “for Bob”);
PieOrderingAction pieAction = new PieOrderingAction(orderService);
TestUtils.setPrivateField(pieAction, “pieOrder”, yummyPieOrder);
pieAction.setAction(Action.SUBMIT);

pieAction.execute();

assertEquals(yummyPie, orderService.getLastPieOrdered())
}


Those following closely will have noticed a call to set the pieOrder field via reflection. So why did we set the private field here (using evil reflection of all things!)? Well, this comes back to not test driving the code the wrong way. We could have added a setter to allow a more traditional way of setting the object state. However, this is not useful to the real world. We are only setting the internal field in order to get our object into the correct state to test it. It is less evil to use reflection to put an object into a valid state for a test situation than it is to add access to internal state legally for all code.

So what are the issues that we have to watch out for here?
  • Obviously the test is now more tightly coupled to the implementation details of our test subject. If we change the internal structure of our object, we could well break our tests. We can mitigate this by not making our object complicated. In this example we only reach into our object to set one field. Try and keep things this simple when using this technique
  • There is a risk that the tests don’t join up properly. If you are going to reach into an object to set its state, you must have corroborating test cases which prove that the object is in fact in this state at the end of the preceding calls in its lifecycle. It may also be worth writing a small number of ‘integration tests’ which run the object through the whole call cycle (here, prepare -> validate -> execute) in order to demonstrate the expected behaviour of caller and to prove that you got things right with the lower level testing.

2 comments:

euluis said...

I would say that you should always have integration tests that verify the implementation works correctly from the component/subsystem level.

I normally see three levels of testing as necessary:

* unit tests - the focus of your post and where TDD allows influencing the design by having a interactive style of development

* integration tests (component or subsystem level tests) - enable subsystem validation and are a very good example to other subsystems on how to use the subsystems they depend on

* acceptance tests (system level tests) - this is where the overall thing must work together

From my point of view the development team is responsible at least for unit test and integration test. If they don't have automated integration tests they are delaying risks and buying a lot of late worries. Another issue which is improved by integration tests is to improve the design of subsystem boundaries.

Jeff Santini said...

I know this is a contrived example, but I must point out that I think both the retrieval of a repository and checking inventory levels belong behind a service boundary and should not happen in an action at all. If the OrderService does this work instead of the Action then the test stays simple.

A WebWork action has no business interacting with a repository in my opinion, and checking stock levels is certainly application logic so should live behind the service boundary.

I try to write the test as simple as it should be then force the tested objects to comply. Doing that would have driven the Repository and Inventory calls underground where they should be(behind the service layer). Then voila no reflection. The room for error here is in writing a test that does not follow the businesses view of the interactions. I.E. you need to understand the domain, and hopefully can speak the business' language.

P.S. I love the point being made, just trying to highlight other strategies to get there.