In Which I Dissect and Improve a Rails Test File I Found on Reddit

by Jason Swett,

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

3 thoughts on “In Which I Dissect and Improve a Rails Test File I Found on Reddit

  1. Federico

    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.

    Reply

Leave a Reply

Your email address will not be published. Required fields are marked *