Is the test wrong – Is there a simpler test

Every so often when following TDD we struggle to make a test go green or we struggle to make a test go green in way that feels right. When this happens it can mean there is a problem with the test. Whenever I feel like I'm getting lost, I start by asking two simple non exclusive questions:

  • Is the test wrong?
  • Is there a simpler test?

This suggests there are four possible directions in which we can go when we encounter such difficulties:

is-the-test-right-is-there-a-simpler-test-2x2

I am slowly trying to introduce Code Katas , Wasas and Randoris to my colleagues. In our most recent Randori session, we tried implementing a List in Java (without using the Collections framework ). Everything was going smoothly, the rules of TDD were being adhered to and the code had built up quickly. We had addressed size, add, and get, the next bit of behaviour we aimed for was addAll. The first addAll test looked like this:

	@Test
	public void shouldAddAll() {
		List otherList = new List();
		otherList.add("1");
		list.addAll(otherList);
	}

At this point we had a failing test because the addAll method didn't yet exist, and so control moved to the next coder. They added the addAll method, then added the following to the shouldAddAll test:

	@Test
	public void shouldAddAll() {
		List otherList = new List();
		otherList.add("1");

		list.addAll(otherList);
		
		assertThat(list, equalTo(otherList));
	}

The test failed. We had not yet overridden the equals method therefore equality was defined by Object.equals(Object obj) as "true if this object is the same as the obj argument". Unlike the previous test failure, this wasn't a compilation failure, this was a behavioural failure. The equals method did not behave as required in order that the test could pass.

We were running short of time, rather than swap coder, we opted to discuss what we would do. I started the discussion by asking why the coder had opted to use the equals method. They stated that the objects should be considered equal and it was nice and clean because there only needed to be one assertion. My next question was how would anyone around the table fix the test. There were essentially two responses. (1) Some reasoned that adhering to the rules of TDD meant they would add the equals method and hard code it to return true. (2) The coder who had added the assertion felt the answer was to use the IDE to generate the equals and hashcode methods.

Creating the equals method and hard coding it to return true seemed legitimate, in that it would adhere to the rules of TDD. However, it didn't really feel like it was in the spirit of the test we were trying to fix. It meant we would not be implementing the behaviour the test was really intended to test for. Perhaps the test is wrong because it is forcing us to do something we don't yet want to do.

Generating the equals and hashcode methods using the IDE would break the rules of TDD because we would be writing more code than was required to fix the test (even if we were being helped by the IDE). I pointed out we would not have very convincing code coverage and we would be risking regressions in our equals method. They conceded that this was true, and then admitted the reason they wanted to generate the equals and hashcode methods was because they didn't want to break the equals hashcode contract . Perhaps there is a simpler test we can solve, for example testing equality.

As far as th e equals hashcode contract is concerned we should expect the implementation of these methods to occur a little bit at time (a little test, a little code, etc). When we write code we can never expect to write the entire solution in one go. Every time we reach the end of the Red-Green-Refactor cycle we shouldn't necessarily expect our code to be complete; not until we have defined a reasonable minimum behaviour can we expect to have a solution that is in any form of usable state.

It was clear something had gone awry, at that point of fixing the shouldAddAll test we should have been interested in the addAll behaviour, instead we were interested in the equals behaviour. Thinking about those two questions, here are the possible ways I can think it could be argued.

The test is right and there is no simpler test. The test makes sense, and it should just add everything from the other list. Therefore we should fix it in the simplest way possible (add the equals method and always return true).

The test is wrong. The assertion is based on the assumption we have a suitably defined equals method, we do not. Therefore we should change the test (use size and get to check the state of the list).

There is a simpler test. A simpler test would not need to check for equality. Therefore the next simplest test is to test addAll where there are no elements in the otherList.

The test is wrong and there is a simpler test. The assertion is based on the assumption we have a suitably defined equals method, we do not, and a simpler test would not need to check for equality. Therefore the next simplest test is to test addAll where there are no elements in the otherList. Then we can come back and change this test (use size and get to check the state of the list).

The simpler test identified above definitely needs addressing first. It will look something like this:

	@Test
	public void shouldAddAllGivenOtherListIsEmpty() {
		List otherList = new List();
		list.addAll(otherList);
		assertThat(list.size(), equalTo(0));
	}

That's where we should have started. Now we remove the @Ignore annotation from the original shouldAddAll test , and it is still plagued by the fact it is trying to use the equals method in the assertion. All we've done is skirted around the issue. Let's ask those two questions again.

Is there a simpler test? As far as addAll is concerned, ignoring null parameters, probably not.

Is the test wrong? Conceptually no. But the test could be more explicit.

That 'but' is interesting. Remember that one of the benefits of TDD is that it gives us reliable low level documentation. At the moment our test indicates that when you use addAll the two lists will be equal. That's confusing, sure in the very specific scenario we have written they will be equal, but that's not what we want to convey to someone else reading our code. We want to convey that the elements from the otherList are now present in the list it has been added to. I think the test should be rewritten to look like this:

	@Test
	public void shouldAddAllGivenOtherListHasOneElement() {
		List otherList = new List();
		String element = "1";
		otherList.add(element);

		list.addAll(otherList);
		
		assertThat(list.size(), equalTo(1));
		assertThat(list.get(0), equalTo(element));
	}

Obviously this would occur in more than one Red-Green-Refactor cycle because we would have to fix the test after writing the first assert.

I forget the exact quote, but it goes something like this "we write code to tell other programmers what we want the computer to do". I think the rewritten test achieves this more effectively than the original. If we wanted to restrict the assertion section to a single assert we could use a matcher along the lines of assertThat(list, onlyContains(element)). Personally, I don't see the need.

TL;DR

Whenever things start going wrong and you're using TDD ask yourself these two simple non exclusive questions:

  • Is the test wrong?
  • Is there a simpler test?

Sometimes the answers are clear, and a path will present itself. Other times the answers you get aren't so black and white. If something isn't clear, explore it, it may be what's causing the problem.

Tweet about this on TwitterShare on FacebookShare on LinkedInShare on Google+Share on RedditEmail this to someone
Posted in Test Driven Development