Mark Needham

I’ve written before about my favoured approach of writing tests in such a way that they have clear ‘Given/When/Then’ sections and something which I come across quite frequently is tests where the latter steps have been combined into one method call which takes care of both of these.

An example of this which I came across recently was roughly like this:

@Test
public void shouldCalculatePercentageDifferences() {
	verifyPercentage(50, 100, 100);
	verifyPercentage(100, 100, 0);
	verifyPercentage(100, 50, -50);
}

private void verifyPercentage(int originalValue, int newValue, int expectedValue) {
	assertEquals(expectedValue, new PercentageCalculator().calculatePercentage(originalValue, newValue));
}

This code is certainly adhering to the DRY principle although it took us quite a while to work out what the different numbers being passed into ‘verifyPercentage’ were supposed to represent.

With this type of test I think it makes more sense to have a bit of duplication to make it easier for us to understand the test.

We changed this test to have its assertions inline and make use of the Hamcrest library to do those assertions:

@Test
public void shouldCalculatePercentageDifferences() {
	assertThat(new PercentageCalculator().calculatePercentage(50, 100), is(100));
	assertThat(new PercentageCalculator().calculatePercentage(100, 100), is(0));
	assertThat(new PercentageCalculator().calculatePercentage(100, 50), is(-50));
}

I think we may have also created a field to instantiate ‘PercentageCalculator’ so that we didn’t have to instantiate that three times.

Although we end up writing more code than in the first example I don’t think it’s a problem because it’s now easier to understand and we’ll be able to resolve any failures more quickly than we were able to previously.

As Michael Feathers points out during Jay Fields’ ‘Beta Test‘ presentation we need to remember why we try and adhere to the DRY principle in the first place.

To paraphrase his comments:

In production code if we don’t adhere to the DRY principle then we might make a change to a piece of code and we won’t know if there’s another place where we need to make a change as well.

In test code the tests always tell us where we need to make changes because the tests will break.

Something which I’ve noticed over the last few projects that I’ve worked on is that at the beginning when I don’t know very much at all about the code base, domain and so on is that pairing with someone to TDD something seems to make it significantly easier for me to follow what’s going on than other approaches I’ve seen.

I thought that it was probably because I’m more used to that approach than any other but in Michael Feathers’ description of TDD in ‘Working Effectively With Legacy Code‘ he points out the following:

One of the most valuable things about TDD is that it lets us concentrate on one thing at a time. We are either writing code or refactoring, we are never doing both at once.

It also temporarily removes us from the feeling of drowning in all the new information which is typical at the start of projects. In a way it reminds me of the ‘Retreat Into Competence‘ pattern from ‘Apprenticeship Patterns‘ as it gives us a chance to regain composure and take in all the new information.

We can probably make a much more useful contribution if we only need to understand one small piece of the system rather than having to understand everything which can often be the case with what Feathers coins the edit & pray approach to development.

It’s still necessary to spend some time trawling around the code base working out how everything fits together but I’m now more aware that it’s also really useful to take some time to just focus on one much smaller class or piece of functionality.

Perhaps also something to keep in mind the next time I’m pairing with someone who’s new to a project that I’ve been working on for a while.

We recently changed the way that we test against our main integration point on the project I’ve been working on so that in our tests we retrieve the service object from our dependency injection container instead of ‘newing’ one up.

Our tests therefore went from looking like this:

[Test]
public void ShouldTestSomeService()
{
	var someService = new SomeService();
	// and so on
}

To something more like this:

[Test]
public void ShouldTestSomeService()
{
	var someService = UnityFactory.Container.Resolve<ISomeService>();
	// and so on
}

This actually happened as a side effect of another change we made to inject users into our system via our dependency injection container.

We have some ‘authenticated services’ which require the request to contain a SAML token for a valid user so it seemed to make sense to use the container in the tests instead of having to duplicate this piece of behaviour for every test.

We needed to add our fake authorised user into the container for our tests but apart from this the container being used is the same as the one being used in the production code.

Our tests are therefore now calling the services in a way which is much closer to the way that they are called in the code than was previously the case.

I think this is good as it was previously possible to have the tests working but then have a problem calling the services in production because something in the container wasn’t configured properly.

The downside is that these tests now have more room for failure than they did previously and they are not just testing the end point which was their original purpose.

In a way what we have done is convert these tests from being contract tests to integration tests.

I like the new way but I’m not completely convinced that it’s a better approach.

Something which I noticed was a problem with the first example test that I provided in my post about API readability and testability is that the assertion we are making is not that great.

[Test]
public void ShouldConstructModelForSomeSituation()
{
	Assert.AreEqual(DateTime.Today.ToDisplayFormat(), model.SomeDate()); 
}

It’s not really obvious what the expected result is supposed to be except that it should be the ‘DisplayFormat’. If that fails then we’ll need to navigate to the ‘ToDisplayFormat’ method to work out what that method does.

I think it should be possible to immediately know why a test failed so that we can address the problem straight away without too much investigation.

In this example we changed the way the code was working which coincidentally allowed us to make the assertion more obvious.

[Test]
public void ShouldConstructModelForSomeSituation()
{
	Assert.AreEqual("10 Oct 2009", model.SomeDate()); 
}

Erik pointed out another example of this a few weeks ago while we were working on some HTML helper code.

The tests in question looked roughly like this:

[Test]
public void ShouldConstructReadOnlyValue()
{
	var readOnlyValue = new HtmlHelper().ReadOnlyValue("someId", "someValue", 'someTextValue');
 
	...
	var expectedValue = new TestHtmlBuilder().AddLabel("someId", "someTextValue").AddHiddenField("someId", "someValue").Build();
 
	Assert.AreEqual(expectedValue, readOnlyValue);
}

The test reads reasonably nicely and it’s fairly obvious what it is we’re testing for.

The problem here is that we’ve hidden away our expectation and in this case we actually found out that the ‘TestHtmlBuilder’ had extra spaces in some places so all our assertions were incorrect and we didn’t even know!

In addition we end up duplicating the logic that the ‘HtmlHelper’ is doing if we create test assertion helpers like these.

[Test]
public void ShouldConstructReadOnlyValue()
{
	var readOnlyValue = new HtmlHelper().ReadOnlyValue("someId", "someValue", 'someTextValue');
 
	...
	var expectedValue = @"<input type=""hidden"" id=""someId"" value=""someId"" /><label for=""someId"">someValue</label>";
 
	Assert.AreEqual(expectedValue, readOnlyValue);
}

The new test doesn’t look as clean as the old one but the assertion is much more obvious so if it fails then we can quickly work out why.

My colleague Matt Dunn and I have been putting a lot of tests around some code over the last few days so that we can safely make some changes around that area and having finally created our safety net we’ve moved onto adding in the new functionality.

We’re test driving the new bit of functionality whereas with the previous code only the code had been written with no unit tests and it’s been quite interesting seeing the contrast in the style of code which seems to come out from these differing styles.

While we were writing the tests around the other code I found it quite surprising how often we came across code which wasn’t actually needed – I guess if you only write the code then you need to make sure that all potential scenarios are covered even if it’s impossible for them to happen.

In one case a null check was being done on a dependency of a class which always has its dependencies injected by a DI container and thus as far as I understand could never actually be null.

It seems to me that the outcome of this approach to coding can contribute greatly to what Fred Brooks coins ‘Accidental Complexity’ in ‘The Mythical Man Month‘ – extra code which makes the code base more difficult to read as a whole but which adds no value apart from that. Essentially waste.

When test driving code I find that the process of coming up with examples/tests before you write any code leads you to question how the system actually works.

For example Matt and I had thought that the new piece of functionality we were working on would need to make a call to a repository to retrieve some data since that’s what was being done for the existing functionality.

However, while writing our tests it become quite clear that we wouldn’t need to do that.

On this occasion we wrote a test with an expectation for a call on the repository, then wrote the code to make that test to pass before moving onto our test where we would assert that the values retrieved from the repository would be populated into the appropriate object.

It was at this stage that we realised that there was actually no need to make that call because we already had the data available to us from another source.

This has happened on quite a few occasions since and while I think pair programming helps put us in a state of mind where we are more inquisitive of the code being written, I think the TDD approach is quite useful as well.

I’m sure it’s possible to avoid some of the problems I’ve described without a test driven approach but I certainly find it more difficult.

I’ve been re-reading a post my colleague Ian Cartwright wrote earlier this year about treating test code the same way as production code and one thing which stands out as something which I’m certainly guilty off is copying and pasting tests.

Ian lists the following problems with doing this:

The first one is cut & paste, for some reason when it comes to unit tests people suddenly start cutting and pasting all over the place. Suddenly you find a file with 20 tests each of which repeats exactly the same few lines of code. I don’t think I need to describe why this is bad and I expect we’ve all seen the outcome: at some point later those tests all start breaking at the same time, if we are unlucky a few tweaks have happened to the cut & pasted code in each so we spend a lot of effort figuring out how to make each one pass again. There is no rule that says we can’t have methods in test fixtures so ExtractMethod still applies, and using SetUp sensibly often helps. The same rational for avoiding cut & paste and the same solutions we know from production code apply to test code.

Despite this I’ve often felt that at some stage we should have factored tests down to a stage where removing any more of the ‘duplication’ would actually harm the readability of the test and that at this stage perhaps it’s ok to copy the skeleton of a test and reuse it for your next test.

Since I first read Ian’s post I’ve been playing around with the idea of never copying/pasting tests when I’ve been working on my own on my project and with stuff I play around with outside work and I like the approach but hadn’t really fully appreciated the benefits that we could get from doing so.

Over the last week or so my colleague Matt Dunn and I have been writing tests around pre-existing code and we decided that we would try wherever possible to not copy tests but instead write them out from scratch each time.

I found it quite difficult to start with as the temptation is always there to just grab a similar test and tweak it slightly to test the new bit of functionality but an interesting early observation was that we were far less accepting of duplication in tests when we had to type out that duplication each time than if we had just copied and pasted it.

I tend to extract method quite frequently when I see test code which is similar but we were seeing potential areas for extracting out common test code which I would never have spotted.

There’s a chapter in Steve Freeman and Nat Pryce’s book ‘Growing Object Oriented Software, guided by tests‘ titled ‘Listening to the tests‘ which I quite like and I think we become much more aware of the tests if we have to write them out each time.

An example of this which Matt spotted was that in one bit of test code we were working on there was a method called ‘StubOutSomeRepository’ which was called by every single test and I instinctively made the call to that method as the first line in our new test.

Matt questioned whether it was actually even needed so we removed it from every single test in that test fixture and they all still worked!

I’m pretty sure that we wouldn’t have spotted that problem if we’d just copy/pasted and I’m also fairly sure that all the other tests in that test fixture were copy/pasted from the first test written in the test fixture when there may actually have been a need to stub out the dependency.

Apart from enabling us to simplify our tests I think that writing out our tests each time keeps us thinking – it’s really easy to turn off and work on auto pilot if you’re just copy/pasting because it’s essentially mindless and doesn’t engage the mind that much.

Keeping in the thinking mindset allows us to see whether we’re actually testing properly and I think it also makes the pairing process a bit more interesting since you now have to talk through what you’re doing with your pair.

One of the other reasons I thought copy/pasting might be useful sometimes is that it can save time if we’re typing out similar types of tests repeatedly and I often feel that it would be quite annoying for my pair to watch me do this.

Now I’m not so convinced by my own argument and I think that the time that we save by writing more readable tests probably easily outweighs any short term time gains.

Our conversations have become more about working out ways that we can

On a related note I recently watched a video of a performance kata by Corey Haines where he works through a problem guided by tests and as far as I remember doesn’t ever copy and paste a test but instead looks for ways to reduce the duplication he has in his test code so that he doesn’t have to do too much typing for each test.

During J.B. Rainsberger’s presentation at Agile 2009 titled ‘Integration tests are a scam‘ he suggests that having lots of integrationt tests covering our code can give us a false sense of confidence that we are testing our code and I think the same can happen with unit tests as well if we’re not careful how we write them.

It’s important to ensure that our unit tests are actually testing something useful otherwise the cost of writing and maintaining them will outweigh the benefits that we derive from doing so.

I’ve come across two type of test recently which don’t test an awful lot but can lead us to thinking our code is well covered at the unit level if we just glance at the test names and don’t take the time to examine exactly what’s going on which is what seems to happen a lot of the time.

Only testing for ‘IsNotNull’

I think testing that a value we want to do some assertions against isn’t null is a great start since it can help us avoid null reference exceptions in our tests but it’s only the first assertion that we want to make, not the only one!

The name of these tests is nearly always misleading and it often seems like these tests are a work in progress but one which never gets finished.

The following is not unusual:

[Test]
public void ShouldPopulateParentModelOnAction()
{
	var someController = new SomeController(...);
 
	// There'd probably be more casting to actually retrieve the actual model 
	// but we'll ignore that for the sake of this example
	var parentModel = someController.SomeAction();
 
	Assert.IsNotNull(parentModel);
}

The problem I have with this type of test is that the name suggests we have completely tested that the controller action is correctly populating the model but in reality all we’re testing is that we got a model and not that its contents are accurate.

It’s surprisingly easy when skimming through tests to not even notice that this is the case.

The danger we’re toying with by doing this is that others might assume that this area of the code is tested and subsequently do refactorings around this code assuming that the tests are providing a safety net when in actual fact they aren’t.

Certainly sometimes ‘IsNotNull’ is all we care about and in those cases it’s fine just to test for that but I think that most of the time it’s just a lazy way out of properly testing a piece of code.

Testing expectations on stubs

I’m not sure if this is just a Rhino Mocks problem but I’ve come across quite a few tests recently which make expectations on stubs and then sometimes verify those expectations.

Most of the time the creation of the stubbed dependency is hidden away in a setup method so the tests wouldn’t be quite as clear cut as this but this is what’s happening:

[Test]
public void ShouldRetrieveSomeValuesFromTheRepository()
{
	var theRepository = MockRepository.GenerateStub<ITheRepository>();
	theRepository.Expect(r => r.SomeMethod()).Return("someValue");
 
	var systemUnderTest = new SystemUnderTest(theRepository);
 
	systemUnderTest.DoSomething();
}

or

[Test]
public void ShouldRetrieveSomeValuesFromTheRepository()
{
	var theRepository = MockRepository.GenerateStub<ITheRepository>();
	theRepository.Expect(r => r.SomeMethod()).Return("someValue");
 
	var systemUnderTest = new SystemUnderTest(theRepository);
 
	systemUnderTest.DoSomething();
 
	theRepository.VerifyAllExpectations();
}

The problem with both of these tests is that they aren’t doing anything useful for us.

From my understanding of the Rhino Mocks code what we are effectively doing is creating a stub on ‘theRepository’ which will return a value of ‘someValue’ the first time that ‘SomeMethod’ is called on it. The ‘VerifyAllExpectations just falls through and does nothing because we’ve created a stub and not a mock.

We could just as easily not call any method on the system under test and we’d still get a green bar.

If we intend to test the way that the system under test interacts with its dependencies then we want to make sure we’ve actually created a mock and then ensure that we check that the expectations have actually been called.

The test above could become a bit more like this for that purpose:

[Test]
public void ShouldRetrieveSomeValuesFromTheRepository()
{
	var theRepository = MockRepository.GenerateMock<ITheRepository>();
	theRepository.Expect(r => r.SomeMethod()).Return("someValue");
 
	var systemUnderTest = new SystemUnderTest(theRepository);
 
	systemUnderTest.DoSomething();
 
	theRepository.VerifyAllExpectations();
}

In summary

I’ve previously just ignored tests like this and just got on with what I was doing but I think it’s important to try and clean them up so that they can make a greater contribution to our testing efforts – Uncle Bob’s Boy Scout Rule applies here – leave the tests in a better state than you found them.

That way we can move towards having justifiable confidence that we can make changes to our code without fear that we’ve broken some piece of functionality.

While the test data builder pattern is quite a useful one for simplifying the creation of test data in our tests I think we need to be quite careful when using it that we don’t lose the intent of the test that we’re writing.

The main advantage that I see with this pattern is that by using it we can provide default values for properties of our objects which aren’t important for the bit of functionality that we’re currently testing but which need to be provided otherwise the test can’t actually be run.

In order for our tests to express their intent clearly it is still important to explicitly set the values that we care about for the current test otherwise it will become quite difficult to work out why a test is failing when it does.

For example I recently came across a failing test similar to this:

[Test]
public void ShouldTestSomeCoolStuff()
{
	var foo = new FooBuilder().Build();
 
	var yetAnotherObject = someOtherObject.ThatTakesIn(foo);
 
	Assert.AreEqual("DEFAULTBAR", yetAnotherObject.Bar);
	Assert.AreEqual("DEFAULTBAZ", yetAnotherObject.Baz)	
}

It was failing because the actual values being returned were ‘defaultBar’ and ‘defaultBaz’ which initially made us wonder if there was some capitalisation going wrong in one of our objects.

As it turned out the values being returned were just the default ones from the ‘FooBuilder’ and we were asserting the wrong thing:

public class FooBuilder
{
	private string bar = "defaultBar";
	private string baz = "defaultBaz";
 
	public FooBuilder Bar(string value)
	{
		bar = value;
		return this;
	}
 
	public FooBuilder Baz(string value)
	{
		baz = value;
		return this;
	}
 
	public Foo Build()
	{
		return new Foo(bar, baz);
	}
}

While it didn’t take us too long to get to the cause of the problem I think it would have made our lives easier if we’d been able to tell why it was failing just from looking at the test instead of having to look in a couple of different classes to figure it out.

A test written more like this would help us to achieve that:

[Test]
public void ShouldTestSomeCoolStuff()
{
	var foo = new FooBuilder().Bar("myBar").Baz("myBaz").Build();
 
	var yetAnotherObject = someOtherObject.ThatTakesIn(foo);
 
	Assert.AreEqual("myBar", yetAnotherObject.Bar);
	Assert.AreEqual("myBaz", yetAnotherObject.Baz)	
}

Then if we want to remove duplication we can just put the expected values into more descriptive variables:

[Test]
public void ShouldTestSomeCoolStuff()
{
	var expectedBar = "myBar";
	var expectedBaz = "myBaz";
	var foo = new FooBuilder().Bar(expectedBar).Baz(expectedBaz).Build();
 
	var yetAnotherObject = someOtherObject.ThatTakesIn(foo);
 
	Assert.AreEqual(expectedBar, yetAnotherObject.Bar);
	Assert.AreEqual(expectedBaz, yetAnotherObject.Baz)	
}

There’s a bit more test code than in the first example but if the test fails again we should be able to work out why more quickly than before because we are now more explicit about which values are actually important for this test.

In a post I wrote earlier in the week I described a dilemma we were having testing some code which made use of abstract classes and Perryn Fowler, Liz Keogh and Pat Maddox pointed out that a useful approach for this problem would be to make use of an abstract test class.

The idea here is that we create an equivalent hierarchy to our production code for our tests which in the example that I provided would mean that we have roughly the following setup:

public abstract class ParentModelTest
{
}
 
public class BusinessProcess1ModelTest : ParentModelTest { }
public class BusinessProcess2ModelTest : ParentModelTest { }
public class BusinessProcess3ModelTest : ParentModelTest { }

We then want to create an abstract method on the ‘ParentModelTest’ class to create the object under test and we’ll implement this method in the sub classes.

We can then put the common tests into the abstract class and they will be run when we run the tests for each of the sub class tests.

My colleague Matt Dunn and I were looking at how you would implement this and he pointed out that it provided quite a nice opportunity to use generics to achieve our goal.

If we make the abstract test class take in generic parameters then it allows us to create our specific type in the ‘create’ method rather than having to create ‘ParentModel’ and then casting that for our specific sub class tests.

public abstract class ParentModelTest<T> where T : ParentModel
{
	protected abstract T CreateModel();	
 
	[Test]
	public void ShouldTestThoseCommonDependencies()
	{
		var model = CreateModel();
		// Do some awesome testing that's common across all 3 sub classes here
	}
}

[TestFixture]
public class BusinessProcessModel1Test : ParentModelTest<BusinessProcessModel1>
{
	protected override BusinessProcessModel1 CreateModel()
	{
		return new BusinessProcessModel1(...);
	}
 
	[Test]
	public void ShouldDoSomeEquallyOutstandingStuff() 
	{
		var model = CreateModel();
		// and test away
	}
}

It seems to work quite well and we realised that it’s actually a pattern that we had also used to test that our controllers would actually be injected with all their dependencies from our DI container or whether we have forgotten to register some of them.

We have an abstract class similar to this which all our controller tests extend:

public abstract class ContainerTest<T> where T : Controller
{
	[Test]
	public void ShouldHookUpAllDependencies()
	{
		var container = CreateTheContainer();
 
		var controller = container.Resolve<T>();
 
		Assert.IsNotNull(controller);
	}
}

public class AwesomeControllerTest : ContainerTest<AwesomeController> { }

When we haven’t got all the dependencies injected correctly the code will actually blow up on the resolve step so the last line is not strictly necessary but the test seems like it’s not testing anything at first glance without it.

We ran into another interesting testing dilemma while refactoring the view model code which I described in an earlier post to the point where we have an abstract class and three sub classes which means that we now have 3 classes which did the same thing 80% of the time.

As I mentioned in a post a couple of weeks ago one of the main refactorings that we did was to move some calls to dependency methods from the constructor and into properties so that those calls would only be made if necessary.

After we’d done this the code looked a bit like this:

public abstract class ParentModel
{
	private readonly Dependency1 dependency1;
 
	...
 
	public decimal Field1 { get { return dependency1.Calculation1(); } }
	public decimal Field2 { get { return dependency1.Calculation2(); } }
}
 
public class BusinessProcess1Model : ParentModel { }
public class BusinessProcess2Model : ParentModel { }
public class BusinessProcess3Model : ParentModel { }

We wanted to ensure that the tests we had around this code made sure that the correct calls were made to ‘depedency1′ but because ParentModel is an abstract class the only way that we can do this is by testing one of its sub classes.

The question is should we test this behaviour in each of the sub classes and therefore effectively test the same thing three times or do we just test it via one of the sub classes and assume that’s enough?

Neither of the options seems really great although if we cared only about behaviour then we would test each of the sub classes independently and forget that the abstract class even exists for testing purposes.

While the logic behind this argument is quite solid we would end up breaking 3 tests if we needed to refactor our code to call another method on that dependency for example.

I suppose that makes sense in a way since we have actually changed the behaviour of all those classes but it seems to me that we only really need to know from one failing test that we’ve broken something and anything beyond that is a bit wasteful.

In C# it’s not actually possible for ‘Field1′ or ‘Field2′ to be overriden with an alternate implementation unless we defined those properties as ‘virtual’ on the ‘ParentModel’ which we haven’t done.

We could however use the ‘new’ keyword to redefine what those properties do if the callee had a reference directly to the sub class instead of to the abstract class which means it is possible for a call to ‘Field1′ to not call ‘dependency1′ which means that maybe we do need to test each of them individually.

I’m not sure which approach I prefer, neither seems better than the other in my mind.