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.

• BrunoF: I have implemented both of your suggestions. Thank you.