Four Key Reasons to Learn Markdown
Back-End Leveling UpWriting documentation is fun—really, really fun. I know some engineers may disagree with me, but as a technical writer, creating quality documentation that will...
Stylish Testing with an RSpec Style Guide
It’s common to have a style guide to help with code quality, but do you have a style guide for your test code? Test code quality is important because test suites get increasingly hard to maintain over time. Fragile tests discourage you from making app changes. Poorly performing tests don’t get run. And difficult-to-read tests slow you down as you’re maintaining your system.
Here are a handful of principles that tend to produce less fragile tests that are easier to understand. These are written with RSpec in mind, but most of the principles apply to any test framework.
- In unit tests, specify one behavior per test case.
- In end-to-end tests, specify many behaviors per test case.
- Initialize objects only for test cases that need them.
- Use single-level contexts.
- Keep related code close together.
- Set only significant fields on test data.
- Set all significant fields on test data.
- Stub in a setup block, then mock in the test case.
- But actually spy rather than mock.
- Use single-line syntax only when it reads like natural language.
- The
subjectshould be whatever object you’re making assertions against. - Don’t duplicate “act” step code.
In unit tests, specify one behavior per test case.
This might just be one assertion/expectation, or it might be a few that are very closely related–but no more than that. You should be able to summarize every assertion in the test case with a single simple natural-language phrase.
Bad:
describe "#push" do
it "pushes the object onto the stack" do
stack.push(object_to_push)
expect(stack.count).to eq(1)
expect(stack.empty).to be_falsy
expect(stack.peek).to eq(object_to_push)
end
end
Good:
describe "#push" do
before do
stack.push(object_to_push)
end
it "increments the count" do
expect(stack.count).to eq(1)
end
it "makes the stack no longer empty" do
expect(stack.empty).to be_falsy
end
it "makes that object the top of the stack" do
expect(stack.peek).to eq(object_to_push)
end
end
In end-to-end tests, specify many behaviors per test case.
There’s one situation where “one behavior per test case” causes a problem: tests that run a request through the whole application stack. For these tests, having only one behavior per test case may make your test suite slow as more tests are added over time. It’s best to do a “feature tour”: specify all the behaviors for a given request in one test case, so that the slow request is made only once.
Bad:
before(:each) do
post "/api/confirmation", {
confirmation: {
password: "Password123",
password_confirmation: "Password123",
confirmation_token: confirmation_token
}
}
end
it 'returns a created status' do
expect(response.status).to eq(201)
end
it 'returns the user email' do
expect(JSON.parse(response.body)["confirmation"]["email"]).to eq(user_login.email)
end
it 'sets the user to confirmed' do
expect(user_login.reload).to be_confirmed
end
Good:
it 'sets the user to confirmed and returns the email' do
post "/api/confirmation", {
confirmation: {
password: "Password123",
password_confirmation: "Password123",
confirmation_token: confirmation_token
}
}
expect(response.status).to eq(201)
expect(JSON.parse(response.body)["confirmation"]["email"]).to eq(user_login.email)
expect(user_login.reload).to be_confirmed
end
Initialize objects only for test cases that need them.
Not every test case needs every test object. In RSpec, it can be easy to get into the habit of writing all your let statements in the “bang” form, let!, which instantiates the object immediately. The problem is that as you add more test cases, this approach will end up instantiating objects that most of your test cases don’t need. Write let first, and change it to let! only if you have to do so to get a test to pass.
Bad:
let!(:user) { User.create! }
let!(:blog_post) { BlogPost.create! }
it "returns blog posts" do
get "/blog_posts", user.auth_header
expect(JSON.parse(response.body).count).to eq(1)
end
Good:
let(:user) { User.create! }
let!(:blog_post) { BlogPost.create! }
it "returns blog posts" do
get "/blog_posts", user.auth_header
expect(JSON.parse(response.body).count).to eq(1)
end
Note that blog_post still needed a bang to be eagerly-instantiated, because it’s never referenced directly. The test assumes the blog post will exist, but nothing makes RSpec lazily-instantiate it. In this case, eager instantiation is the right approach.
Don’t go too many levels deep with “contexts” (context blocks, subclasses or folders, depending on your test framework). More than one level of context can make it hard to find all the variable definitions and setup blocks that apply. Instead, as ExUnit explains, it’s best to have one level of context that names your entire testing setup. For test code, readability is generally more important than the DRY principle (“Don’t Repeat Yourself”).
Bad:
context "not logged in" do
# ...
end
context "logged in" do
context "as a non-admin user" do
context "unconfirmed" do
# ...
end
context "confirmed" do
context "active" do
# When will it end!?!?...
end
context "deactivated" do
# ...
end
end
end
context "as an admin user" do
# ...
end
end
Good:
context "not logged in" do
# ...
end
context "as an unconfirmed user" do
# ...
end
context "as an active user" do
# ...
end
context "as a deactivated user" do
# ...
end
context "as an admin user" do
# ...
end
Note that the contexts here aren’t as explicit: we don’t actually say that “an active user” is confirmed and non-admin. But since readers will likely assume a user is confirmed and non-admin by default, we don’t need to write that out explicitly.
Keep related code close together.
Related code can get separated in RSpec when you use let() statements and before blocks at too high a level, so that your test cases use objects defined in a completely different part of the file. Any minor performance gains are likely not worth the readability loss.
Bad:
let(:path) { "/api/posts" }
let(:params) {
{
title: title,
author: author,
body: body,
}
}
# many lines of test
context "valid post" do
let(:title) { "Title" }
let(:author) { "Test Person" }
let(:body) { "Body" }
it "creates a post" do
post path, params # what even ARE path and params???
# ...
end
end
Good:
# many lines of test
context "valid post" do
it "creates a post" do
post "/api/posts", {
title: "Title",
author: "Test Person",
body: "body",
} # ah, all here in one place
# ...
end
end
Set only significant fields on test data.
When setting up test data, it’s best to only show the values that are necessary to understand the test case, and to leave the rest to defaults. This makes the test quicker to read, as well as preventing you from having to make widespread changes when you add a new required field to a class. Here are a few approaches that can help:
For models, define default data using a factory. Factories can either be created by hand or by using a library like factory_girl.
Bad:
let(:titleless_post) {
Post.create!( title: nil,
body: "Blah",
author: "Sally Jones",
publish_status: Post::PUBLISHED,
publish_date: DateTime.now ) # which of these fields do I care about?
}
let(:bodyless_post) {
Post.create!( title: "Blah",
body: nil,
author: "Andre Smith",
publish_status: Post::PUBLISHED,
}
Good:
# factories/post.rb
factory :post do
title "Test Post"
body "This is a test post."
author "Test Person"
publish_status Post::PUBLISHED
publish_date DateTime.now
end
# spec
let(:titleless_post) { FactoryGirl.create(:post, title: nil) }
let(:bodyless_post) { FactoryGirl.create(:post, body: nil) }
For objects that aren’t models, define them with in the broader context with variables for fields that need to change, then define those variables in narrower contexts.
Bad:
define "#process_email_bounce_webhook" do
context "soft bounce" do
let(:webhook_data) {
{
bounce_type: "soft",
recipient: "test@example.com",
subject: "Test Message",
date_sent: DateTime.now,
}
}
# ...
end
context "hard bounce" do
let(:webhook_data) {
{
bounce_type: "hard",
recipient: "test@example.com",
subject: "Test Message",
date_sent: DateTime.now,
}
}
# ...
end
end
Good:
define "#process_email_bounce_webhook" do
let(:webhook_data) {
{
bounce_type: bounce_type,
recipient: "test@example.com",
subject: "Test Message",
date_sent: DateTime.now,
}
}
context "soft bounce" do
let(:bounce_type) { "soft" }
# ...
end
context "hard bounce" do
let(:bounce_type) { "hard" }
# ...
end
end
In RSpec, use shared contexts for multiple related objects of different types.
Bad:
describe "#show" do
context "as a staff member" do
let(:staff) { FactoryGirl.create(:staff) }
let(:user_login) { FactoryGirl.create(:user_login, role: staff) }
let(:access_token) { Doorkeeper::AccessToken.create!(resource_owner_id: user_login.id).token }
let(:auth_header) { { "Authorization" => "Bearer #{access_token}" } }
#...
end
end
describe "#create" do
context "as a staff member" do
let(:staff) { FactoryGirl.create(:staff) }
let(:user_login) { FactoryGirl.create(:user_login, role: staff) }
let(:access_token) { Doorkeeper::AccessToken.create!(resource_owner_id: user_login.id).token }
let(:auth_header) { { "Authorization" => "Bearer #{access_token}" } }
#...
end
end
Good:
# support/with_a_staff_member_access_token.rb
RSpec.shared_context "with a staff member access token" do
let(:staff) { FactoryGirl.create(:staff) }
let(:user_login) { FactoryGirl.create(:user_login, role: staff) }
let(:access_token) { Doorkeeper::AccessToken.create!(resource_owner_id: user_login.id).token }
let(:auth_header) { { "Authorization" => "Bearer #{access_token}" } }
end
# spec
describe "#show" do
context "as a staff member" do
include_context "with a staff member access token"
#...
end
end
describe "#create" do
context "as a staff member" do
include_context "with a staff member access token"
#...
end
end
Set all significant fields on test data.
This is the flip side of the previous principle. If a field is significant for a test case, don’t rely on a default value, even if it’s the value you need. Go ahead and set that value within your test case. Not doing so leads to test fragility: if the default value of that field changes, lots of tests could start failing. It also hinders readability: it’s not as clear to the reader what field values matter for the test.
Bad:
# factories/post.rb
factory :post do
published true
# ...
end
# spec
context "when the post is published" do
# are we *sure* this is published?
let(:published_post) { FactoryGirl.create(:post) }
# ...
end
Good:
# factories/post.rb
factory :post do
published true
# ...
end
# spec
context "when the post is published" do
let(:post) { FactoryGirl.create(:post, published: true) }
# ...
end
Stub in a setup block, then mock in the test case.
If you’re specifying only one behavior per test case, there will often be a lot of outgoing messages you don’t care about for that test case, but that still need to work. Stubs (set up with the allow() method in RSpec Mocks) take care of supplying that behavior, but all those stubs will clutter up your tests with a lot of duplication. Instead, put all your stub setup in a single before block. Then your test cases will only have the mocks (expect() in RSpec Mocks) related to what they’re specifying.
Not so good:
let(:model) { instance_double(Model) }
let(:mailer) { instance_double(Mailer) }
let(:job) { instance_double(NewModelJob) }
it "saves the model" do
expect(model).to receive(:save).with({validate: false}).and_return(true)
allow(mailer).to receive(:send)
allow(job).to receive(:enqueue)
command.execute
end
it "sends a notification email" do
allow(model).to receive(:save).and_return(true)
expect(mailer).to receive(:send)
allow(job).to receive(:enqueue)
command.execute
end
it "enqueues a job" do
allow(model).to receive(:save).and_return(true)
allow(mailer).to receive(:send)
expect(job).to receive(:enqueue).with(model)
command.execute
end
Better:
let(:model) { instance_double(Model) }
let(:mailer) { instance_double(Mailer) }
let(:job) { instance_double(NewModelJob) }
before {
allow(model).to receive(:save).and_return(true)
allow(mailer).to receive(:send)
allow(job).to receive(:enqueue)
}
it "saves the model" do
expect(model).to receive(:save).with({validate: false})
command.execute
end
it "sends a notification email" do
expect(mailer).to receive(:send)
command.execute
end
it "enqueues a job" do
expect(job).to receive(:enqueue).with(model)
command.execute
end
When your method calls are stubbed, you don’t need to set expectations before calling your production code; the stubs will allow them to pass. Then you’re free to use spying instead of mocking. Spying on methods tends to be more readable than mocking them, because this allows your code to follow the “arrange, act, assert” order.
Okay: (see above)
Even Better:
let(:model) { instance_double(Model) }
let(:mailer) { instance_double(Mailer) }
let(:job) { instance_double(NewModelJob) }
before {
allow(model).to receive(:save).and_return(true)
allow(mailer).to receive(:send)
allow(job).to receive(:enqueue)
command.execute
}
it "saves the model" do
expect(model).to have_received(:save).with({validate: false})
end
it "sends a notification email" do
expect(mailer).to have_received(:send)
end
it "enqueues a job" do
expect(job).to have_received(:enqueue).with(model)
end
Note that you don’t have to create your test doubles with RSpec’s spy or instance_spy methods in order to spy on method calls. The instance_double method verifies both that the method was stubbed and that it exists on the doubled object, so we recommend it.
Use single-line syntax only when it reads like natural language.
If RSpec’s single-line syntax (it { is_expected.to... }) makes it harder to understand what you’re specifying, just use the normal multiline syntax with an English description instead.
Bad:
subject(:product_ids) { JSON.parse(response.body)["product_ids"] }
it { is_expected.to include matching_category.products.map { |p| p["id"] }) }
# Read it out loud: “…matching category products map p p id”…?
Good:
subject(:product_ids) { JSON.parse(response.body)["product_ids"] }
it "includes all expected products" do
expect(product_ids).to include matching_category.products.map { |p| p["id"] }
end
Also good:
subject(:product_ids) { JSON.parse(response.body)["product_ids"] }
let(:matching_product_ids) { matching_category.products.map { |p| p["id"] } }
it { is_expected.to include matching_product_ids }
# “It is expected to include matching product IDs.”
The subject should be whatever object you’re making assertions against.
This isn’t always the class that the test relates to: sometimes it might be the result of a method call.
Bad:
describe DateFormatter do
subject(:formatter) { DateFormatter.new(format_string) }
describe "#format" do
let(:formatted_date) { formatter.format(date_to_format) }
# tests that all refer to formatted_date, not to the "subject", formatter
end
end
Good:
describe DateFormatter do
let(:formatter) { DateFormatter.new(format_string) }
describe "#format" do
subject(:formatted_date) { formatter.format(date_to_format) }
# tests that all refer to the subject, formatted_date
end
end
Don’t duplicate “act” step code.
There are a few different ways to avoid having to write out your “act” step multiple times.
When you will always be checking the return value of your “act” step, make it your subject and use one-liner syntax. This shortens your test cases considerably.
Bad:
it "includes a matching product" do
expect(catalog.product_ids).to include matching_product.id
end
it "does not include a non-matching product" do
expect(catalog.product_ids).not_to include non_matching_product.id
end
it "does not include a deactivated product" do
expect(catalog.product_ids).not_to include deactivated_product.id
end
it "does not include a product from another catalog" do
expect(catalog.product_ids).not_to include other_catalog_product.id
end
Good:
subject(:product_ids) { catalog.product_ids }
it { is_expected.to include matching_product.id }
it { is_expected.not_to include non_matching_product.id }
it { is_expected.not_to include deactivated_product.id }
it { is_expected.not_to include other_catalog_product.id }
When you need to run the “act” step before each test case, put it in a before block. That way, all your test cases might consist entirely of expect statements.
Bad:
subject(:stack) { Stack.new }
it "puts the object on top of the stack" do
stack.push(object_to_push)
expect(stack.peek).to eq(object_to_push)
end
it "results in the count being 1" do
stack.push(object_to_push)
expect(stack.count).to eq(1)
end
Good:
subject(:stack) { Stack.new }
before do
stack.push(object_to_push)
end
it "puts the object on top of the stack" do
expect(stack.peek).to eq(object_to_push)
end
it "results in the count being 1" do
expect(stack.count).to eq(1)
end
When your “act” step needs to run in the middle of your test cases, name it as a “bang variable”. Instead of duplicating the “act” step in each test case, you can define it in a subject or let statement, and put a bang at the end of the variable name to indicate that it’s performing functionality.
Bad:
context "when empty" do
subject(:stack) { Stack.new([]) }
it "raises an error" do
expect{ stack.pop }.to raise_error("nothing to pop")
end
end
context "when one element" do
subject(:stack) { Stack.new(["hello"]) }
it "returns the element" do
expect(stack.pop).to eq("hello")
end
end
Good:
subject(:pop!) { stack.pop }
context "when empty" do
let(:stack) { Stack.new([]) }
it "raises an error" do
expect{ pop! }.to raise_error("nothing to pop")
end
end
context "when one element" do
let(:stack) { Stack.new(["hello"]) }
it "returns the element" do
expect(pop!).to eq("hello")
end
end
Give these principles a try and see if they help your tests’ readability and performance. Putting in the effort now will set you up for a much less frustrating test suite in the future.
Do you have other guidelines for writing tests that are readable and maintainable? Share them with us in the comments!
