Rails Testing Antipatterns: Models

This is the second post in the series about antipatterns in testing Rails applications. See part 1 for thoughts related to fixtures and factories.

Creating records when building would also work

It is common sense to say that with plain ActiveRecord or a factory library you can both create and only instantiate new records. However, probably because testing models so often needs records to be present in the database, I have seen myself and others sometimes forget the build-only option. It is, however, one of the key ways to making your test suite faster.

Consider this spec for full_name:

RSpec.describe User do
  describe "#full_name" do

    before do
      @user = User.create!(first_name: "Johnny", last_name: "Bravo")
    end

    it "is composed of first and last name" do
      expect(@user.full_name).to eql("Johnny Bravo")
    end
  end
end

It triggers an interaction with database and disk drive to test logic which does not require it at all. Using User.new would suffice. Multiply that by the number of examples reusing that before block and all similar occurrences and you get a lot of overhead.

All model, no unit tests

MVC plus services is good enough for typical Rails apps when they’re starting out. Over time, however, the application can benefit from some domain specific classes and modules. Having all logic in models ties it to the database, eventually leading to tests so slow that you avoid running them.

If your experience with testing started with Rails, you may be associating unit tests with Rails models. However, in “classical” testing terminology, unit tests are considered to be mostly tests of simple, “plain-old” objects. As described by the test pyramid metaphor, unit tests should be by far the most frequent and the fastest to run.

Not having any unit tests in a large application is not technically an antipattern in testing, but an indicator of a possible architectural problem.

Take for example a simple shopping cart model:

class ShoppingCart < ApplicationRecord
  has_many :cart_items

  def total_discount
    cart_items.collect(&:discount).sum
  end
end

The test would go something like this:

require "rails_helper"

RSpec.describe ShoppingCart do

  describe "total_discount" do

    before do
      @shopping_cart = ShoppingCart.create!

      @shopping_cart.cart_items.create!(discount: 10)
      @shopping_cart.cart_items.create!(discount: 20)
    end

    it "returns the sum of all items' discounts" do
      expect(@shopping_cart.total_discount).to eql(30)
    end
  end
end

Observe how the logic of ShoppingCart#total_discount actually doesn’t care where cart_items came from. They just need to be present and have a discount attribute.

We can refactor it to a module and put it in lib/. That would be a good first pass — merely moving a piece of code to a new file has benefits. I personally prefer to use modules only when I see behavior which will be shared by more than one class. So let’s try composition. Note that in real life you usually have more methods that can be grouped together and placed in a more sophisticated directory structure.

# lib/shopping_cart_calculator.rb
class ShoppingCartCalculator

  def initialize(cart)
    @cart = cart
  end

  def total_discount
    @cart.cart_items.collect(&:discount).inject(:+)
  end
end

To test the logic defined in ShoppingCartCalculator, we now no longer need to work with the database, or even Rails at all:

require 'shopping_cart_calculator'

RSpec.describe ShoppingCartCalculator do

  describe "#total_discount" do

    before do
      cart = double
      cart_items = [double(discount: 10),
                    double(discount: 20)]
      allow(cart).to receive(:cart_items).and_return(cart_items)

      @calculator = ShoppingCartCalculator.new(cart)
    end

    it "returns the sum of all cart items' discounts" do
      @calculator.total_discount.should eql(30)
    end
  end
end

Notice how the spec for the calculator is requiring spec_helper instead of rails_helper. Requiring rails_helper in a spec file generally means that your whole Rails application needs to load before the first example runs. Depending on your machine and application size, running a single spec may then take from a few to 30+ seconds. This is avoided with use of application preloading tools such as Spring. It is nice, however, if you can achieve indepedence of them.

To get more in-depth with the idea I recommend watching the Fast Rails Tests talk by Corey Haines. Code Climate has a good post on practical ways to decompose fat ActiveRecord models.

Note that new Rails 5 apps are encouraged to use concerns, which are a handy way of extracting model code that DHH recommended a while ago.

The elegance of the entire suite being extremely fast, not requiring rails_helper, etc, is secondary in my opinion. There are many projects that can benefit from this technique even partially applied, because they have all logic in models, as in thousands of lines of code that can be extracted into separate modules or classes. Also, do not interpret the isolated example above as a call to remove all methods from models in projects of all size. Develop and use your own sense when it is a good moment to perform such architecture changes.

Using let to set context

Using let(:model) (from RSpec) in model tests may lead to unexpected results. let is lazy, so it doesn’t execute the provided block until you use the referenced variable further in your test. This can lead to errors which can make you think that there is something wrong with the database, but of course it is not; the data is not there because the let block has not been executed yet. Non-lazy let! is an alternative, but it’s not worth the trouble. Simply use before blocks to initialize data.

RSpec.describe ShoppingCart do

  describe ".empty_carts" do
    let(:shopping_cart) { ShoppingCart.create }

    it "returns all empty carts" do
      # fails because let is never executed
      expect(ShoppingCart.empty_carts).to have(1).item
    end
  end
end

On a sidenote, some people prefer not to use let at all, anywhere. The idea is that let is often used as a way to “DRY up” specs, but what is actually going on is that we’re trying to hide the complexity of the test setup, which is a code smell.

Incidental database state

Magic numbers and incidental state can sneak in specs that deal with database records.

RSpec.describe Task do
  describe "#complete" do

   before do
     @task = create(:task)
   end

    it "completes the task" do
      @task.complete

      # Creating another completed task above while extending the test suite
      # would make this test fail.
      expect(Task.completed.count).to eq(1)
    end

    # Take 2: aim to measure in relative terms:
    it "completes the task" do
      expect { @task.complete }.to change(Task.completed, :count).by(1)
    end
  end
end

In this article, we’ve covered most common antipatterns in writing tests for Rails applications when testing models. Have some questions or examples of your own? Share them in the comments below.

Want to focus on writing code and not worry about how your tests run?
Try Semaphore, the simplest and fastest CI/CD for free. Among other helpful features, it has Test Reports that allow you to get an overview of how your test suite is doing. See skipped and failed tests and find the slowest tests – all from the Tests Dashboard.

Other posts you might also like:

• ls Testing Antipatterns: Fixtures and Factories
• Getting a 10-minute CI build for 100k of Rails code
• Rails Testing Handbook: A Free Ebook to Help You Build Better Apps
• Tips on treating flakiness in your Rails test suite
• Lies, Damn Lies, and the State of Testing in Rails
• Revving up Continuous Integration with Parallel Testing

Post updated on 9 May 2018 by Thiago Araújo Silva.