Testing Styleguide
In this section we're discussing on what is considered a good practice and what should be absolutely avoided when writing specs for our codebase.
Always write a meaningful description in examples, do not do it do
You should write a clear english sentence in third person to improve readability of spec output. It will also improve rspec output in case of failures and makes clear what actually failed
RSpec.describe Array do
describe '#sum' do
context 'GOOD EXAMPLES' do
it 'adds array content' do
expect([1, 2].sum).to eq(3)
end
it 'fails with wrong numbers' do
expect([1, 2].sum).to eq(4)
end
end
context 'BAD EXAMPLES' do
it do
expect([1, 2].sum).to eq(3)
end
it do
expect([1, 2].sum).to eq(4)
end
end
end
end
When in doubt run your specs with doc format to see how the final sentence will look like. In our repo you have to
disable fuubar formatter that's on by default.
rspec -fd path/to/some/spec
Please notice the difference in RSpec output, when using bad examples output is not descriptive at all and it uses last matcher as description example.
Exception for this rule are for oneliners such as it { should do_something } when they uses a meaningful
matcher that will produce a good message in its assertion. Again run with -fd to test it and if it doesn't
produce a good output you can always use one of these forms:
# GOOD
it { should validate_presence_of(:some_field) } # Good description by matcher
it('produces 123') { should eq(123) } # Good description from example desc
# BAD
it { should eq(123) } # Will produce it is expected to eq 123
# Will have a good desc but is a bad form to have it do with onliners
it do
is_espected.to validate_presence_of(:some_field)
end
It's pretty hard to understand what's failed while looking at RSpec summary
Real test case from our codebase, it changes this output
into
Write short test cases
A unit test should be focused only on a single thing it should not test lot of different stuff. As a rule of thumb an example should have this structure
- setup (
letorbefore) - invocation of code under test
- expectations
Some guidelines (numbers are mine, YMMV) on these parts of a test
itbody should be under 10 lines, the lesser is betteritbody should have less than 3 expectationsleta test case should not have more than 3/4 letbeforeshould be under 3 lines
Actual rules for new code will be enforced by RSpec rubocop rules.
Write consistent factories and test cases
When creating factories for new models pay attention to performances and consistency. It's easy to create (or generate) a factory with default code
FactoryBot.define :some_model do
association :venue
association :user
association :customer
end
But this will have very bad performances and also may produce test cases with not realistic behavior if used as is. The
following piece of code will create (at least) 6 different venues (the 3 from association and 3 from customer factory)
a dozen of Areas and so on.
that's likely not the intended result.
let(:models) { create_list(:some_model, 3) }
You should always think to involved associations and how they can be made coherent. Something like this:
FactoryBot.define :some_model do
association :venue
association :user
customer { |m| association(:customer, venue: m.venue, user: m.user) }
# Alternative pattern that can be used in some cases to avoid venue proliferation
venue { Venue.first || association(:venue) }
end
It could be a good practice to write some specs for complex factories you're writing, take a look at this example for how to do that
A consequence of this is that a hypotetical test case that's written with something like this it's likely unoptimized and possibly wrong
RSpec.describe 'an hypotetical scope' do
# You're likely creating a lot of dependent objects (SM, venues, customers, etc)
# and most importantly these appointments are not in the same venue
let(:venue) { monday_appointment.venue }
let!(:monday_appointment) { create(:appointment, date: monday) }
let!(:tuesday_appointment) { create(:appointment, date: tuesday) }
it 'returns monday\'s appointment' do
# If venue or customer is not consistent you're testing something wrong because customer is different,
# venue is different and so on, for this reason you're not testing that tuesday_appointment is excluded
# by date
expect(venue.appointments_for(monday, monday_appointment.customer)).to contain_exactly(monday_appointment)
end
end
Write factories or traits when you're building a let longer than 5 rows (especially if reused)
It's pretty common in our codebase to have a very long list of let, especially in V1 controller tests. This will make
tests ugly and difficult to follow. An extreme example of this comes from Api::V1::MarketingPromotionsController
where in order to build a single marketing promotion with wanted traits you have to create lot of different nested objects.
It's counterpart is the bundle_discount factory,
that creates the same type of object with coerents resources with a
single line of code
If you're creating more than 3 objects to build a composite one, you're doing wrong and you will pay in terms of lines of code and headcaches what you're doing.
Try to abstract it into a dedicated factory or trait.
Factories are code as well and you should invest your time in creating reusable factories.
Always use shoulda matchers to test validation constraints
They produces meaningful example description, are pretty concise and reusable. They also support additional parameters
such as with_message (docs)
and they takes care of assigning values for you.
Practical example
RSpec.describe StaffMember do
context 'using shoulda matchers' do
it { should validate_presence_of(:first_name) }
end
context 'with be_valid' do
it do
sm = StaffMember.new
sm.first_name = nil
expect(sm).to_not be_valid
sm.first_name = ''
expect(sm).to_not be_valid
end
end
end
Output:
Corollary: Never use expect(something).to_not be_valid
Whenever possible shoulda matchers to test controller filters
We have lot of controller test cases that test for authentication and/or authorization, or that tests for 404 and so on.
When the code that drives the 401/403, or generally speaking when we're testing a before_action simply remove all those
specs because:
- They are not testing that controller, but only the fact that it will use
- They are essentially cut and paste code scattered across all our codebase
- All that code can be simply replaced with a
it { should use_before_action :xxx}
A partial exception to this rule is when you're using before_action :something, only: :xxx because shoulda
matcher doesn't accept any argument to test for that. So in that case you may want to test specific exceptions
(maybe with a custom matcher?)
Sample commit and its results
Before:
After:
Always use have_enqueued_job instead of expect(SomeJob).to receive(:perform_later)
This one is easy, always use have_enqueued_job matcher,
in short it provides a nicer syntax, a nicer assertion description and it can be used in block form (e.g. expect { ... }.to have_enqueued_job ).
Most important thing it also checks that job arguments are properly serializable.
Example file to show the above points
RSpec.describe 'Something' do
class SomeJob < ApplicationJob
end
SomeObject = Struct.new(:foo) do
def self.enqueue
SomeJob.perform_later(new(123))
end
end
describe 'Job Enqueuing' do
let(:argument) { SomeObject.new(123) }
context 'GOOD EXAMPLES' do
it 'is expected to fail with actual reason' do
pending('It will fail with serialization error')
expect { SomeObject.enqueue }.to have_enqueued_job(SomeJob).with(argument)
end
# No desc to show -fd output
it do
expect { SomeJob.perform_later(1) }.to have_enqueued_job(SomeJob).with(1)
end
end
context 'BAD EXAMPLES' do
it 'passes while it should have been failed' do
expect(SomeJob).to receive(:perform_later).with(argument)
SomeObject.enqueue
end
# No desc to show -fd output
it do
expect(SomeJob).to receive(:perform_later).with(1)
SomeJob.perform_later(1)
end
end
end
end
Output
Prefer expect { ... }.to change(...)
Use whenever possible expect { some piece of code }.to change(something).to(other), instead
of some piece of code; expect(something).to eq(other)
This one is stylistic but it could also prevent false positives in certain cases. When you're testing a piece of code that is supposed to set a value, or to schedule o job the suggested syntax will actually ensure that the code inside the block is triggering what is the expected behavior and not preexistent condition. Practical example:
RSpec.describe 'Something' do
Foo = Struct.new(:a) do
def a_buggy_method(new_value_for_a)
# self.a = new_value_for_a
end
end
describe 'Change VS simple assertion' do
let(:foo) { Foo.new(1) }
describe '#a_buggy_method' do
context 'with change' do
it 'detects the error' do
pending('Notice that a is already 1')
expect { foo.a_buggy_method(1) }.to change(foo, :a).to(1)
end
end
context 'with later assertion' do
it 'does not detect the error' do
foo.a_buggy_method(1)
expect(foo.a).to eq(1) # it was 1 also before the assertion
end
end
end
end
end
Output:
Document your specs
TLDR specs are code, please document and comment them (where appropriate).
Rationale about this point should need no explaination, but what you should comment? Generally speaking you should document:
- with example descriptions and
context(ordescribe) as we already discussed - with pure comments into the setup of your test if test case is complex
- line by line when needed
- why you are creating an object
- why do you need a certain state before the test case
- why an assertion is expected to pass (or to fail)
- if a spec is a reproduction of a specific bug, or somehow related to a specific task add a reference to it.
For this one I will share a couple of links to one of the most complex part of our codebase I worked with (excluding Availability services 😉)
Legacy code with no docs
Refactored one commented spec
It's a good exercise to write a custom matcher for specific use cases
It's a feature of rspec that will improve readability of code, enforce DRY and produce beautiful assertion messages. It also allow you to master rspec builtin assertion, try to write a custom matcher and find a relevant use case in our test suite.
Custom matchers can be global and availabley in all specs, or they can be local and available only in the context they have been defined.
Don't use to_not be_valid (never)
It will essentially lead to false positives because specs written with to_not be_valid won't check why a model is
actually invalid. Proof of concept
class User < ApplicationRecord
validates_presence_of :name
validates_presence_of :email
end
RSpec.describe User do
it 'validates presence of name' do
subject.name = ''
# subject.email is nil as well
expect(subject).to_not be_valid # This will be green
# In order to fix this you must assert that user has error on intented field
# expect(subject.errors[:name]).to eq('some error message')
end
end
Rule of thumb:
- Always use Shoulda Matchers
- Test custom validations with
should_not allow_value(xxx).for(:field)(use.with_messageif relevant)
Applied to our code base (it also fixes other issues)
Before:
After:
Notice runtime improvement, about 40% faster
Don't use instance variables in specs @some-var
They are ugly, not used in spec files and they can change. Prefer let(:some_var) that is memoized and reads better.
Also with instance variables specs are harder to refactor (true also for production code).
Don't compare objects by id whenever possible
Don't write assetions like expect(some_scope_results.map(&:id)).to eq(some_expected_results.map(&:id)), they are ugly
to read and when they fails they produce useless error messages.
Remember that every ActiveRecord object implements equality methods based on object id, so the following examples
are equivalent but in the first case the failure doesn't tell us anything.
RSpec.describe 'Record comparison' do
let(:bundle) { create :bundle_discount }
let(:discount) { create :marketing_promotion, :automatic_discount }
describe 'Compare by id' do
it do
pending('expected failure')
expect([bundle].map(&:id)).to eq([discount].map(&:id))
end
end
describe 'Compare by object identity' do
it do
pending('expected failure')
expect([bundle]).to eq([discount])
end
end
end
Don't share too much context across specs
For instance to test a scope don't create 5 venues with let! they will hurt performances a lot and make tests more
complex. Also if you need to change an attribute or some value in one of your objects you will likely break other
specs that use the same context. Also they make tests harder to read.
Also you will pay that time even when you're dealing with simple specs that won't need those data.
As rule of thumb don't ever write a let! in main RSpec.describe block because there's no undo.
If you really need to do that use a shared_context and include it in your example or introduce a new scope with
an artificial describe/context
RSpec.describe 'Bad Let Sharing' do
let!(:shared_by_all) { sleep(2); rand(100) }
let!(:another_shared) { sleep(2); rand(100) }
it 'pays 4 seconds (BAD)' do
# This will pay 4 seconds even if don't use above variables
end
end
RSpec.describe 'Workaround' do
context 'with shared setup' do
let!(:shared_by_all) { sleep(2); rand(100) }
let!(:another_shared) { sleep(2); rand(100) }
it 'pays 4 seconds only this' do
# This will pay 4 seconds even if don't use above variables
end
end
it 'will not pay 4 seconds' do
# Whatever
end
end
Don't abuse record creation
TLDR start with subject, then build_stubbed, then build and finally create.
In lot of specs there's no need to have a fully saved record to test what we wrote (e.g. validations, testing instance
methods, etc). Remember that every create! will run full object lifecycle: instantiation, validation, association creation and
all callbacks. Have all this code run only to test an instance method defined on an active record object is a waste of
CPU and database time that will be paid in every CI build.
Don't test full controller (or service) responses output, any field addition will invalidate all your spec.
TLDR don't use be_json_eql (unless you're testing a specific path) and don't use expect(parsed_response).to eq(some_json)
When you're testing controllers (or services) do not write assertions against whole response. That will make your spec
fragile and it will break on every field added to a presenter. If you do that essentially you're testing a Presenter
inside your controller. We should move this kind of specs in spec/presenters and just test for identity in controller specs.
E.g. if you're testing a filter for index you don't need to test full response to check that you're filter is working.
Good: Presenter Spec and controller spec
Bad: All existing specs must be changed
Don't name variables (especially let ones) venue_1, treatment_1, staff_member_1
Again, spec files are code and it must be written with the same code patterns you're using for production code. In our specs (especially old ones) we have lot of specs written in this way:
RSpec.describe do
let!(:manufacturer1) { create(:manufacturer) }
let!(:manufacturer2) { create(:manufacturer) }
let!(:manufacturer3) { create(:manufacturer) }
let!(:manufacturer4) { create(:manufacturer) }
let!(:product_category1) { create(:product_category) }
let!(:product_category2) { create(:product_category) }
let!(:product_category3) { create(:product_category) }
let!(:product_category4) { create(:product_category) }
let!(:product_line1) { create(:product_line) }
let!(:product_line2) { create(:product_line) }
let!(:product_line3) { create(:product_line) }
let!(:product_line4) { create(:product_line) }
# 500 lines below
expect(product_category3.some_properties).to eq(product_category2.whatever)
end
Avoid these patterns, use your fantasy and name things with meaningful names. Instead of naming stuff with generic_name_n
give them names like
- Venue Treatments:
color,haircut,blow_dry - Staff members:
john,george, whatever - Venues:
italian_venue,cash_enabled_venue, and so on - Products:
shampoo,color - etc,
With this convention it's clear what you're doing with those variables and you can mentally associate
some properties to them, e.g. color costs 100, haircut is performed by john instead of having
in specs venue_treatment1 is performed by staff_member_3.
Don't create order dependent tests
TLDR use contain_exaclty, match_array unless you're testing an ordered scope.
Prefer contain_exaclty when you're testing against records and use match_array against collections.
Every time you're asserting against a collection that's not explicitely ordered you're possibly introducing a
flaky test because database can possibly return records in a different order. The order can depend on postgres
version, on specific decision taken by query optimizer, in general every query with no ORDER BY clause
can return records in any order.
let(:some_venues) { create_list :venue, 2 }
# Good
expect(Venue.some_unordered_scope).to contain_exactly(first_venue, second_venue)
expect(Venue.some_unordered_scope).to match_array(some_venues)
# Bad
expect(Venue.some_unordered_scope).to match_array([first_venue, second_venue])
expect(Venue.some_unordered_scope).to eq([first_venue, second_venue])
expect(Venue.some_unordered_scope).to eq(some_venues)