Testing Rails Controllers

Development

Reading Time: 5 minutes

This is a guest blog post by Piotr Solnica. You can find the original article on his blog. We can highly recommend Piotr’s blog! If you want to get in contact with Piotr you can find him on twitter.


It really surprises me that there are people arguing that writing controller tests doesn’t make sense. Probably the most common argument is that actions are covered in acceptance tests along with checking if views are properly rendered. Right? Right…well that’s just wrong! Are you trying to say that your slow acceptance tests are covering every possible controller action scenario? Are you trying to say that, for instance, every redirect that should take place is tested within an acceptance test? Are you also checking every invalid request in acceptance tests? Are you telling me that your acceptance test suite takes 27 hours and 13 minutes to finish because you fully test your controllers there?! Oh I’m sure that your acceptance test suite runs faster and you probably cover only ‘the happy scenarios’ there…which basically means you miss A LOT of test coverage.

A Simple Fact About a Controller

Here’s a simple fact about a controller – it’s a class with methods! Yes, it’s a class with methods I repeat. It should have tests. Every method (action) should be covered by a test.

Why? Well, for the same reason that you write tests for any other class in your application. We want to make sure that our code behaves like expected. It’s also much easier to keep your controllers thin when you have tests. If it’s trivial to write a test for a controller then it’s probably a well implemented controller. If you’re drowning in mocks then you probably want to refactor your controller.

Here’s an example of a users controller with a sample create action:

#!ruby
class UsersController < ApplicationController
  before_filter :load_group

  rescue_from ActiveRecord::RecordNotFound do
    render :not_found
  end

  def create
    @user = @group.users.create(params[:user])

    if @user.persisted?
      redirect users_path, :notice => ‘User created!’
    else
      render :new
    end
  end

  private

  def load_group
    @group = Group.find(params[:group_id])
  end
end

As you can see we load a group object in the before filter and then we use that object to create a new user. Pretty dead-simple. If you just write an acceptance spec for this controller I bet you will not cover the case when the group cannot be found. After all people usually write acceptance tests for optimistic scenarios. If they want to cover every case then I’m sure they will miss the deadline ;)

But it’s not just about covering every path! It’s also about the quality of the code. When you look at the example above you probably won’t see any code smells, right? OK so let me write a spec for the create action:

#!ruby
describe UsersController do
  describe "#create" do
    subject { post :create, :group_id => group_id, :user => attributes }

    let(:group_id) { mock(‘group_id’) }
    let(:group)    { mock(‘group’) }
    let(:user)     { mock(‘user’) }
    let(:users)    { mock(‘users’) }

    before do
      Group.should_receive(:find).with(group_id).and_return(group)
      group.should_receive(:users).and_return(users)
      users.should_receive(:create).with(attributes).and_return(user)
    end

    context ‘when attributes are valid’ do
      it ‘saves the user and redirects to the index page’ do
        user.should_receive(:persisted?).and_return(true)
        subject.should redirect_to(:users)
      end
    end

    context ‘when attributes are not valid’ do
      it ‘saves the user and redirects to the index page’ do
        user.should_receive(:persisted?).and_return(false)
        subject.should render_template(:new)
     end
    end
  end
end

What happens there? Because we reach deeper into group object, get its users and then use it to build a new user the spec requires more mocks then it should. This is a trivial example but you can probably imagine how a spec would look like if the action was more complicated, had more branching logic and even more structural coupling.

Let’s quickly make a small refactor of the action:

#!ruby
class UsersController < ApplicationController
  # stuff

  def create
    @user = @group.create_user(params[:user])

    if @user.persisted?
      redirect users_path, :notice => ‘User created!’
    else
      render :new  
    end
  end

  # more stuff
end

Now the spec will be a bit simpler:

#!ruby
describe UsersController do
  describe "#create" do
    subject { post :create, :group_id => group_id, :user => attributes }

    let(:group_id) { mock(‘group_id’) }
    let(:group)    { mock(‘group’) }
    let(:user)     { mock(‘user’) }

    before do
      Group.should_receive(:find).with(group_id).and_return(group)
      group.should_receive(:create_user).with(attributes).and_return(users)
    end

    context ‘when attributes are valid’ do
      it ‘saves the user and redirects to the index page’ do
        user.should_receive(:persisted?).and_return(true)
        subject.should redirect_to(:users)
      end
    end

    context ‘when attributes are not valid’ do
      it ‘saves the user and redirects to the index page’ do
        user.should_receive(:persisted?).and_return(false)
        subject.should render_template(:new)
      end
    end
  end
end

We should also add an example checking the case where a group is not found. This is going to be a rather rare case but this doesn’t change the fact that we should have a test for it:

#!ruby
describe UsersController do
  subject { post :create, :group_id => group_id, :user => attributes }

  let(:group_id) { mock(‘group_id’) }
 let(:group)    { mock(‘group’) }

  describe ‘#create’ do
    context ‘when group is not found’ do
      before do
        Group.should_receive(:find).with(group_id).
          and_raise(ActiveRecord::RecordNotFound)
      end

      it { should render_template(:not_found) }
    end
  end
end

That’s it! Small test, fast test. Code is covered. Controller is thin. Now it’s probably a good idea to write some views specs for users controller too but I’m not going to write about it here, that’s boring!

Sign up for a free Codeship Account

Summing up

Yes, you should write tests for controllers. Whenever somebody asks you if he/she should write them, your answer should be ‘yes!’. There are no good arguments against writing controller tests. It’s code after all, it should be covered by tests. Let the tests drive your design and you will be happy with your controllers, otherwise you might end up with a mess. Without proper tests you won’t be able to truly verify if your controllers are thin and well designed. So please go and write controller tests!

In case you missed it you might want to read Avdi’s great post about “Law of Demeter” where he writes about structural coupling which is mentioned in this post too.


We want to thank Piotr for making his original blog post available for our readers. Piotr has a lot of interesting articles on his blog. Have a look and be sure to follow him on twitter!

Subscribe via Email

Over 60,000 people from companies like Netflix, Apple, Spotify and O'Reilly are reading our articles.
Subscribe to receive a weekly newsletter with articles around Continuous Integration, Docker, and software development best practices.



We promise that we won't spam you. You can unsubscribe any time.

Join the Discussion

Leave us some comments on what you think about this topic or if you like to add something.

  • zishe

    You wrote the same `it` for different `context`:
    ‘it ‘saves the user and redirects to the index page’ do’

    In the second condition it should be different.

  • steved

    And the test for Group.create_user ? :)

  • gogiel

    I don’t find your tests useful. I write tests because it is easier to refactor code later. You mock AR making it impossible to refactor. Test should check outcome of method, not the way it achieved it. What if you change your load_group method to:

    def load_group
    @group = Group.find_by_id(params[:group_id]) || default_group
    end
    this test fill wil (because find_by_id was used, not find.

  • darrencauthon

    Here’s another reason to write tests this way: Now we can use TDD.

    And when I say TDD, I mean test-driven-design. Not test-first, and not test-a-happy-path-and-then-debate-the-rest. I mean writing a simple failing test, making the test pass with the smallest bit of code possible, and then iterate. I’ve found TDD to be a great design mechanism, enabling me to write complicated software I otherwise couldn’t write. Instead of trying to solve a big problem, I keep solving tiny problems by asking simple questions.

    I’ve been writing Ruby for over two years now, and I’m surprised at how foreign the idea of TDD is in Ruby. I thought me and my orthodox-TDD beliefs would fit in, but I quickly found that TDD isn’t even on the radar of the majority of Rails developers. They write complicated, slow tests through the views, and when this poor testing practice fails they give up or argue that they don’t need to test. I mean, gosh… the Ruby/Rails community supposedly takes testing so seriously, yet here’s an article arguing against the commonly-held belief that Rails programmers shouldn’t test controllers!

    Code is code. If we write it, we should test it… even if it’s in a controller.

  • Zubin

    I agree with your argument that controllers need to be tested, however I’m not so sure about the specs themselves. There should be one assertion per test, however you have two `should_receive`s in the before block and two in each example.

  • Benjamin E. White

    The point about controllers being a class with methods like any other, and therefore in need of unit tests, is compelling (imperfect code example notwithstanding).

  • Controller specs have almost no value for me because:

    1) They are mostly logicless (on rails you mostly delegate the business logic to the models or services, so you end up writing unit tests for them rather than the controller)
    2) Stubs easily break and are a pain in the ass to fix
    3) They are ofter already covered by Integration tests

    I only write controller tests when:

    1) Testing concerns against an anonymous controller.
    2) Testing rack middlewares
    3) Non-active record input sanitization or HTTP header handling

  • Those test examples not only test the desired outcome but the specific implementation.

    You’re testing that a specific chain of methods is called – which I find to be brittle. It makes controllers a lot more expensive to refactor than a standard class with a well defined public API

    • Yeah it’s better to rely on your own interfaces for sure. I must admit I no longer write tests like that. I always introduce my own interfaces and I do mock them in controller tests.

  • Radoslav Stankov

    Good article. Most of my controller actions end with just a “respond_with” call. So in my tests I just stub out rendering most of the time and assert that only “respond_with” is called. Since “respond_with” does the proper thing based on the object state. This technique simplified a lot of my controllers and the specs for them.

  • Grumpy Cat

    No.

  • itsNikolay

    What to test if you use “inherited_resources” ?