I intend this to be a general question on writing an effective set of test cases for a controller action.

I include the following ingredients:

Ruby on Rails

RSpec: A testing framework. I considered doing a vanilla Rails question, but I personally use RSpec, and I feel many other folks do too. The principles that come out of this discussion should be portable to other testing frameworks, though.

Will Paginate: I include this to provide an example of code whose implementation is blackboxed to the controller. This is an extreme example of just using the model methods like @programs = Program.all. I chose to go this route to incorporate an additional factor for discussion, and to demonstrate that the same principles apply whether using external application code (e.g., model code) or an external plugin.

There seems to be a lack, given my humble Google Fu, of style guides for RSpec testing at this level, so it is my hope that, on top of me improving my code, this can become a useful guide for my fellow travelers.

For example purposes, let's say I currently have in my controller the following:

Sidebar: For those unfamiliar with will_paginate, it tacks onto a ActiveRecord Relation (all, first, count, to_a are other examples of such methods) and delivers a paginated result set of class WillPaginate::Collection, which basically behaves like an array with a few helpful member methods.

What are the effective tests I should run in this situation? Using RSpec, this is what I've conceived at the moment:

describe ProgramsController do
def mock_program(stubs={})
@mock_program ||= mock_unique_program(stubs)
end
def mock_unique_program(stubs={})
mock_model(Program).as_null_object.tap do |program|
program.stub(stubs) unless stubs.empty?
end
end
describe "GET index" do
it "assigns @programs" do
Program.stub(:paginate) { [mock_program] }
get :index
response.should be_success
assigns(:programs).should == [mock_program]
end
it "defaults to showing 30 results per page" do
Program.should_receive(:paginate).with(:page => nil, :per_page => 30) do
[mock_program]
end
get :index
response.should be_success
assigns(:programs).should == [mock_program]
end
it "passes on the page number to will_paginate" do
Program.should_receive(:paginate).with(:page => '3', :per_page => 30) do
[mock_program]
end
get :index, 'page' => '3'
response.should be_success
assigns(:programs).should == [mock_program]
end
it "passes on the per_page to will_paginate" do
Program.should_receive(:paginate).with(:page => nil, :per_page => '15') do
[mock_program]
end
get :index, 'per_page' => '15'
response.should be_success
assigns(:programs).should == [mock_program]
end
end
end

I wrote this with the following principles in mind:

Don't test non-controller code: I don't delve into the actual workings of will_paginate at all and abstract away from its results.

Test all controller code: The controller does four things: assigns @programs, passes on the page parameter to the model, passes on the per_page parameter to the model, and defaults the per_page parameter to 30, and nothing else. Each of these things are tested.

No false positives: If you take away the method body of index, all of the test will fail.

Mock when possible: There are no database accesses (other ApplicationController logic notwithstanding)

My concerns, and hence the reason I post it here:

Am I being too pedantic? I understand that TDD is rooted in rigorous testing. Indeed, if this controller acts as a part of a wider application, and say, it stopped passing on page, the resulting behaviour would be undesirable. Nevertheless, is it appropriate to test such elementary code with such rigor?

Am I testing things I shouldn't be? Am I not testing things I should? It think I've done an okay job at this, if anything veering on the side of testing too much.

Are the will_paginate mocks appropriate? You see, for instance, with the final three tests, I return an array containing only one program, whereas will_paginate is quite liable to return more. While actually delving this behaviour by adding, say 31 records, may (?) violate modular code and testing, I am a bit uneasy returning an unrealistic or restrictive mock result.

Can the test code be simplified? This seems quite long to test one line of controller code. While I fully appreciate the awesomeness of TDD, this seems like it would bog me down. While writing these test cases were not too intensive on the brain and basically amounted to a fun vim drill, I feel that, all things considered, writing this set of tests may cost more time than it saves, even in the long run.

4 Answers
4

You forgot to test what view should be rendered. If you use this, your specs will be much cleaner. The last four should be standard matchers. See this as an example.

describe ProgramsController do
let(:programs) { [mock_model(Program)] }
describe "GET index" do
it { should paginate(Program).with_default_per_page(30) }
describe "after pagination" do
before(:each) do
Program.stub(:paginate).and_return(programs)
get :index
end
it { should assign_to(:programs).with(programs) }
it { should respond_with(:success) }
it { should render_template(:index) }
it { should_not set_the_flash }
end
end
end

My only real suggestion is to have just one assertion per test, including any #should_receive or #should= statements. The intention of most of your tests seems to be to test one single unit of functionality, so there is no need to repeat assertions, particularly when they are dependent. It will help focus more clearly on the aim of the individual tests, reducing their volume and heightening the readability.

As far as RSpec style, there are a few idioms I have seen more commonly used:

using #and_return in place of returning values from blocks

using mock_model

doing setup in before blocks

Overall, I would likely have written it like this:

describe ProgramsController do
before(:each) do
@mock_programs = [mock_model(Program)]
Program.stub(:paginate).and_return(@mock_programs)
end
describe "GET index" do
it "succeeds" do
get :index
response.should be_success
end
it "renders the index template" do
get :index
response.should render_template("programs/index")
end
it "assigns @programs" do
get :index
assigns(:programs).should == [mock_program]
end
it "defaults to showing 30 results per page" do
Program.should_receive(:paginate).with(:page => nil, :per_page => 30).and_return(@mock_programs)
get :index
end
it "passes on the page number to will_paginate" do
Program.should_receive(:paginate).with(:page => '3', :per_page => 30).and_return(@mock_programs)
get :index, 'page' => '3'
end
it "passes on the per_page to will_paginate" do
Program.should_receive(:paginate).with(:page => nil, :per_page => '15').and_return(@mock_programs)
get :index, 'per_page' => '15'
end
end
end

I think, for the limited scope which you've provided, this is a good set of tests. You may be able to skip the test for basic success since all your other tests are dependent on a successful action. I personally can't speak too deeply about testing will_paginate, since I do much the same when testing the use of it from the controller. If it's tested well in the model that uses it, you've covered the basic behavior.

Any compelling reason why you're not checking the assigns(:programs) in the last three tests? I assume it's because otherwise, the call to will_paginate with arguments would just fail. Correct?
–
Steven XuFeb 2 '11 at 18:13

Correct. I only stub program#paginate to avoid the will_paginate failure. Depending on how often I stub something, I may also move it to the before block of a specific context. I figure that I've already tested the assigns once and, without further behavior that may alter the contents of the hash, I do not need to verify it works in other examples.
–
Denny AbrahamFeb 5 '11 at 21:05

@PavelDruzyak above makes a good point. We've tested the interaction of the controller with the model, but not with the view layer. Whether you choose to use RSpec or Shoulda matchers (when I last checked, several weeks ago, the latest versions of each were incompatible, though that may be fixed), you ultimately should go beyond verifying the response was a success and ensure the appropriate response is returned
–
Denny AbrahamFeb 10 '11 at 19:35

just something that i have considered how about hiding the paginate stuff behind a helper method and then stub the helper method this way you can swap out the pagination implementation with anytime you want
and then just test the helper method is delegating the params to will_paginate

I'm a few years out of RoR, but worked for a company that was heavy on testing an we used RSpec.

In a case like this we would have tested some additional points, mainly about the content of the response. A few lines like (syntax may be totally wrong, as said I'm out of this):

test the amount of records
@programs.should.have(30).records

then there was a command to test if certain records are in the result
@programs.should.contain(...)
this requires fixtures of course.

The idea behind this was to make sure we could replace a certain plugin against a new one and be sure it doesn't break anything or contains bugs. (For example the acts_as_taggable plugin had a bug that made it impossible to delete tags, thing was running into an infinite loop)
And of course to avoid programmers to make changes while testing in the browser or similar. With will_paginate a typical bug would be to set the pagination to 10 to see several pages in the browser (because there are only 20 records in the fixtures...) and forget to set this value back to 30. And if it's not the programmer, then maybe the guy working on the HTML/CSS. We made sure every of those details were tested

More tests would make sure that the right file would be used for rendering.
something like response.should.render_template(name)

This seems to be a lot of work. But on the positive side: When my first web site for a customer went online, after six weeks of programming and my humble 3 months of knowing about Ruby on Rails, it had no bugs at all and stood 1.000 hits/minute without a single line of code to be changed.