Rails Testing Antipatterns: Controllers

This is the third post in the series about antipatterns in testing Rails applications. See part 1 for thoughts on fixtures and factories, and part 2 on models.

Skipping controller tests

When starting out with Rails, with all the “magic” going on it can be easy to forget that controllers are just classes that need tests as well. If it seems to be hard to write a test for a controller, you should embrace the pain; it is telling you that your controller is doing too much and should be refactored.

A controller handles the request and prepares a response. So it shouldn’t be subscribing a user to a newsletter or preparing an order. In general we’ve found that controllers should be limited to:

– Extracting parameters;
– Checking for permissions, if necessary;
– Directly calling one thing from the inside of the app;
– Handling any errors, if they affect the response;
– Rendering a response or redirecting.

I’ve found that controllers are best tested with a mocking approach, since they tie many things together. The freedom you get from mocking should be used as an advantage in molding the controller method’s code to an ideal shape.

Creating records in controller tests

Because Rails controllers should be designed as “callers” of logic defined – and tested – elsewhere, it is pure overhead to involve the database in controller tests. Use mocks and test doubles to decouple from other application layers and stay focused on the task of specifying the functional behavior of the controller.

This is not really a controller spec but a semi-integration test:

describe "GET show" do
  let(:user) { create(:user) }
  let(:report) { create(:report, :user => user) }

  it "fetches user's report" do
    user_id = user.id
    report_file = report
    get :show, :user_id => user_id, :id => report_file.id
    assigns(:report).id.should eql(report.id)
    response.status.should eql(200)
  end
end

The code above is more like an attempt to write a passable test for an existing implementation. Contrast that with writing a test first, thinking only about interfaces, one assertion at a time, without interacting with the database:

describe "GET show" do
  let(:user) { build_stubbed(User) }
  let(:report) { build_stubbed(Report) }

  before do
    User.stub(:find) { user }
    user.stub(:find_report) { report }
  end

  describe "GET show" do
    it "finds the user's report" do
      user.should_receive(:find_report) { report }

      get :show, :user_id => user.id, :id => report.id
    end

    it "assigns a report" do
      get :show, :user_id => user.id, :id => report.id

      assigns(:report).should eql(report)
    end

    it "renders the 'show' template" do
      get :show, :user_id => user.id, :id => report.id

      response.should render_template("show")
      response.code.should eql(200)
    end
  end
end

Sometimes however, data is sensitive enough and goes through some kind of an API to warrant an integration test with real records. This is a use case for _request specs_.

# spec/requests/login_attempts_spec.rb
require "spec_helper"

describe "login attempts" do

  context "failed" do

    it "records a new failed login attempt" do
      expect {
        post user_session_path,
          :user => { :email => "user@example.com", :password => "badpassword" }
      }.to change(LoginAttempt, :count).by(1)
    end
  end
end

No controller tests, only integration

One might argue that it is not necessary to test controllers when there are comprehensive integration tests.

In practice, creating an integration test for every possible edge case is much harder and never really done. Integration tests are also much slower, because they always work with the database and may involve UI testing. Controller specs that use mocks and test doubles are extremely fast.

Complex controller spec setup

Epic setup is generally a code smell, telling us that our class is doing too much. See this context for verifying that a welcome email for a newly registered user goes out:

describe UsersController do

  describe "POST create" do

    context "on successful save" do

      before do
        @user = double(User)
        @user.stub(:save).and_return(true)

        User.stub(:new) { @user }

        @mailer = double(UserMailer)
        UserMailer.stub(:welcome) { @mailer }
        @mailer.stub(:deliver)

        ReferralConnector.stub(:execute)
        NewsletterSignupService.stub(:execute)
      end

      it "sends a welcome email" do
        @mailer.should_receive(:deliver)

        post :create, :user => { :email => "jo@example.com" }
      end
    end
  end
end

Difficult tests always go hand in hand with a problematic implementation. The spec above is for a controller that looks something like this:

class UsersController

  def create
    @user = User.new(user_params)

    if @user.save
      UserMailer.welcome(@user).deliver
      ReferralConnector.execute(@user)
      NewsletterSignupService.execute(email, request.ip)

      redirect_to dashboard_path
    else
      render :new, :error => "Registration failed"
    end
  end

  private

  def user_params
    params[:user].permit(*User::ACCESSIBLE_ATTRIBUTES)
  end
end

A developer following the BDD approach and thinking a bit in advance could write a spec for a method handling user registration so that all post-signup things, whatever they may be, are delegated to a PostSignup class, which can be tested in isolation.

describe "POST create" do

  context "on successful save" do

    before do
      User.any_instance.stub(:save) { true }
      PostSignup.any_instance.stub(:run)
    end

    it "runs the post signup process" do
      PostSignup.should_receive(:new) { post_signup }
      post_signup.should_receive(:run)

      post :create
    end

    it "redirects to dashboard" do
      post :create
      response.should redirect_to(dashboard_path)
    end
  end
end