A colleague in the process of refactoring a legacy codebase she had inherited recently asked me if she should be writing tests for the builders she was creating.
I'm never short on opinions, so I gave an off the cuff answer, but we thought it would be interesting to apply a little more rigour to the question.
Builders are a well known pattern that help construct complex objects. They are particularly important for constructing immutable objects that require large constructor parameter lists, but can also result in cleaner code when building mutable objects, particularly in tests.
The basic idea is to use a mutable object that supports method chaining as input to an immutable one. The pattern is described in effective java with an example implementation that looks like
public class NutritionFacts {
private final int servingSize;
private final int servings;
private final int calories;
private final int fat;
private final int sodium;
private final int carbohydrate;
public static class Builder {
// Required parameters
private final int servingSize;
private final int servings;
// Optional parameters - initialized to default values
private int calories = 0;
private int fat = 0;
private int carbohydrate = 0;
private int sodium = 0;
public Builder(int servingSize, int servings) {
this.servingSize = servingSize;
this.servings = servings;
}
public Builder calories(int val)
{ calories = val; return this; }
public Builder fat(int val)
{ fat = val; return this; }
public Builder carbohydrate(int val)
{ carbohydrate = val; return this; }
public Builder sodium(int val)
{ sodium = val; return this; }
public NutritionFacts build() {
return new NutritionFacts(this);
}
}
private NutritionFacts(Builder builder) {
servingSize = builder.servingSize;
servings = builder.servings;
calories = builder.calories;
fat = builder.fat;
sodium = builder.sodium;
carbohydrate = builder.carbohydrate;
}
}
}
A NutritionFact can then be constructed with
NutritionFacts cocaCola = new NutritionFacts.Builder(240, 8).
calories(100).sodium(35).carbohydrate(27).build();
This style of builder uses a nested class, but there are many variations where the builder is separate from the class, the builder itself is immutable etc.
My colleague was using mutable builders implemented as top level classes.
Based on an earlier conversation my colleague was sold on the advantages of introducing builders into the particular codebase she was working on, but had a reservation. Should she be writing tests for these new builder classes? That sounded like a lot of work.
My initial answer was no - I'd treat builders like the getters and setters on beans.
I'd expect them to have 100% line coverage, but I wouldn't write any explicit tests for the builders. I'd assume that the tests that described the behaviour of the constructed objects would highlight any faults in the builders that created them.
My colleague was uncomfortable with creating a class without a corresponding test. This unease could be reduced by following the common convention of implementing builders as static inner classes of the class they construct, but whether the builders are stand alone or nested the same logic applies as to whether they should be explicitly tested.
So how do we go about testing the theory that builders are sufficiently tested by describing the behaviour of the object they create?
Well the simplest place to start is by looking at the line coverage.
A quick check on our sonar instance confirmed that the existing builders did indeed have 100% line coverage.
Question closed?
Well no. I can talk at some length about the difference between executing code and testing code.
To see if the drive by coverage would actually catch bugs in the builders we need to go a little deeper. We need to mutation test.
I won't re-describe mutation testing here as I think the pitest site does a reasonable job, but for those not familiar the tl;dr version is that it is a way of gauging the quality of a test suite by introducing deliberate faults into the code under test.
This wasn't a great codebase for mutation testing as it contained some quite slow not-really-unit-tests and others I was not confident would run cleanly in parallel, but after a little fiddling and a few seconds analysis pitest produced a report looking at just the Builders.
Normally I would recommend using the default set of mutation operators, but as the mutated code was both small and non complex I enabled the full set to ensure a reasonable number of interesting faults were generated.
Of the available operators, only two generated mutations in most of the builders. The return values mutator and the member variable mutator.
The return value mutations aren't very interesting as this is a very unstable mutation when it is applied to object returns - it just changes them to return null. So the builder code is changed to be the equivalent of
public AppleBuilder withRipeness(double ripeness) {
this.ripeness = ripeness;
//return this;
return null;
}
The only difference between a killed return value mutation and line coverage for objects is that the mutation confirms that some bit of code somewhere did something with the returned value. Return value mutations are more interesting for primitive values, but builders don't normally return primitives.
The member variable mutator is much more interesting for builders.
The generated mutations follow the form
public AppleBuilder withRipeness(double ripeness) {
//this.ripeness = ripeness;
return this;
}
These mutations tell us a lot more. If the above mutation is not killed by a test we know there is no effective check that the ripeness value propagates through the code.
Almost all the generated mutations were killed by the test suite for this codebase. Where the mutations were not killed it was because the code has been written pre-emptively and had no line coverage.
Based on this quick analysis I'd stick with my original answer of no, not explicitly.
The same analysis on a different codebase could yield different results.
There is of course a trade-off being made here. If a builder did contain a bug that caused a test to fail it might not be easy to track down the cause, but I personally think the overhead added by test driving builders is not worth it.
We have reason to be confident that such a bug would cause a test to fail and alert us to the fact that there was a problem somewhere. Such bugs should be relatively uncommon in the simple boiler-plate code of builders. As they are fulfilling a well understood contract there is little need to document the behaviour of builders via tests.
If logic starts to find it's way into the builders in addition to the boilerplate the answer will of course change. This had happened in a couple of the builders in this codebase - this code needs to be removed and test driven back into the builder or a more appropriate location.
So we have some evidence that a decision not to test drive hand built builders is a reasonable one, but can we do better?
We could avoid the whole question by auto generating the builders in some fashion.
There are various projects to do this, most of which are code generators that are run from the IDE or build script
A small sample of the available options is shown below
While less code is always a good thing I've personally never been a fan of code generators and I've usually regretted using them when I have introduced them to a code base.
I'm aware of two options that do not work as code generators. I don't think either is suitable for general purpose builders in production code, but both are worth considering for the special case of Test Data Builders.
One very common usage of builders is Test Data Builders. These help write clear understandable tests by enabling them to specify only those parts of an object that need to vary for a test, and use sensible defaults for other required values.
There are various styles of test data builder, the example below is similar to that described in the GOOS book and is in fact lightly adapted from one of the author's examples in a project linked further down.
public class AppleBuilder {
private double ripeness = 0.0;
private int leaves = 1;
private AppleBuilder() {}
public static AppleBuilder anApple() {
return new AppleBuilder();
}
public Apple build() {
Apple apple = new Apple(leaves);
apple.ripen(ripeness);
return apple;
}
public AppleBuilder withRipeness(double ripeness) {
this.ripeness = ripeness;
return this;
}
public AppleBuilder withLeaves(int leaves) {
this.leaves = leaves;
return this;
}
}
This can be used in a test a follows
public class AppleProcessorTest {
AppleProcessor testee = new AppleProcessor();
@Test
public void shouldRejectApplesWithLessThan42Leaves() {
assertFalse(testee.process(anApple().withLeaves(41).build());
}
. . . etc
}
This test cares only about the number of leaves the apple has. The builder ensures this is the only value the test needs to mention.
This benefit may seem slight for an object with only 2 properties, but becomes much clearer with more complex objects.
Nat Pryce maintains a small library called make-it-easy that reduces the required builder boiler plate using pure java.
Modifiable properties are modelled as typed property objects
public class AppleMaker {
public static final Property<Fruit,Double> ripeness = newProperty();
public static final Property<Apple, Integer> leaves = newProperty();
public static final Property<Banana,Double> curve = newProperty();
public static final Instantiator<Apple> Apple = new Instantiator<Apple>() {
@Override public Apple instantiate(PropertyLookup<Apple> lookup) {
Apple apple = new Apple(lookup.valueOf(leaves, 2));
apple.ripen(lookup.valueOf(ripeness, 0.0));
return apple;
}
};
}
These can then be used as below
Maker<Apple> appleWith2Leaves = an(Apple, with(2, leaves));
Maker<Apple> ripeApple = appleWith2Leaves.but(with(ripeness, 0.9));
Maker<Apple> unripeApple = appleWith2Leaves.but(with(ripeness, 0.125));
Apple apple1 = make(ripeApple);
Apple apple2 = make(unripeApple);
Although I like the idea behind make it easy I've never really taken to it. My issue with it is partly that it still requires some boiler plate, but mainly that the interface the builders present isn't discoverable.
With a classic builder you can type builder.
in eclipse and get auto suggestions for what to set. With make-it-easy you have to search around for the allowable properties.
The other option for minimal code builders I'm aware of is my own offering QuickBuilder.
This has a different set of trade offs than make-it-easy. It creates builders at run time that implement a familiar looking builder interface that you supply.
For a mutable java bean all you'd need to write is
interface PersonBuilder extends Builder<Person> {
PersonBuilder withName(String name);
PersonBuilder withAge(int age);
PersonBuilder withPartner(Builder<Person> partner);
PersonBuilder withFriend(Person partner);
}
and then get an instance with
QB.builder(PersonBuilder.class)
For non beans you need to do a little more work, but the amount of code required is still far less than a hand built builder.
The downside is it uses magic (aka bytecode manipulation), so certain types of error become runtime errors rather than compile time errors. It offers options to reduce the magic at the expense of writing a little more boilerplate, but some magic always remains and that can cause headaches.
Is this a better approach than hand rolled builders or code generators? I doubt there's any single correct answer to that question but I have been happy with the trade offs in the code bases I've used it in so far.
I'd be interested in feedback from anyone that does give quickbuilder a try.
I've received a lot of feedback since this was posted. Ignoring the strong opinions on the correct placement of commas most can be paraphrased as one of :-
This is correct in that some builders will do more than just provided named parameters.
The ones in this codebase didn't (and most ones I've written recently don't), but as the link to the effective java shows they may provide default values and checks on the state in the build method.
I'm not suggesting that this application specific logic is somehow exempt from unit testing. As I said if the builder contains logic, that logic should be explicitly tested.
A more accurate title for the article would probably have been "should I explicitly test the boilerplate code within my builders?".
If you've written some code utilising the builder boilerplate, whether that code is within the builder class itself or elsewhere, it is the behaviour of that higher level code you should be explicitly describing.
This quick mutation analysis suggests that such tests are likely to catch faults in the boilerplate code.
It is being tested, just not explicitly.
It isn't being described by a test as the behaviour of the boilerplate is already well know - like the setter on a bean.
The boilerplate is written as the logic that then uses it requires it. That logic can be test driven - you need never write a line of boilerplate without first writing a test.
Yes. When you put it like that it is fairly obvious and not too controversial.
Henry Coles
Tags: tdd, java
February 26th 2015