Controller tests (Rails 4.2 and RSpec 3.5)

Question

The following code is a controller spec used in a Rails 4.2 application with RSpec 3.5. I would greatly appreciate your suggestions on how to improve it (e.g., efficiency, readability, maintainability, DRYness).

# spec/controllers/authors_controller_spec.rb
require "rails_helper"

RSpec.describe AuthorsController, :focus, :type => :controller do
  login_admin

  let(:author) { FactoryGirl.create(:author) }
  let(:valid_attributes) do
    # The Author model validates the presence of associated records.
    # attributes_for does no create associated records by default.
    FactoryGirl.build(:author, name: "New name").attributes.symbolize_keys.
      except(:id, :slug, :created_at, :updated_at)
  end
  let(:invalid_attributes) { valid_attributes.update(name: nil) }

  describe "GET #index" do
    it "requires login" do
      sign_out login_user
      get :index
      expect(response).to require_login
    end

    it "enforces authorization" do
      get :index
      expect(controller).to enforce_authorization
    end

    it "populates an array of all authors" do
      authors = FactoryGirl.create_pair('author')
      get :index
      expect(assigns(:authors)).to match_array(authors)
    end
  end

  describe "GET #show" do
    it "requires login" do
      sign_out login_user
      get :show, id: author
      expect(response).to require_login
    end

    it "enforces authorization" do
      get :show, id: author
      expect(controller).to enforce_authorization
    end

    it "assigns the requested author to @author" do
      get :show, id: author
      expect(assigns(:author)).to eq(author)
    end
  end

  describe "GET #new" do
    it "requires login" do
      sign_out login_user
      get :new
      expect(response).to require_login
    end

    it "enforces authorization" do
      get :new
      expect(controller).to enforce_authorization
    end

    it "assigns a new Author to @author" do
      get :new
      expect(assigns(:author)).to be_a_new(Author)
    end
  end

  describe "GET #edit" do
    it "requires login" do
      sign_out login_user
      get :edit, id: author
      expect(response).to require_login
    end

    it "enforces authorization" do
      get :edit, id: author
      expect(controller).to enforce_authorization
    end

    it "assigns the requested author to @author" do
      get :edit, id: author
      expect(assigns(:author)).to eq author
    end
  end

  describe "POST #create" do
    it "requires login" do
      sign_out login_user
      post :create, author: valid_attributes
      expect(response).to require_login
    end

    it "enforces authorization" do
      post :create, author: valid_attributes
      expect(controller).to enforce_authorization
    end

    context "with valid attributes" do
      it "saves the new author in the database" do
        expect{
          post :create, author: valid_attributes
        }.to change(Author, :count).by(1)
      end

      it "redirects to authors#show" do
        post :create, author: valid_attributes
        expect(response).to redirect_to author_path(assigns[:author])
      end
    end

    context "with invalid attributes" do
      it "does not save the new author in the database" do
        expect{
          post :create, author: invalid_attributes
        }.not_to change(Author, :count)
      end

      it "re-renders the :new template" do
        post :create, author: invalid_attributes
        expect(response).to render_template :new
      end
    end
  end

  describe "PATCH #update" do
    it "requires login" do
      sign_out login_user
      patch :update, {id: author, author: valid_attributes}
      expect(response).to require_login
    end

    it "enforces authorization" do
      patch :update, {id: author, author: valid_attributes}
      expect(controller).to enforce_authorization
    end

    context "with valid attributes" do
      it "updates the requested author" do
        patch :update, {id: author, author: valid_attributes}
        author.reload
        expect(author).to have_attributes(valid_attributes)
      end

      it "redirects to the updated author" do
        patch :update, id: author, author: valid_attributes
        expect(response).to redirect_to author_url
      end
    end

    context "with invalid attributes" do
      it "does not update the requested author" do
        # Do not attempt to "refactor" the following to any of the following:
        # not_to change { author }
        # not_to change { author.attributes }
        # not_to have_attributes(invalid_attributes)
        # None of the above will work. See
        # https://github.com/rspec/rspec-expectations/issues/996#issuecomment-310729685
        expect {
          patch :update, id: author, author: invalid_attributes
        }.not_to change { author.reload.attributes }
      end

      it "re-renders the :edit template" do
        patch :update, id: author, author: invalid_attributes
        expect(response).to render_template :edit
      end
    end
  end

  describe "DELETE #destroy" do
    let!(:author) { FactoryGirl.create(:author) }

    it "requires login" do
      sign_out login_user
      delete :destroy, id: author
      expect(response).to require_login
    end

    it "enforces authorization" do
      delete :destroy, id: author
      expect(controller).to enforce_authorization
    end

    it "deletes the author" do
      # Must use let! (bang) to create author. Or else, it is both created
      # and deleted INSIDE the block, causing the count not to change.
      expect{
        delete :destroy, id: author
      }.to change(Author, :count).by(-1)
    end

    it "redirects to authors#index" do
      delete :destroy, id: author
      expect(response).to redirect_to authors_url
    end
  end
end

The app uses Devise for authentication. The following custom matcher asserts that unauthenticated users get redirected to the log-in page.

# spec/support/matchers/require_login.rb
RSpec::Matchers.define :require_login do |expected|
  match do |actual|
    expect(actual).to redirect_to \
      Rails.application.routes.url_helpers.new_user_session_path
  end

  failure_message do |actual|
    "expected to require login to access the method"
  end

  failure_message_when_negated do |actual|
    "expected not to require login to access the method"
  end

  description do
    "redirect to the login form"
  end
end

The app uses Pundit for authorization. The following custom matcher asserts that Pundit's authorize or policy_scope method is called within the controller action.

# spec/support/matchers/enforce_authorization.rb
# This custom matcher asserts that Pundit's #authorize or #policy_scope were
# called by the tested controller action. Given that the custom matcher is
# called *after* #authorize or #policy_scope is called, it is unable to mock
# or spy on those methods. Hence, it requires the following lines of code to be
# included in spec_helper.rb
# config.before(:each, :type => :controller) do |spec|
#   allow(controller).to receive('policy_scope').and_call_original
#   allow(controller).to receive('authorize').and_call_original
# end
RSpec::Matchers.define :enforce_authorization do |expected|
  match do |actual|
    if actual.request.parameters[:action] == "index"
      expect(actual).to have_received(:policy_scope)
    else
      expect(actual).to have_received(:authorize)
    end
  end

  failure_message do |actual|
    "expected action to enforce authorization policy"
  end

  failure_message_when_negated do |actual|
    "expected action NOT to enforce authorization policy to access the method"
  end

  description do
    "enforce authorization policy"
  end
end

Thanks in advance.

Answer 1

You've done a good job. The tests are simple and the code communicates its intent well. The comments are excellent, especially the "do not change this code" comment. You've made good use of customized matchers. There isn't much I'd change in this code.

Put login_admin in a before block

In this code,

RSpec.describe AuthorsController, :focus, :type => :controller do
  login_admin
  ...
end

login_admin is run at the time the spec file is loaded, not at the time the test is run. This is a bit of a fine distinction, because the file is loaded just before the test is run, but it does not communicate its intent well. To prevent confusion, it should be moved into a proper before block:

RSpec.describe AuthorsController, :focus, :type => :controller do
  before do
    login_admin
  end
  ...
end

This will log in before each test. It is usually better do to setup actions like this before each test; doing so helps to keep test cases independent of each other.

If you do want to login_admin just once, before any test is run, then change the before block to:

  before(:all) do
    login_admin
  end

Another way to deal with author needing to be a let! block

Another way to deal with author needing to be created outside of the expect is to reference it:

let(:author) { ... }
...
it "deletes the author" do
  # Force author to exist before the `expect` block.
  # Otherwise, it is both created
  # and deleted INSIDE the block, causing the count not to change.
  author
  expect{
    delete :destroy, id: author
  }.to change(Author, :count).by(-1)
end

This is not objectively better than what you've done, except in one way: It keeps the workaround close to where it is needed.