Mark Needham

Thoughts on Software Development

TDD: Combining the when and then steps

with 7 comments

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.

Be Sociable, Share!

Written by Mark Needham

November 14th, 2009 at 12:17 am

Posted in Testing

Tagged with

  • http://bryancook.net Bryan Cook

    I had the exact same argument with a developer the other day. Concise, elegant tests are great, but there comes a point where abstracting away test details limits our ability to understand the structure of the code itself.

  • http://www.planetgeek.ch Daniel Marbach

    I would prefer the following structure:

    @Test
    public void WhenOriginalValueAndNewValueAreEqualCalculatedPercentageMustBeZero() {
    const int originalValue = 100;
    const int newValue = 100;
    const int expectedPercentageDifference = 0;

    PercentageCalculator testee = this.createTestee();

    assertEquals(expectedPercentageDifference ,testee.calculatePercentage(originalValue, newValue)
    }

    @Test
    public void WhenOriginalValueIsHalfOfNewValueCalculatedPercentageMustBeOneHundred() {
    const int originalValue = 50;
    const int newValue = 100;
    const int expectedPercentageDifference = 100;

    PercentageCalculator testee = this.createTestee();

    assertEquals(expectedPercentageDifference ,testee.calculatePercentage(originalValue, newValue)
    }

    and so one…

    private PercentageCalculator createTestee() {
    return new PercentageCalculator();
    }

  • Pingback: Reflective Perspective - Chris Alcock » The Morning Brew #477

  • http://joshribakoff.com Josh Ribakoff

    I disagree with you, test code duplication is bad and causes all the same problems it causes in production, however unlike production code it is not as high of a priority, there is a higher priority which is tests as documentation (which is why it should have been moved in your example).

    If you are duplicating data that does not affect the outcome of the test, then you move it out of the test method so we say to the reader “the value you do not see do not affect the outcome values” – XUnit Testing Patterns

  • http://joshribakoff.com Josh Ribakoff

    You should also factor out duplication into creation methods that accept as parameters, values that are relevant to the outcome of that particular test.

  • http://joshribakoff.com Josh Ribakoff

    Take for example

    [code]
    function testListAll()
    {
    $this->createMMY();
    $test1_id = $this->insertModel( $this->make_id, 'Y' );
    $test2_id = $this->insertModel( $this->make_id, 'Z' );
    $model = $this->findModelById( $this->model_id );
    $expected = array();
    $expected[0] = $this->findModelById( $this->model_id );
    $expected[1] = $this->findModelById( $test1_id );
    $expected[2] = $this->findModelById( $test2_id );
    $this->assertEquals( $expected, $model->listAll(), 'should return exactly the right items, in alphabetical order, and nothing more' );
    }
    [/code]

    Becomes…

    [code]
    function testListAll()
    {
    // fixture
    list( $inserted_id1, $inserted_id2 ) = $this->createMMYWithAddtModels();
    // SUT
    $model = $this->findModelById( $this->model_id );
    // verify!
    $expected = $this->getEntities( $this->model_id, $inserted_id1, $inserted_id2 );
    $this->assertEquals( $expected, $model->listAll(), 'should return exactly the right items, in alphabetical order, and nothing more' );
    }
    [/code]

  • http://www.markhneedham.com Mark Needham

    That seems fine to me – it’s still possible to understand what’s going on in the test just from reading it and you haven’t pulled out too much of the test into other methods.