I recently found a Reddit post which shared the code of a test and basically asked, “Am I doing this right?”
I’m going to dissect this test and make suggestions for improvement. Here’s the original code as shared in the post.
The original code
require 'test_helper'
class UserTest < ActiveSupport::TestCase
let(:user) { FactoryBot.build :user}
it "must be valid" do
value(user).must_be :valid?
end
it "can be saved" do
assert_difference "User.count", 1 do
user.save
end
assert user.persisted?
end
describe "won't be valid" do
it "with an duplicated email" do
user.save
user2 = FactoryBot.build :user
user2.wont_be :valid?
user2.errors.count.must_equal 1
user2.errors[:email].must_equal ["has already been taken"]
end
it "without an email" do
user.email = nil
value(user).wont_be :valid?
user.errors[:email].first.must_equal "can't be blank"
end
it "with an falsy email" do
user.email = "thisis@falsemail"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is invalid"
end
it "with an email without a mx-record" do
user.email = "hi@thisdomainwillneverexistnorhaveamxrecord.com"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is invalid"
end
it "with an email that is on our blacklist" do
user.email = "test@trashmail.com"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is a blacklisted email provider"
end
end
end
Initial overall thoughts
Overall I think the OP did a pretty good job. He or she got the spirit right. The changes I would make are fairly minor.
Before I get into the actual improvements I want to say that I personally am not familiar with the style of using matchers like `must_be` and I actually wasn’t able to turn up any discussion of `must_be` using a quick Google search. So as I add improvements to the test code, I’m also going to convert the syntax to the syntax I’m personally used to using.
Validity of fresh object
Let’s first examine the first test, the test for the validity of a `User` instance:
it "must be valid" do
value(user).must_be :valid?
end
It’s not a bad idea to assert that a model object must be valid when it’s in a fresh, untouched state. This test could be improved, however, by making it clearer that when we say “it must be valid”, we mean we’re talking about a freshly instantiated object.
Here’s how I might write this test instead:
context "fresh instance" do
it "is valid" do
expect(user).to be_valid
end
end
We can actually make the test even more concise by taking advantage of `subject`:
subject { FactoryBot.build(:user) }
context "fresh instance" do
it { should be_valid }
end
Persistence
Let’s look at the next test case, the test that asserts that a user can be persisted.
it "can be saved" do
assert_difference "User.count", 1 do
user.save
end
assert user.persisted?
end
I don’t see any value in this test. It’s true that an object needs to be able to be successfully saved, but we’ll be doing plenty of saving in the other tests anyway. Plus it’s typically pretty safe to take for granted that saving an ActiveRecord object is going to work. Rather than improving this test, I would just delete it.
Email validity
Lastly let’s take a look at the tests for email validity.
describe "won't be valid" do
it "with an duplicated email" do
user.save
user2 = FactoryBot.build :user
user2.wont_be :valid?
user2.errors.count.must_equal 1
user2.errors[:email].must_equal ["has already been taken"]
end
it "without an email" do
user.email = nil
value(user).wont_be :valid?
user.errors[:email].first.must_equal "can't be blank"
end
it "with an falsy email" do
user.email = "thisis@falsemail"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is invalid"
end
it "with an email without a mx-record" do
user.email = "hi@thisdomainwillneverexistnorhaveamxrecord.com"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is invalid"
end
it "with an email that is on our blacklist" do
user.email = "test@trashmail.com"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is a blacklisted email provider"
end
end
The overall idea is sound although the structure is actually backwards from how I would do it. Rather than saying “here are all the ways the object could be invalid”, I would list the various conditions and then, inside of each condition, assert the validity state. So here’s how I might structure these tests:
context "with an duplicated email" do
it "is not valid" do
user.save
user2 = FactoryBot.build :user
user2.wont_be :valid?
user2.errors.count.must_equal 1
user2.errors[:email].must_equal ["has already been taken"]
end
end
context "without an email" do
it "is not valid" do
user.email = nil
value(user).wont_be :valid?
user.errors[:email].first.must_equal "can't be blank"
end
end
context "with an falsy email" do
it "is not valid" do
user.email = "thisis@falsemail"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is invalid"
end
end
context "with an email without a mx-record" do
it "is not valid" do
user.email = "hi@thisdomainwillneverexistnorhaveamxrecord.com"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is invalid"
end
end
context "with an email that is on our blacklist" do
it "is not valid" do
user.email = "test@trashmail.com"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is a blacklisted email provider"
end
end
I’ll also try to improve each of these test cases individually.
Email uniqueness
This test can be written more concisely. Rather than this:
context "with an duplicated email" do
it "is not valid" do
user.save
user2 = FactoryBot.build :user
user2.wont_be :valid?
user2.errors.count.must_equal 1
user2.errors[:email].must_equal ["has already been taken"]
end
end
We can do this:
context "when email is not unique" do
let!(:other_user) { FactoryBot.create(:user, email: subject.email) }
it "is not valid" do
expect(subject.errors[:email]).to eq(["has already been taken"])
end
end
Email presence, email format validity, MX record validity, and blacklist check
These four test cases are all very similar so I’ll deal with them as a group. Here are the original version:
context "without an email" do
it "is not valid" do
user.email = nil
value(user).wont_be :valid?
user.errors[:email].first.must_equal "can't be blank"
end
end
context "with an falsy email" do
it "is not valid" do
user.email = "thisis@falsemail"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is invalid"
end
end
context "with an email without a mx-record" do
it "is not valid" do
user.email = "hi@thisdomainwillneverexistnorhaveamxrecord.com"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is invalid"
end
end
context "with an email that is on our blacklist" do
it "is not valid" do
user.email = "test@trashmail.com"
value(user).wont_be :valid?
user.errors[:email].first.must_equal "is a blacklisted email provider"
end
end
For the most part these tests are fine. There’s only one significant thing I would do differently. I find it redundant to both assert that there’s an error and assert that the object is invalid. If the object has an error I think it’s safe to assume that it’s also invalid. Here are my versions:
context "without an email" do
it "is not valid" do
subject.email = nil
subject.save
expect(subject.errors[:email]).to include("can't be blank")
end
end
context "with an invalid email" do
it "is not valid" do
subject.email = "thisis@falsemail"
subject.save
expect(subject.errors[:email]).to include("is invalid")
end
end
context "with an email without a mx-record" do
it "is not valid" do
subject.email = "hi@thisdomainwillneverexistnorhaveamxrecord.com"
subject.save
expect(subject.errors[:email]).to include("is invalid")
end
end
context "with an email that is on our blacklist" do
it "is not valid" do
subject.email = "test@trashmail.com"
subject.save
expect(subject.errors[:email]).to include("is a blacklisted email provider")
end
end
The entire improved test file
Lastly, here’s the full file after I’ve made my improvements:
require 'rails_helper'
RSpec.describe User, type: :model do
subject { FactoryBot.build(:user) }
context "fresh instance" do
it { should be_valid }
end
context "when email is not unique" do
let!(:other_user) { FactoryBot.create(:user, email: subject.email) }
it "is not valid" do
expect(subject.errors[:email]).to eq(["has already been taken"])
end
end
context "without an email" do
it "is not valid" do
subject.email = nil
subject.save
expect(subject.errors[:email]).to include("can't be blank")
end
end
context "with an invalid email" do
it "is not valid" do
subject.email = "thisis@falsemail"
subject.save
expect(subject.errors[:email]).to include("is invalid")
end
end
context "with an email without a mx-record" do
it "is not valid" do
subject.email = "hi@thisdomainwillneverexistnorhaveamxrecord.com"
subject.save
expect(subject.errors[:email]).to include("is invalid")
end
end
context "with an email that is on our blacklist" do
it "is not valid" do
subject.email = "test@trashmail.com"
subject.save
expect(subject.errors[:email]).to include("is a blacklisted email provider")
end
end
end
yeah, well done. it makes sense to just call save and not explicit call .valid?
I could see an argument for either. Personally I like `save`.
I prefer four-phase testing (eg: https://robots.thoughtbot.com/four-phase-test) with very big test files, having many `let` scopes gets confusing easily, you have to go up and down the file to fish for definitions and nested contexts.
With four-phase testing you have a flat structure of tests. There is some repetition, sure, but it’s worth for making the test easy to read in a linear fashion.