How to write good tests

Writing tests is pretty easy once you've got the hang of your favourite testing library, but it's surprisingly difficult to grow and maintain a test suite.

Tests, like a lot of production code, often begin well but get unwieldy as your codebase grows. Without good maintenance, eventually even small changes require you to rewrite all the tests.

One of the best ways to stay on top of a test suite is to structure your tests well. This is what I consider to be the best way to write a standard unit test, with a breakdown behind some of my reasoning. We're going to use Ruby, but these rules apply generally, at least to all languages and frameworks I've encountered.

The subject

class Person 
    attr_reader :first_name, :last_name 

    def initialize(first_name, last_name) 
       @first_name = first_name 
       @last_name = last_name
    end

    def full_name
        "#{first_name} #{last_name}"
    end

    def in_house_identifier
        "#{last_name.upcase.reverse}"
    end
 end

This is the thing we're trying to test. It's a simple Person class to help us format the person's name. We have a method to grab their full name, as well as an in-house identifier which is just a transformed version of their last name.

The tests

require 'faker'

describe Person do 
    it 'create a new person with a first and last name' do       
        person = build_person(first_name:'Joanna', last_name: 'Smith')
        expect(person).to have_attributes(first_name: 'Joanna') 
        expect(person).to have_attributes(last_name: 'Smith') 
    end 

    it 'provides the full name' do       
        person = build_person(first_name: 'Henrietta', last_name: 'Appleseed')
        expect(person.full_name).to eq('Henrietta Appleseed')
    end

    it 'reverses the surname in upper-case for the in-house identifier' do
        person = build_person(last_name: 'Thompson')
        expect(person.in_house_identifier).to eq('NOSPMOHT')
    end
end

def build_person(first_name: Faker::Name.first_name, last_name: Faker::Name.last_name)
   Person.new first_name, last_name
end

These might not be totally comprehensive, but this is a great start. So what makes them so good?

1. No shared state

it 'create a new person with a first and last name' do
    person = build_person(first_name:'Joanna', last_name: 'Smith')
end

it 'provides the full name' do
    person = build_person(first_name: 'Henrietta', last_name: 'Appleseed')
end

it 'reverses the surname in upper-case for the in-house identifier' do
    person = build_person(last_name: 'Thompson')
end

We want to be able to make changes without breaking the other tests. This means that we need a fresh Person for every test. Some testing libraries offer a way of ensuring this but still allowing you to have shared state, like so:

describe Person do 
    before(:each) do
        # Re-create our person before each test
        @person = build_person(first_name:'Joanna', last_name: 'Smith')
    end

    it 'create a new person with a first and last name' do
        # Use @person to reference it       
        expect(@person).to have_attributes(first_name: 'Joanna') 
        expect(@person).to have_attributes(last_name: 'Smith') 
    end
end

So what's wrong with this? While these tests are independent, as you can be sure you have a fresh person every test, we've still coupled our tests together and made them harder to read.

I must now read both the before-each block to understand how the test works, meaning I'm now scrolling between them regularly. In a file which can stretch to hundreds (if not thousands) of lines this becomes unreasonable, and there isn't
a clean split between them. As a reader, I'll likely arrive at this file with no context and need to fix a single failing test. I'll This is suddenly a much more painful job.

This also means all my tests are relying on the same example data: Joanna Smith. If my test required a longer surname, I now need to edit all the tests in the file.

2. The builder method

it 'reverses the surname in upper-case for the in-house identifier' do
    person = build_person(last_name: 'Thompson')
    expect(person.in_house_identifier).to eq('NOSPMOHT')
end

def build_person(first_name: Faker::Name.first_name, last_name: Faker::Name.last_name)
    Person.new first_name, last_name
end

While we don't want shared state, we also want to reduce the amount of boilerplate. Ideally, we want the test to only talk about what's needs setting up to make it pass. We can achieve this by using a a builder method and setting some default arguments.

In the example above, the in-house identifier only relies on the last name, so we don't bother passing in a first name at all. Since we don't use the first name to make the test pass, it shows readers we don't care what it is.

We can take this even further with Faker.

3. Using Faker

require 'faker'

Faker::Name.first_name
# Danielle

Faker::Name.last_name
# Smith

Faker is a small detail that really helps prevent bad habits. With builder methods, like the one above, it's easy to set the default values to some known good values and never bother overriding them in the tests, like so:

it 'reverses the surname in upper-case for the in-house identifier' do
    # No arguments given
    person = build_person()
    expect(person.in_house_identifier).to eq('NOSPMOHT')
end

# Everyone we make is called Samuel Thompson by default
def build_person(first_name: 'Samuel', last_name: 'Thompson')
    Person.new first_name, last_name
end

This is basically re-inventing the before-each block we discussed earlier. By sharing important details, like the person's name, outside of the test we are making it harder for readers to know which arguments are relevant to this
particular test.

Using Faker, we make these default arguments random, so we can't rely on them. In order to assert on a value, we need to override it in the test, which confirms to readers what's relevant.

A note about using Faker is that it's very easy to misuse by using them as part of your assertions.

it 'reverses the surname in upper-case for the in-house identifier' do
    # Get a random name each time
    person = build_person()
    # We convert the name here and check they're the same
    expect(person.in_house_identifier).to eq(person.last_name.upcase.reverse)
end

def build_person(first_name: Faker::Name.first_name, last_name: Faker::Name.last_name)
    Person.new first_name, last_name
end

I avoid this as it makes tests harder to debug, for example, is Lee their first name or last name? We don't know, since we didn't explicitly set what we were testing.

I generally try to avoid unnecessary variables altogether.

4. No unnecessary variables

it 'create a new person with a first and last name' do       
    person = build_person(first_name:'Joanna', last_name: 'Smith')
    # Hard-coded Joanna
    expect(person).to have_attributes(first_name: 'Joanna')
    # Hard-coded Smith
    expect(person).to have_attributes(last_name: 'Smith') 
end 

It's tempting to create a variable for the first and last names. It allows you to quickly change the variable to something else, but it offers little for readability.

The more logic that goes into a test the harder it is to understand. If someone has to compile the test in their head, they'll struggle to make changes to it.