Using shoulda to refactor rspec tests on Rails models

Question

After learning about shoulda-matchers by answering another StackOverflow question on attribute accessibility tests (and thinking they were pretty awesome), I decided to try refactoring the model tests I did in The Rails Tutorial in an attempt to make them even more concise and thorough. I did this thanks to some inspiration from the documentation for modules Shoulda::Matchers::ActiveRecord and Shoulda::Matchers::ActiveModel, as well as this StackOverflow answer on structuring shoulda tests in models. However, there's still a few things I am not sure about, and I am wondering how these tests could be made better.

I will use the User spec in the Rails Tutorial as my example as it is the most detailed, and covers lots of areas that could be improved. The following code example has been changed from the original user_spec.rb, and replaces the code down until the describe "micropost associations" line. The spec tests against the user.rb model, and its factory is defined in factories.rb.

spec/models/user_spec.rb

# == Schema Information
#
# Table name: users
#
#  id              :integer          not null, primary key
#  name            :string(255)
#  email           :string(255)
#  created_at      :datetime         not null
#  updated_at      :datetime         not null
#  password_digest :string(255)
#  remember_token  :string(255)
#  admin           :boolean          default(FALSE)
#
# Indexes
#
#  index_users_on_email           (email) UNIQUE
#  index_users_on_remember_token  (remember_token)
#

require 'spec_helper'

describe User do

  let(:user) { FactoryGirl.create(:user) }

  subject { user }

  describe "database schema" do
    it { should have_db_column(:id).of_type(:integer)
                              .with_options(null: false) }
    it { should have_db_column(:name).of_type(:string) }
    it { should have_db_column(:email).of_type(:string) }
    it { should have_db_column(:created_at).of_type(:datetime)
                              .with_options(null: false) }
    it { should have_db_column(:updated_at).of_type(:datetime)
                              .with_options(null: false) }
    it { should have_db_column(:password_digest).of_type(:string) }
    it { should have_db_column(:remember_token).of_type(:string) }
    it { should have_db_column(:admin).of_type(:boolean)
                              .with_options(default: false) }
    it { should have_db_index(:email).unique(true) }
    it { should have_db_index(:remember_token) }
  end

  describe "associations" do
    it { should have_many(:microposts).dependent(:destroy) }
    it { should have_many(:relationships).dependent(:destroy) }
    it { should have_many(:followed_users).through(:relationships) }
    it { should have_many(:reverse_relationships).class_name("Relationship")
                         .dependent(:destroy) }
    it { should have_many(:followers).through(:reverse_relationships) }
  end

  describe "model attributes" do
    it { should respond_to(:name) }
    it { should respond_to(:email) }
    it { should respond_to(:password_digest) }
    it { should respond_to(:remember_token) }
    it { should respond_to(:admin) }
    it { should respond_to(:microposts) }
    it { should respond_to(:relationships) }
    it { should respond_to(:followed_users) }
    it { should respond_to(:reverse_relationships) }
    it { should respond_to(:followers) }
  end

  describe "virtual attributes and methods from has_secure_password" do
    it { should respond_to(:password) }
    it { should respond_to(:password_confirmation) }
    it { should respond_to(:authenticate) }
  end

  describe "accessible attributes" do
    it { should_not allow_mass_assignment_of(:password_digest) }
    it { should_not allow_mass_assignment_of(:remember_token) }
    it { should_not allow_mass_assignment_of(:admin) }
  end

  describe "instance methods" do
    it { should respond_to(:feed) }
    it { should respond_to(:following?) }
    it { should respond_to(:follow!) }
    it { should respond_to(:unfollow!) }
  end

  describe "initial state" do
    it { should be_valid }
    it { should_not be_admin }
    its(:remember_token) { should_not be_blank }
    its(:email) { should_not =~ /\p{Upper}/ }
  end

  describe "validations" do
    context "for name" do
      it { should validate_presence_of(:name) }
      it { should_not allow_value(" ").for(:name) }
      it { should ensure_length_of(:name).is_at_most(50) }
    end

    context "for email" do
      it { should validate_presence_of(:email) }
      it { should_not allow_value(" ").for(:email) }
      it { should validate_uniqueness_of(:email).case_insensitive }

      context "when email format is invalid" do
        addresses = %w[user@foo,com user_at_foo.org example.user@foo.]
        addresses.each do |invalid_address|
          it { should_not allow_value(invalid_address).for(:email) }
        end
      end

      context "when email format is valid" do
        addresses = %w[[email protected] [email protected] [email protected] [email protected]]
        addresses.each do |valid_address|
          it { should allow_value(valid_address).for(:email) }
        end
      end
    end

    context "for password" do
      it { should ensure_length_of(:password).is_at_least(6) }
      it { should_not allow_value(" ").for(:password) }

      context "when password doesn't match confirmation" do
        it { should_not allow_value("mismatch").for(:password) }
      end
    end

    context "for password_confirmation" do
      it { should validate_presence_of(:password_confirmation) }
    end
  end

  # ...
end

Some specific questions about these tests:

1. Is it worth testing the database schema at all? A comment in the StackOverflow answer mentioned above says "I only test things that are related to behavior and I don't consider the presence of a column or an index behavior. Database columns don't just disappear unless someone intentionally removes them, but you can protect against that with code reviews and trust", which I agree with, but is there any valid reason why the structure of the database schema would be tested for, and thus justifying the existence of the Shoulda::Matchers::ActiveRecord module? Perhaps just the important indexes are worth testing...?
2. Do the should have_many tests under "associations" replace their corresponding should respond_to tests under "model attributes"? I can't tell whether the should have_many test just looks for the relevant has_many declaration in a model file or actually performs the same function as should respond_to.
3. Do you have any other comments/suggestions to make these tests more concise/readable/thorough, both in content and structure?

Lockhart

Answer 1

1) The Shoulda::Matchers::ActiveRecord module has a lot more in it than just column and index matchers. I would dig around in the included classes a little and see what you can find. This is where the have_many, belong_to etc come from. For the record though, I see little value in most of what is in there.

2) Yes, macros such as have_many test a lot more than whether or not a model responds to a method. From the source code, you can see exactly what it is testing:

def matches?(subject)
  @subject = subject
  association_exists? &&
    macro_correct? &&
    foreign_key_exists? &&
    through_association_valid? &&
    dependent_correct? &&
    class_name_correct? &&
    order_correct? &&
    conditions_correct? &&
    join_table_exists? &&
    validate_correct?
end

3) Making the tests more readable and/or concise is definitely a subjective question to answer. Everyone will give you a different answer to this depending on their background and experience. I would personally get rid of all of the respond_to tests and replace them with tests that have value. When someone looks at your tests, they should be able to understand the public API for that class. When I see that your objects respond_to something like "following?", I can make assumptions, but don't really know what it means. Does it take an argument? Does it return a boolean value? Is the object following something or is something following the object?

Homolographic

This is an excellent answer and exactly what I wanted. Thank you so much! I think "when someone looks at your tests, they should be able to understand the public API for that class" is a very good rule to go by; time for some refactoring, I think. – Lockhart

Answer 2

Your question touched on a few points, I would like to address two of them:

The answer is subjective, so I will give you my personal take.

1) Test ActiveRecord that way?
My answer is yes. You could write complex tests with real data but if you basically trust ActiveRecord you can do it this way and if you get to doing tdd, with these tests first they can help in that process.

2) Write the model tests at all?
My answer is yes. What I do is focus controller and request specs on the happy path and then for cases where validations and the like are needed I write unit model tests for them. This has turned out to be a good division of responsibility to me.

Abet

Thanks very much for your response. +1 for the advice in number 2); much appreciated. – Lockhart

Answer 3

I think this whole thing should be seen from specification point of view.

If you have a component-testing-level specification which covers the necessary database columns for the given model, you should, otherwise not.

If not covered, but as a responsible developer you feel it important to have (your sw and its quality characteristics are better that way), you have to arrange to include this info in the spec, then you can put these tests in the test suite.

Gurolinick

Thanks for the answer. If someone, like a client, hands me a specification that absolutely must include low-level model testing of this kind for whatever reason, then of course I'd do it. But I guess I'd question your second point about being a "responsible developer": is it "responsible" or worth it to do these kinds of low-level tests, or is it just an excuse to write more unnecessary test specs that don't provide much extra value over behaviour-related tests? Also, do you have any comment about the second question above regarding the difference between have_many and respond_to? – Lockhart

Answer 4

Lower testing levels' requirements mostly come from within your organization (internal docs), customer mostly provides only the customer requirement spec (let's say this is the highest level in testing V-model). As your organization starts design the sw creates the lower test levels specs step by step.

For the "do we really need this" question: it depends on many things: the app complexity, safety critical or not, standards to follow, contractual/legal/industrial regulations, etc.

In generally I would say, for a correct ideal application reqirement responsible for unit testing should write the unit level spec and tester should implement test based on this spec.

For "have_many and respond_to" I am afraid I have no background info how they are implemented, so can not answer.

Gurolinick

Answer 5

I've found some value in writing tests for the presence of database columns. Here's why:

1) Writing them keeps me in the rhythm of TDD.
2) Migrations are usually pretty awesome, until they aren't. I know you're not supposed to edit an existing migration, but when I'm just working on something myself I sometimes do it anyway. And if someone else is working on the same application and changes an existing migration instead of writing a new one, these tests have isolated the problem pretty quickly for me.

If you get bogged down with too many column names & types you can do something like this to save yourself typing:

describe User do

  describe 'database' do 

    describe 'columns' do 

      %w[reset_password_sent_at remember_created_at current_sign_in_at 
        last_sign_in_at confirmed_at confirmation_sent_at 
        created_at updated_at
        ].each do |column|
        it { should have_db_column(column.to_sym).of_type(:datetime) }
      end
    end

    describe 'indexes' do 

      %w[confirmation_token email reset_password_token
      ].each do |index|
        it { should have_db_index(index.to_sym).unique(true)}
      end
    end
  end  
end

Hope that helps.

Calorific