Clean tests, Part 2: Comments
In the first part we looked at the names of our tests. I recommended to neither start your test with the word test nor use the method you are calling as part of the name. The tests are your specification, that means write them as human readable as possible. But now let’s forget about names and discuss comments.
Given-When-Then
Let’s check the implementation again of the example taken from Toothpick project:
//GIVEN
Scope parentScope = Toothpick.openScope(“root”);
Scope childScope = Toothpick.openScopes(“root”, “child”);//WHEN
Scope scope = childScope.getParentScope(Singleton.class);//THEN
assertThat(scope, is(parentScope));
This Given-When-Then, also known as Arrange-Act-Assert, is a very common pattern. But every time I see it like this, shivers run down my spine.
Why? Because every time you need to explain your code with a comment you should stop, take a deep breath and think about what you just did!
When it comes to non testing code, many readers would probably agree here but this rule also applies to tests!
Reduce the noise
What makes it worse for me, is that this comment pattern repeats for every single test! Three(!) lines of comments plus newlines for every single test!? Just the pure amount of code we need to write should make you want to scream out loud. You might template this but it’s still the same ugly outcome.
In case you want to disagree here, wait. Let’s forget a moment what I just said and think about an ideal test.
The ideal test
I think the ideal test has one line!
Could be something like:
assertTrue(tested.isTrue());Following the pattern we saw earlier, this would we need to split this into two lines + all the comments including an empty one:
//GIVEN//WHEN
boolean result = tested.isTrue();//THEN
assertTrue(result);
Is this truly better? Not at all. There is just too much noise in there.
You might want to remove the empty block but that does destroy the whole idea of a strict order. You are sabotaging your own rule.
But I admit that this test is a bit trivial. So let’s look at another example:
ImportantThing result = tested.getTheThing()assertTrue(result.isTrue())
Would this test read better by adding multiple lines of comments?
I don’t think so, it can be read pretty nice!
Now you might say that most tests are more complicated than this.
Right!
Actually I disagree once more. Small test should make a lot of our tests base: Simple: two-liners!
Large tests smell
What is in your tests that makes you need to separate the code into three blocks? It sounds your test is too complex!
The middle (act or when) part should be one line, as we will call one method.
The assertion or then is one line as well. If it has more assertions we should extract all of them into one method.
This leaves us with the setup, the given part. If we need to setup a lot of things, this is a code smell that the method we are calling does more than just one thing! A complex test indicates that the implementation is too complex, it does too many things! Maybe its time to refactor, afterwards your test might be easier.
On the other a short method needs neither explanations nor separations.
Setting your test up
Of course there is code you need to setup for the object under test. But that code should go into the setUp() method!
We should try to push most of our code there, even if that code is only needed by some of the tests. Why?
Because no one will read your setUp() method!
But you and/or other developers will read your test!
Keep the tests clean!
Don’t explain what is obvious
I admit, it is nice to have GIVEN-WHEN-THEN order. But wait, you have that anyway! How would you build any other order in a test? Try! You can not assert before you did something! And it would make no sense to setup your tests after the assertion either. So the order is clear anyway.
Two line tests are basically a WHEN and a THEN line, always!
Three line tests are probably GIVEN and WHEN and THEN.
Maybe keep some new lines to separate them.
So don’t comment what is obvious anyway.
Don’t shoot yourself in the foot
Let me show you another examples that shows the weird path this leads to:
@Test(expected = NullPointerException.class)
public void should_throw_NPE() {
// WHEN
tested = new Something(null);
}As the expected exception needs to be put in the annotation the chain is broken, so sticking to the GIVEN-WHEN-THEN rule does not make any sense here.
Assertions
If you find the last part of the test, the assertion code, hard to read then don’t fix with a comment but use a better assertion library.
So instead of
assertEquals(3, list.size());you can write with fest assertions
assertThat(list.size()).isEqualTo(3);or with Hamcrest Matchers:
assertThat(list, hasSize(3));Now we talked a full episode just about comments. But we removed a lot of noise from our test. And there is more, stay tuned for part three!
