Posted by Martijn Versluis on 27-9-2018
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:
1
2
3
4
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 'includes a descriptive error message' 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.