Martijn

Martijn Versluis – 27 September 2018
812 words in about 4 minutes

Every (more experienced) developer will tell you that a good codebase does not include duplicate code (is “DRY”) and has details abstracted away to a certain extent. Often when I read tests I can recognise those rules being applied there too.

However, in my opinion those rules do not apply to tests. A good test can be read from start to end like a specification, just like an interaction designer would write down the specifications for a user story. You could even read an application’s tests to understand what it does, as if it was documentation.

Imagine this model test for a User model.

NB: if you are not familiar with testing in Ruby/Rails: let generates a “global” variable within its scope.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
RSpec.describe User do
  let(:user) { MyTestFactory.build_user }
  
  # ... lots of other specs
  
  describe 'validation' do
    before do
      MyTestFactory.build_and_persist_user
    end
  
    it 'validates uniqueness of nickname' do
      expect(user.save).to be false
    end
  end
end

The test seems to verify if a uniqueness constraint of the nickname is working correctly. This test will work fine. But does it show you the specifications of the feature in one glance?

If it was the specification for a user story I would probably look something like this:

When I create a user with the nickname “Johnny12” And I try to create another user with the nickname “Johnny12” It should fail And it should explain to me that the nickname “Johnny12” is already taken

Let’s see if you can make the test look like the specification above.

It turns out that the factory you use always takes the same nickname for a user it generates. Maybe you should explicitly set the nickname to clarify that. (Note: this could be dangerous, other tests might depend on the user having a different nickname. You will solve this later on.)

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
RSpec.describe User do
  let(:user) { MyTestFactory.build_user(nickname: 'Johnny12') }. # <== changed
  
  # ... lots of other specs
  
  describe 'validation' do
    before do
      MyTestFactory.build_and_persist_user(nickname: 'Johnny12')            # <== changed
    end
  
    it 'validates uniqueness of nickname' do
      expect(user.save).to be false
    end
  end
end

Furthermore, inside the before block you create a user to simulate the situation where there is an existing user with the nickname you try to use. Let’s give that user a name. Use an underscore as prefix to indicate you will not actually use the variable.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
RSpec.describe User do
  let(:user) { MyTestFactory.build_user(nickname: 'Johnny12') }.
  
  # ... lots of other specs
  
  describe 'validation' do
    before do
      _existing_user = MyTestFactory.build_and_persist_user(nickname: 'Johnny12') # <== changed
    end
  
    it 'validates uniqueness of nickname' do
      expect(user.save).to be false
    end
  end
end

This looks already a bit nicer. However, it is not easy to read yet. You have to combine all lets and befores to know which steps are executed. What if you just merge all those parts?

1
2
3
4
5
6
7
8
9
10
11
12
RSpec.describe User do
  # ... lots of other specs
  
  describe 'validation' do
    it 'validates uniqueness of nickname' do
      _existing_user = MyTestFactory.build_and_persist_user(nickname: 'Johnny12')
      new_user = MyTestFactory.build_user(nickname: 'Johnny12')
      
      expect(new_user.save).to be false
    end
  end
end

As a bonus: thanks to RSpec’s predicate matchers you can make that expectation look a bit nicer:

1
2
new_user.save
expect(new_user).not_to be_persisted

This starts to look like our specifications! Finally, it would be nice to actually test if saving the user failed because of the uniqueness constraint:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
RSpec.describe User do
  # ... lots of other specs
  
  describe 'validation' do
    it 'validates uniqueness of nickname' do
      _existing_user = MyTestFactory.build_and_persist_user(nickname: 'Johnny12')
      new_user = MyTestFactory.build_user(nickname: 'Johnny12')
      new_user.save
      
      expect(new_user).not_to be_persisted
      expect(new_user.errors.full_messages).to include 'Nickname has already been taken' # <== added
    end
  end
end

However, my colleague legitimately made his point that this spec violates the best practise to only test one thing at the time. You’ll have to divide it up.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
RSpec.describe User do
  # ... lots of other specs
  
  describe 'validation' do
    context 'when using a nickname that has already been taken' do
      it 'does not persist the user' do
        _existing_user = MyTestFactory.build_and_persist_user(nickname: 'Johnny12')
        new_user = MyTestFactory.build_user(nickname: 'Johnny12')
        new_user.save
      
        expect(new_user).not_to be_persisted
      end
      
      it 'does not persist the user' do
        _existing_user = MyTestFactory.build_and_persist_user(nickname: 'Johnny12')
        new_user = MyTestFactory.build_user(nickname: 'Johnny12')
        new_user.save
      
        expect(new_user.errors.full_messages).to include 'Nickname has already been taken'
      end
    end
  end
end

As you might already have noticed, I’m not a fan of constructs like let/let!, before etc. I have learned to avoid them if possible, and only use them where the use case is valid. For example, a valid use case could be initializing a (stubbed) RabbitMQ connection right at the top of the spec and using that and only that connection throughout all examples. Jason Draper wrote two posts that explain the issues with using let:

At Kabisa we know a lot about writing good tests. Leave us a message if you would like to get in touch. Also, let us know when you have questions or feedback about this article.