Yes, You Should Write Controller Tests!

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 acceptence 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:

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:

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:

Now the spec will be a bit simpler:

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:

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 ;)

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.

  • Peter Jaros

    You know, there’s still structural coupling here.  The controller is calling Group.find(5).create_user.  That’s a Demeter violation (and a concrete dependency).

    This is why I hate controller tests: because Rails doesn’t want me to write my controllers like real objects.  Turning group_id: 5 into a Group?  That should be some other object’s responsibility.  And when I test that object, I expect to be able to inject Group.

    But this object I don’t even *construct* in my test.  I have to call post :create.  All of the OO design benefits of TDD fly out the window.  Plus I have to write a lot of crappy boilerplate.  And my test has to either know what parameters are valid for my model, or be structurally coupled to my model to stub validation.  Either option impairs readability and design feedback.  It’s noise that I repeat for *every gods-damned controller test*.

    So that’s why I hate controller tests.  I still write them.  But usually I let my pair do the typing.

    • http://solnic.eu/ solnic

      Agreed! But it’s rails, so what can you do? I could follow up with a post “Why controller tests in rails SUCK” but on the other hand I really want people to write them, even if they are not pure unit tests :)

      • http://www.seven-sigma.com/ Jeff Dickey

        @Peeja:disqus, do you think this is a *Rails* design flaw, or is it something that would be inherent in any Web framework (Sinatra/Padrino, Renee, etc.), or is it a misfeature of HTTP in general? I’ve only been back in Ruby for a bit over a year, after a decade in the PHP and Python wilderness (where even suckier bugbears lurk). I’m not trying to start a pissing war here; I see the flaws in the sample code (and in the Rails code I’ve been writing). The question remains, how much effort at what level would be required to get a clean object model we could deal with?

        Late to the party as usual…

        • Peter Jaros

          It’s a great question. I don’t think it’s in any way inherent to HTTP. The solution that I like is that your controller object doesn’t inherit behavior from a framework class, but instead is instantiated with a smart request object which offers a similar interface to the private methods AC::Base supplies. Now I can provide my own request object in a test, and thus test the object in isolation without any dangerous tricks.

        • http://solnic.eu/ solnic

          I think it is a design flaw in rails. I imagine it would be better if you could provide a custom object to handle any request rather than having some magic controller object inheriting from framework abstract controller class.

          I’m not even mentioning sharing-controller-ivars-with-views-paranoia that I’m avoiding nowadays.

          • http://www.seven-sigma.com/ Jeff Dickey

            @solnic:disqus After poking around knee-deep in the ActiveController ooze for a couple of days, I’m inclined to agree. I’m presently on a project in grave danger of becoming a Death March, and trying to write simple, clean BDD with Rails (after a dozen BDD projects in other languages) has been a big part of the problem.

            One of my hats at work is setting tech/tools policy and standards; I’m looking wistfully at other Ruby Web frameworks to see what obvious land mines they carry. Everything I’ve read about Rails 4 leads me to believe that it won’t be significantly more BDD-friendly than Rails 3.2.x. I hope to have small PoC projects in Renee and Padrino up by the end of the month. If anybody has any useful comments about them or about other BDD-friendly alternatives, please email me off list.

          • http://solnic.eu/ solnic

            Rails 4 will be exactly the same as previous versions. Rails doesn’t change much and I don’t expect it to change to be honest. You will see a lot of improvements, but those are details, at its core Rails will probably stay the same.

  • Justin Ko

    It’s so nice to see a controller spec with describe '#create' do. Unless the action responds to multiple HTTP verbs, it doesn’t make sense to write the example like this:

    describe ‘POST to create’ do
    end

    I think this stems from the old non-REST days. Either way, the HTTP verb is implied when using REST/CRUD in Rails.

    Even with custom routes, I still don’t specify the verb because most of them will only respond to one type (or at least they should).

    • http://github.com/dkubb Dan Kubb

      Justin, I know you’re on the rspec team and I wanted to ask if you think the way Piotr is declaring/using the subject above in a way that was intended?

      Piotr and I work together and while we both follow this style, there was question a few months ago on whether or not we were abusing subject. While we were using it to describe the method that was under unit test, it was thought that the subject should be the main object under test.

      • brynary

        IMHO, this is an abuse of #subject. I use explicit #subject declarations sparingly, but when I do I believe it should be an object under test (approximate to a “Given”). This use of #subject is capturing the result of an action performed (a “When”).

        Instead of #subject, I’d either repeat the invokation of the controller action, or extract a method in the spec that does the same thing.

        • http://solnic.eu/ solnic

          I started writing my specs like that because it reduces the code I need to write to the minimum. Even if it’s an abuse of subject method. I may write some extension that would allow me to write such short specs but using a different syntax that would be more semantically correct.

      • Justin Ko

        subject was added to RSpec for Shoulda matchers. It was never really intended to be a public API — the same as described_class.

        I believe it should only be used in the context of “implicit receiver”:

        describe Array do  describe ‘when initialized with no arguments’ do    it { should be_empty }  endend

        However, I’m still not a fan of this. For example, with the above code, it is not clear what the expectation is being called upon. It abstracts too much. This is much more clear:

        describe Array do  describe ‘when initialized with no arguments’ do    let(:array) { Array.new }    specify { array.should be_empty }  endend

        I can clearly see that Array.new is the “subject” under test, and what “initialized with no arguments” means in code. But, to contradict myself, this is an amazing use case for implicit receiver:

        describe 5 do
          it { should be 4 }
        end

        Unfortunately, the subject in “real world specs” are rarely, if ever, as simple as a Fixnum.

        Bottom line, every subject that I see out in the wild can be replaced with a let. In solnic’s example, it would become this:

        let(:response) { post :create, group_id: group_id, user: attributes }
        specify { response.should redirect_to(:user) }

        With all of that said, I do use shoulda-matchers, but only because Rails controllers are so predictable and limited: they only render and redirect. I don’t place a high importance on their example descriptions.

    • Peter Jaros

      But isn’t that a lie?  You’re not calling #create, you’re calling post :create.  Your test still knows the verb.

      • http://solnic.eu/ solnic

        Yes, it’s not a pure unit test cause Rails doesn’t allow you to write proper unit tests for controllers. I like to *think* about those tests as unit tests though. It’s the best I can do at least.

      • Justin Ko

        post :create happens to be how you call the method. Any other verb will not be recognized (ex. get :create). The “create” action is still an instance method of the controller class.

        If were testing a private method (which you shouldn’t), you  wouldn’t write the spec like describe "SEND to the_method" do for send(:the_method).

  • jdudek

    I almost never write tests for controllers. If I had a custom rescue_from, I’d cover it in one acceptance test. If I had “more branching logic and even more structural coupling” then I’d clearly see it in the code and move it to the model. Isn’t following a simple rule like “call only 1 or 2 model methods” enough?

    For me controllers should only delegate to the model and render or redirect. It’s not that much work to cover in acceptance tests (especially when using fast testing tool like Rack::Test). And I don’t see much design when writing controllers.

    Maybe there is some value in writing unit tests for simple controllers like the one in article, but it’s just a lot of effort that I don’t believe to be justified. Or maybe it’s just my lack of experience and I’ll see this value a few thousands lines of code later :)

    • http://solnic.eu/ solnic

      I simply cannot imagine a bigger project with a full test coverage where controller’s logic is covered by acceptance specs.

      Regarding “clearly seeing” code smells in controllers (or anywhere else of course)…I know that when you have some experience it’s easy to find code smells by “looking at the code” but think about it from the other way around – if you wrote tests you probably wouldn’t have those code smells :)

      • jdudek

        Yeah, probably I just haven’t worked on big enough projects.

        I often write integration tests which are not strictly end-to-end. For example, when server provides just the API called from the client, I prefer to have separate tests for the client- and server-side code. Server-side tests make requests and verify responses. They are usually quite fast and it’s possible to cover whole controller logic this way. I don’t think I could achieve this with Selenium scenarios… which are slow and unreliable. But the price is of course that I don’t test the whole integration between server and client.

        I’m still not convinced that benefits of writing unit tests for every controller outweigh the costs. But maybe I just need to learn it the hard way :)

    • Artur Roszczyk

      C’mon, code related to certain use case should not be in the model!

      • jdudek

        My point is that controllers usually delegate work to other objects, be it models or explicit use-case classes. But if choose to keep your logic in controllers than I’d agree that it’s good to unit-test them.

  • Tim Case

    Nope, don’t agree this, and I don’t believe in 100% test coverage either.  Given enough experience with rails apps you’ll see problems tend to crop up in certain places, not just when the code is initially written but especially over time as changes are made, and controllers are not one of these places.  Assuming that you are working to keep your models fat and your controllers skinny, I think a good strategy is to short change your controller tests and put more effort into writing really good model tests that place intense focus on your domain logic.  Don’t sweat the small stuff, don’t sweat controllers.  As far as refactoring controller code into better code through testing…  Well maybe like once or twice, but if you are constantly refactoring your controller code, something wrong is going on, controller code should get standard as time goes on.

    • http://solnic.eu/ solnic

      What do you mean you don’t believe in 100% test coverage? There’s nothing to believe in. You either have it or you don’t have it. And probably you really really want to have it.

      If controllers are skinny and their code doesn’t change too much – then it’s great, but that does not change the fact that you should cover that code in your tests. Right now this just boils down to the principles of TDD.

  • http://dockyard.com Brian Cardarella

    I am in the “don’t test your controllers” camp. Not because I don’t think it is important but because it always sucks and I feel like my integration tests are hitting this anyway. However, there have been times in which I would prefer to unit test my controllers. I think that is the true issue here, there is no clear and easy path to unit testing my controller classes. The setup and tear downs are a pain in the ass. Rspec certainly helps quite a bit but it is never as straight forward as with other classes.

    • http://solnic.eu/ solnic

      Totally agree on your point about no clear and easy path but I still believe (no, I don’t believe, I simply KNOW it) that it’s worth to write these tests even if it’s not following rules of proper unit testing. Controller tests saved my ass many times whereas integration specs are simply too huge to be able to cover everything with them.

  • http://avdi.org Avdi Grimm

    Recently I’ve started doing some controller tests again. FWIW, when I do *not* use Rails controller helpers. And I don’t make the test dependent on Rails being loaded. I just unit test the controller like any other plain ‘ole object.

    • Peter Jaros

      How do you write an isolated unit test for a Rails controller?  (I feel like I’ve asked you this before.)

      • http://avdi.org Avdi Grimm

        You know, I’m going to have to start charging you a beer per blog post answering your questions ;-)

        • Peter Jaros

          I accept your terms.

        • http://twitter.com/donaldball donaldball

          That’s a bargain at twice the price. Can anyone get in on this action?

        • http://twitter.com/nicholaides nicholaides

          I’ll gladly contribute to the Avdi Grim Beer-for-Blogs Fund. (or should that be Bourbon-for-Blogs?)

    • http://solnic.eu/ solnic

      I’d love to do that too. I think we used to do that in Merb times. Or maybe I’m wrong :) I need to investigate what’s possible in Rails.

  • http://twitter.com/alovak Pavel Gabriel

    I don’t like mocks because if you will make a refactoring inside your controller later you will have to change your specs.

    I only worry about this:

    expect { post :create, … }.to change(User.count).by(1)

    should redirect_to

    should render_template :new

    and so on.

    • http://avdi.org Avdi Grimm

      Most folks who don’t like mocks for that reason never learned to use them well. Gregory Moeck has a good talk on the subject: http://confreaks.com/videos/659-rubyconf2011-why-you-don-t-get-mock-objects

    • http://solnic.eu/ solnic

      The thing is I don’t want to hit the DB in controller tests…if I call a model method which changes user’s count then I don’t care about its result, I mock it and check if it’s called properly. This method is obviously fully covered in a model unit test.

      • Justin Ko

        Exactly. Only the model and integration/request/acceptance tests should be hitting the DB, never controllers.

    • Peter Jaros

      No, if you change the *behavior* of your controller, you will have to change your specs.  If you refactor it, the specs (and the behavior) stay the same.  Interacting with a collaborator, like a model class, is behavior, not an implementation detail.  Unless you consider your controller object to own the model entirely, and never use the model outside of the controller.  But surely you want your models to be able to interact with other models, observers, views, and other peers in your app.  If a peer sends a message to a peer, that’s behavior, and if it’s not in the unit test it’s untested behavior.

      • josh_cheek

        Disagree for a couple of reasons.

        1. AR::B instances have hundreds of methods. Your controller could call create, or call new and then save later. They accomplish the same thing, but they are done differently, your tests shouldn’t have to change around this. The api is too large to test the interaction between the controller and the model, it’s better to test the state of the models before and after (or possibly to have another object which does the interesting bits for you, which you can then mock out).

        2. I think it is much wiser to think of AR::B instances like data structures than like objects. When you pass a string to some object and it calls upcase on the string, you don’t pass in doubles for strings and assert that upcase was called on them. That’s again testing implementation, and doesn’t check the result, just the implementation. In the same way, the interesting behaviour is that the object was created, not that the model had create invoked vs the new method and a save.

        • Peter Jaros

          Okay, that’s all fair.  And that’s why interacting with ActiveRecord objects in a pain in the ass.  How is it that we overload an object with an insane amount of behavior and still treat it like a data structure.

          Better, I think, to interact with a domain object that *doesn’t* have hundreds of methods (because, really, that’s insane).  Then write actual unit tests for all objects involved.

          There’s a principle here that I think is missing from SOLID.  I’ll call it the One Way Principle.  There should be one and only one way to tell an object to do something it can do.  ActiveRecord objects fail this test hard, and that’s why it’s so hard to make expectations about interactions with them.

          • http://solnic.eu/ solnic

            Yup, I completely agree with you, I even wrote about it (not using AR directly) here: http://solnic.eu/2011/08/01/making-activerecord-models-thin.html

  • diabolist

    You argue that     

    “If you just write an acceptance spec for this controller I bet you will not cover the case when the group cannot be found”. 

    I would argue that you shouldn’t be able to mention group in your controller without having some acceptance tests in your project for group. So the acceptance test for the case where the group does not exist is   

    Given there are no groups   
    When I create a user   
    Then …

    This is actually much easier to write than your controller spec, uses already defined step defs and gives the business the opportunity to specify more about a groups behaviour.Your mechanism for testing what happens if there is no group is also very fragile.

    You have   

    before do     
    Group.should_receive(:find).with(group_id).and_return(group)

    Why should Group receive this message? This is a user_controller_spec its got nothing to do how Group is implemented

    So what happens when you write specs like this. Well you change your model implementation and suddenly loads of controller specs that have nothing to do with your model break.

    This is all the fault of rails and the way the controller model interface just sucks. I would argue that controller tests acerbate the problem and undermine the value of the rest of your test suite.

    This is why I don’t write controller specs, write anaemic controllers, and write acceptance criteria that deal with the routing, error message and business response if the group doesn’t exist. In the long run its much easier and gives better coverage (at the business level) as well as with rcov.

    • Peter Jaros

      > Why should Group receive this message? This is a user_controller_spec its got nothing to do how Group is implemented

      That’s not the implementation of Group.  That’s *exactly* not the implementation of group.  That’s the interface to Group.  That’s the part you *should* know.

  • http://twitter.com/chamnap Chamnap Chhorn

    If I write controller spec, it’ll duplicate some in the integration tests. Or just write the integration tests for only success cases?

    • Justin Ko

      No one has a clear answer on this.

      I’ve recently been questioning the value of integration/request specs. I always spec my controllers, and when I do write request specs, it does feel like duplication.

      The idea of having a single request spec that basically “runs through the app” is beginning to appeal to me. This is just to catch errors, not verify anything.

      • http://solnic.eu/ solnic

        When you start treating controller specs as unit tests for controllers then things become clear :)

        One thing I consider as a really good practice is to test error handling only on controller-spec level and skip it in request specs. For instance it’s a common practice to display the same 404 page in certain cases – testing it in request specs seems like an overkill.

    • Peter Jaros

      Let’s take this out of the context of controller tests:

      Any time you have unit tests and integration tests (that is, higher-level tests), you’ll be testing the same code twice, by definition.  This works best when the higher-level tests are designed to exercise as much interaction of the components as they can without being long-winded.  You needn’t test the basic correctness of the application in integration tests; save that for the unit tests, or you’ll be crippled by a combinatorial morass of integration test cases.  Just test enough to demonstrate that things integrate correctly.

      J.B. Rainsberger wrote a series of articles on this topic.  They seem to be missing now, but InfoQ wrote up a nice summary: http://www.infoq.com/news/2009/04/jbrains-integration-test-scam

      As for controller/request integration tests, the same rule applies: demonstrate that the components hook up and don’t blow up.  That might mean only testing a happy path.  Use your judgement and your understanding of how the underlying objects work together.  If they can hook up in a variety of ways, it might be worth writing a few different test cases.

      The upshot is that you’ll definitely be testing the same objects, but you won’t be duplicating the intent of your tests.  If you’re testing, say, validation logic in an integration test, that’s a smell.

  • http://www.stefanhuska.sk Štefan Húska

    I write test for EVERY controller. Even if its action is only rendering some static template.

    Every time I see a failing test while editing code, I’m almost happy, because I can catch bug and solve it before client found it.

    Simple rule: Write tests, sleep well.
    Thanks for article.