Test Smells: Cleaning up Unit Tests

In practical terms, knowing how not to write code might be as important as knowing how to write it. This goes for test code, too; and today, we're going to look at common mistakes that happen when writing unit tests.

Although writing unit tests is common practice for programmers, tests are still often treated as second-class code. Writing good tests isn't easy — just as in any programming field, there are patterns and anti-patterns. There are some really helpful chapters on test smells in Gerard Meszaros's book about xUnit patterns — and more great stuff around the internet; however, it's always helpful to have practical examples.

Here, we're going to write one unit test quick and dirty, and then improve it like we would in our work. The full example is available on GitHub.

One Test's Evolution

To begin with, what are we testing? A primitive function:

public String hello(String name) {
	return "Hello " + name + "!";
}

We begin writing a unit test for it:

@Test
void test() {

}

And just like that, our code already smells.

1. Uninformative Name

Naturally, it's much simpler to just write test, test1, test2, than to write an informative name. Also, it's shorter!

But having code that is easy to write is much less important than having code that is easy to read - we spend much more time reading it, and bad readability wastes a lot of time. A name should communicate intent; it should tell us what is being tested.

@Test
void shouldReturnHelloPhrase() {
	assert(hello("John")).matches("Hello John!");
}

Nobody (apart from the framework) is going to call the test method directly, so it's not a problem if the name seems too long. It should be a descriptive and meaningful phrase (DAMP).

2. No arrange-act-assert

The name is okay, but now there is too much code stuffed into one line. It's a good idea to separate the preparation, the behavior we're testing, and the assertion about that behavior (arrange-act-assert).

Like this:

@Test
void shouldReturnHelloPhrase() {
    String a = "John";

    String b = hello("John");

    assert(b).matches("Hello John!");
}

In BDD, it's customary to use the Given-When-Then pattern, and in this case, it's the same thing.

3. Bad Variable Names and No Variable Re-Usage

But it still looks like it's been written in a hurry. What's "a"? What's "b"? You can sort of infer that, but imagine that this is just one test among several dozen others that have failed in a test run (perfectly possible in a test suite of several thousand tests). That's a lot of inferring you have to do when sorting test results!

So — we need proper variable names.

Something else we've done in a hurry — all our strings are hard-coded. It's okay to hard-code some stuff — only as long as it's not related to other hard-coded stuff!

Meaning, that when you're reading your test, the relationships between data should be obvious. Is "John" in 'a' the same as "John" in the assertion? This is not a question we should be wasting time on when reading or fixing the test.

So we rewrite the test like this:

@Test
void shouldReturnHelloPhrase() {
    String name = "John";

    String result = hello(name);
    String expectedResult = "Hello " + name + "!";

    assert(result).contains(expectedResult);
}

4. The Pesticide Effect

Here's another thing to think about: automated tests are nice because you can repeat them at very little cost — but that also means their effectiveness falls over time because you're just testing the exact same thing over and over. That's called the pesticide paradox (a term coined by Boris Beizer back in the 1980s): bugs build resistance to the thing you're killing them with.

It's probably not possible to overcome the pesticide paradox completely — but there are tools that reduce its effect by introducing more variability into our tests, for instance, Java Faker. Let's use it to create a random name:

@Test
void shouldReturnHelloPhrase() {
    Faker faker = new Faker();
    String name = faker.name().firstName();

    String result = hello(name);
    String expectedResult = "Hello " + name + "!";

    assert(result).contains(expectedResult);
}

Good thing we've changed the name to a variable in the previous step — now we don't have to look over the test and fish out all the "Johns."

5. Uninformative Error Messages

Another thing we've probably not thought about if we've written the test in a hurry — is the error message. You need as much data as possible when sorting test results, and the error message is the most important source of information. However, the default one is pretty uninformative:

java.lang.AssertionError at org.example.UnitTests.shouldReturnHelloPhrase(UnitTests.java:58)

Great. Literally the only thing this we know is that the assertion hasn't passed. Thankfully, we can use assertions from JUnit's `Assertions` class. Here's how:

@Test
void shouldReturnHelloPhrase4() {
    Faker faker = new Faker();
    String name = faker.name().firstName();

    String result = hello(name);
    String expectedResult = "Hello " + name + "";

    Assertions.assertEquals(
        result,
        expectedResult
    );
}

And here's the new error message:

Expected :Hello Tanja! Actual :Hello Tanja

...which immediately tells us what went wrong: we've forgotten the exclamation mark!

Lessons Learned

And with that, we've got ourselves a good unit test. What lessons can we glean from the process?

A lot of the problems were caused by us being a bit lazy. Not the good kind of lazy, where you think hard about how to do less work. The bad kind, where you follow the path of least resistance, to just "get it over with."

Hard-coding test data, doing cut and paste, using "test" + method name (or "test1", "test2", "test3") as the name of the test are marginally easier to do in the short run, but make the test base much harder to maintain.

On the one hand, it is a bit ironic that we've been talking about readability and making tests easier on the eyes, and at the same time turned a 1-line test into 9 lines. However, as the number of tests you're running grows, the practices we're proposing here will save you a lot of time and effort.