David Chelimsky new controller examples

new controller examples

June 30th, 2008

There’s been a lot of discussion about clarity over DRY lately. This is something that I’ve been espousing for some time, but recent posts by Jay Fields, Mikel Lindsaar and Dan North have gotten me thinking about it again with more focus.

With this in mind, I’ve been refining the examples generated for restful controllers when you run script/generate rspec_scaffold with the rspec-rails plugin. I’ve got them now where I’m pretty happy with them, but I’m curious to hear what you think. I’m not going to tell you what I changed or what to look for, I’m just going to ask you to look it over and post your comments.

There are two listings: the generated code and the output you get from running the examples. Thanks in advance for any feedback.

script/generate rspec_scaffold account

require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')

describe AccountsController do

  def mock_account(stubs={})
    stubs = {
      :save => true,
      :update_attributes => true,
      :destroy => true,
      :to_xml => ''
    }.merge(stubs)
    @mock_account ||= mock_model(Account, stubs)
  end

  describe "responding to GET /accounts" do

    it "should succeed" do
      Account.stub!(:find)
      get :index
      response.should be_success
    end

    it "should render the 'index' template" do
      Account.stub!(:find)
      get :index
      response.should render_template('index')
    end

    it "should find all accounts" do
      Account.should_receive(:find).with(:all)
      get :index
    end

    it "should assign the found accounts for the view" do
      Account.should_receive(:find).and_return([mock_account])
      get :index
      assigns[:accounts].should == [mock_account]
    end

  end

  describe "responding to GET /accounts.xml" do

    before(:each) do
      request.env["HTTP_ACCEPT"] = "application/xml" 
    end

    it "should succeed" do
      Account.stub!(:find).and_return([])
      get :index
      response.should be_success
    end

    it "should find all accounts" do
      Account.should_receive(:find).with(:all).and_return([])
      get :index
    end

    it "should render the found accounts as xml" do
      Account.should_receive(:find).and_return(accounts = mock("Array of Accounts"))
      accounts.should_receive(:to_xml).and_return("generated XML")
      get :index
      response.body.should == "generated XML" 
    end

  end

  describe "responding to GET /accounts/1" do

    it "should succeed" do
      Account.stub!(:find)
      get :show, :id => "1" 
      response.should be_success
    end

    it "should render the 'show' template" do
      Account.stub!(:find)
      get :show, :id => "1" 
      response.should render_template('show')
    end

    it "should find the requested account" do
      Account.should_receive(:find).with("37")
      get :show, :id => "37" 
    end

    it "should assign the found account for the view" do
      Account.should_receive(:find).and_return(mock_account)
      get :show, :id => "1" 
      assigns[:account].should equal(mock_account)
    end

  end

  describe "responding to GET /accounts/1.xml" do

    before(:each) do
      request.env["HTTP_ACCEPT"] = "application/xml" 
    end

    it "should succeed" do
      Account.stub!(:find).and_return(mock_account)
      get :show, :id => "1" 
      response.should be_success
    end

    it "should find the account requested" do
      Account.should_receive(:find).with("37").and_return(mock_account)
      get :show, :id => "37" 
    end

    it "should render the found account as xml" do
      Account.should_receive(:find).and_return(mock_account)
      mock_account.should_receive(:to_xml).and_return("generated XML")
      get :show, :id => "1" 
      response.body.should == "generated XML" 
    end

  end

  describe "responding to GET /accounts/new" do

    it "should succeed" do
      get :new
      response.should be_success
    end

    it "should render the 'new' template" do
      get :new
      response.should render_template('new')
    end

    it "should create a new account" do
      Account.should_receive(:new)
      get :new
    end

    it "should assign the new account for the view" do
      Account.should_receive(:new).and_return(mock_account)
      get :new
      assigns[:account].should equal(mock_account)
    end

  end

  describe "responding to GET /accounts/1/edit" do

    it "should succeed" do
      Account.stub!(:find)
      get :edit, :id => "1" 
      response.should be_success
    end

    it "should render the 'edit' template" do
      Account.stub!(:find)
      get :edit, :id => "1" 
      response.should render_template('edit')
    end

    it "should find the requested account" do
      Account.should_receive(:find).with("37")
      get :edit, :id => "37" 
    end

    it "should assign the found Account for the view" do
      Account.should_receive(:find).and_return(mock_account)
      get :edit, :id => "1" 
      assigns[:account].should equal(mock_account)
    end

  end

  describe "responding to POST /accounts" do

    describe "with successful save" do

      it "should create a new account" do
        Account.should_receive(:new).with({'these' => 'params'}).and_return(mock_account)
        post :create, :account => {:these => 'params'}
      end

      it "should assign the created account for the view" do
        Account.stub!(:new).and_return(mock_account)
        post :create, :account => {}
        assigns(:account).should equal(mock_account)
      end

      it "should redirect to the created account" do
        Account.stub!(:new).and_return(mock_account)
        post :create, :account => {}
        response.should redirect_to(account_url(mock_account))
      end

    end

    describe "with failed save" do

      it "should create a new account" do
        Account.should_receive(:new).with({'these' => 'params'}).and_return(mock_account(:save => false))
        post :create, :account => {:these => 'params'}
      end

      it "should assign the invalid account for the view" do
        Account.stub!(:new).and_return(mock_account(:save => false))
        post :create, :account => {}
        assigns(:account).should equal(mock_account)
      end

      it "should re-render the 'new' template" do
        Account.stub!(:new).and_return(mock_account(:save => false))
        post :create, :account => {}
        response.should render_template('new')
      end

    end

  end

  describe "responding to PUT /accounts/1" do

    describe "with successful update" do

      it "should find the requested account" do
        Account.should_receive(:find).with("37").and_return(mock_account)
        put :update, :id => "37" 
      end

      it "should update the found account" do
        Account.stub!(:find).and_return(mock_account)
        mock_account.should_receive(:update_attributes).with({'these' => 'params'})
        put :update, :id => "1", :account => {:these => 'params'}
      end

      it "should assign the found account for the view" do
        Account.stub!(:find).and_return(mock_account)
        put :update, :id => "1" 
        assigns(:account).should equal(mock_account)
      end

      it "should redirect to the account" do
        Account.stub!(:find).and_return(mock_account)
        put :update, :id => "1" 
        response.should redirect_to(account_url(mock_account))
      end

    end

    describe "with failed update" do

      it "should find the requested account" do
        Account.should_receive(:find).with("37").and_return(mock_account(:update_attributes => false))
        put :update, :id => "37" 
      end

      it "should update the found account" do
        Account.stub!(:find).and_return(mock_account)
        mock_account.should_receive(:update_attributes).with({'these' => 'params'})
        put :update, :id => "1", :account => {:these => 'params'}
      end

      it "should assign the found account for the view" do
        Account.stub!(:find).and_return(mock_account(:update_attributes => false))
        put :update, :id => "1" 
        assigns(:account).should equal(mock_account)
      end

      it "should re-render the 'edit' template" do
        Account.stub!(:find).and_return(mock_account(:update_attributes => false))
        put :update, :id => "1" 
        response.should render_template('edit')
      end

    end

  end

  describe "responding to DELETE /accounts/1" do

    it "should find the account requested" do
      Account.should_receive(:find).with("37").and_return(mock_account)
      delete :destroy, :id => "37" 
    end

    it "should call destroy on the found account" do
      Account.stub!(:find).and_return(mock_account)
      mock_account.should_receive(:destroy)
      delete :destroy, :id => "1" 
    end

    it "should redirect to the accounts list" do
      Account.stub!(:find).and_return(mock_account)
      delete :destroy, :id => "1" 
      response.should redirect_to(accounts_url)
    end

  end

end

script/spec spec/controllers/accounts_controller_spec.rb -fn

AccountsController
  responding to GET /accounts
    should succeed
    should render the 'index' template
    should find all accounts
    should assign the found accounts for the view
  responding to GET /accounts.xml
    should succeed
    should find all accounts
    should render the found accounts as xml
  responding to GET /accounts/1
    should succeed
    should render the 'show' template
    should find the requested account
    should assign the found account for the view
  responding to GET /accounts/1.xml
    should succeed
    should find the account requested
    should render the found account as xml
  responding to GET /accounts/new
    should succeed
    should render the 'new' template
    should create a new account
    should assign the new account for the view
  responding to GET /accounts/1/edit
    should succeed
    should render the 'edit' template
    should find the requested account
    should assign the found Account for the view
  responding to POST /accounts
    with successful save
      should create a new account
      should assign the created account for the view
      should redirect to the created account
    with failed save
      should create a new account
      should assign the invalid account for the view
      should re-render the 'new' template
  responding to PUT /accounts/1
    with successful update
      should find the requested account
      should update the found account
      should assign the found account for the view
      should redirect to the account
    with failed update
      should find the requested account
      should update the found account
      should assign the found account for the view
      should re-render the 'edit' template
  responding to DELETE /accounts/1
    should find the account requested
    should call destroy on the found account
    should redirect to the accounts list

Tags: clarity, dry, generator, rails, rspec

16 Responses to “new controller examples”

Pat Maddox Says:
July 13th, 2008 at 06:06 AM
I like it. A couple comments:

As I read the specs, I thought of different orders in which people might write the examples. I’ve seen several people that write them in the same order you did here, but might organize them in the opposite order. That is, they’d start with “should be_success” as the first one, but then insert the “should render_template” example above it.

I really like the ordering you’ve used though. It does seem to tell a story to me:

“What should the accounts index do?” “It should show a page” “Which page?” “The index page” “What must go on the page?” “Accounts”

When I write specs for actions with multiple respond_to formats, I usually drop some of the examples. Like in the “responding to GET /accounts.xml” I would delete the “should_receive(:find)” example. That’s already covered in the other one, and I know I’m not going to do the find any differently just because it’s XML. On the other hand, I would include the “should be_success” one just to make sure the response is a 200. So I’d probably write the XML example as
```
describe "responding to GET /accounts.xml" do
  before(:each) do
    request.env["HTTP_ACCEPT"] = "application/xml" 
  end

  it "should succeed" do
    Account.stub!(:find).and_return([])
    get :index
    response.should be_success
  end

  it "should render the found accounts as xml" do
    Account.stub(:find).and_return(accounts = mock("Array of Accounts"))
    accounts.should_receive(:to_xml).and_return("generated XML")
    get :index
    response.body.should == "generated XML" 
  end
end
```
I have mixed feelings about omitting an expectation to #save in the “POST to create” spec. It’s easy to read over it and think “does it actually save the object?” I actually like omitting it as you did, but I also draw attention to the save call:
```
describe "responding to POST /accounts" do

  describe "with successful save" do

    def successful_account
      mock_account(:save => true)
    end

    it "should create a new account" do
      Account.should_receive(:new).with({'these' => 'params'}).and_return(successful_account)
      post :create, :account => {:these => 'params'}
    end

    it "should assign the created account for the view" do
      Account.stub!(:new).and_return(successful_account)
      post :create, :account => {}
      assigns(:account).should equal(successful_account)
    end

    it "should redirect to the created account" do
      Account.stub!(:new).and_return(successful_account)
      post :create, :account => {}
      response.should redirect_to(account_url(successful_account))
    end

  end
```
As you can see, I will even do it when the stubbed methods are the same as the defaults. Having that successful_account helper method just makes explicit the fact that we’re dealing with an object that saved successfully. It immediately conveys all the context the reader needs to understand the following examples.

p.s. your blog seems to not let me post my email and URL

David Chelimsky Says:
July 13th, 2008 at 06:06 AM

Thanks for the feedback Pat. As you might imagine, I went through a few iterations of this. At one point I had something like what you have for “with successful save” (with additional valid_account and invalid_account helper methods) and at one point I had this:

describe "with successful save" do

  it "should create a new account" do
    Account.should_receive(:new).with({'these' => 'params'}).and_return(mock_account(:save => true))
    post :create, :account => {:these => 'params'}
  end

  it "should assign the created account for the view" do
    Account.stub!(:new).and_return(mock_account(:save => true))
    post :create, :account => {}
    assigns(:account).should equal(mock_account)
  end

  it "should redirect to the created account" do
    Account.stub!(:new).and_return(mock_account(:save => true))
    post :create, :account => {}
    response.should redirect_to(account_url(mock_account))
  end

end

I think you’re right that duplicating the defaults tells the story better, so I’m inclined to go back to the additional helpers or add the bit of verbosity as in the code in this comment. Which do you (or anyone else) find more readable?

Kerry Buckley Says:
July 13th, 2008 at 06:06 AM
I like the examples, but I have a comment on the ‘describe’ labels.

At some point recently the scaffold generator started producing separate specs for routing, which I like, because routing ought to be independent of the controller. However, the specs still say things like “responding to GET /accounts”, even though they are actually calling the action methods directly. I think there are cases where the routing could be changed, but the examples would still pass, with misleading descriptions.

When writing controller examples from scratch, I tend to use descriptions more like “responding to a GET on an account”, which leave the description of where the request came from to the routing spec.
Mikel Lindsaar Says:
July 13th, 2008 at 06:06 AM
Nice David.

I like the verbosity, personally, I want to be able to read one page of my text editor and be able to understand what the blazes is going on from. If I have to resort to the mouse wheel, then I know I need to re-factor my descriptive text.

But Pat has a point, duplicate specs of the model interaction is a bit redundant and it reads better without. I also do the same thing, if I am spec’ing a javascript output, I only spec that part of the code execution part (the respond to block) knowing that the rest has been covered by previous specifications handling the http request.

The only other thing I would mention is using descriptive names for the helpers….
```
 def given_a_valid_account
   Account.stub!(:new).and_return(mock_account(:save => true))
 end

 it "should assign the created account for the view" do
   given_a_valid_account
   post :create, :account => {}
   assigns(:account).should equal(mock_account)
 end

 etc...
```
I think the biggest advantage of using this quazi-proto-dsl in the method name ‘given_a_valid_account’ is that it really helps explain to the novice user what is going on in the test.

I recently had to bring someone up from zero ruby up to speed on a rails project in two weeks. He is now specing and everything, but being able to show him the specs where I put the given_a_blah type model names, he understood the spec faster and could think with it. it really made a big difference.

The other point is that this code is really used by the beginner developer a lot to try and grok RSpec in general… so if we can make it as clear as possible, we then lower the barrier to entry.

Anyway, my 2c.

Mikel
Dan Manges Says:
July 13th, 2008 at 06:06 AM
Reading over the examples, they didn’t feel to me like the examples would look if they were all done using TDD. In particular, I wouldn’t have examples like the show action renders the show template. That’s the default behavior in Rails – that example would have never been red. I’ll even go as far as to say I think it hurts the examples overall. If I see an example like that, I think there must be something interesting going on in the code that motivated somebody to test an implicit render. I took a quick pass over the examples and left some comments: http://pastie.org/227513

I think the examples that would come out of TDDing the controller would be better examples.
David Chelimsky Says:
July 13th, 2008 at 06:06 AM
Hey Dan – Thanks for your critique. I agree with most of your comments, though there are a couple of additions I feel are necessary to make it all hang together. Please check out the result and let me know what you think.
Dan Manges Says:
July 13th, 2008 at 06:06 AM
Hey David – I like your latest revision much better. IMO, passing the args to mock_account gives the examples more clarity since you can tell that a specific example cares about the mock responding a certain way to certain method calls. I also feel like the proportion of the examples as a whole to the controller is much better. I’m looking forward to seeing what you end up with.
David Chelimsky Says:
July 13th, 2008 at 06:06 AM
OK – here’s another crack, taking @Kerry’s concerns into account (so to speak):

http://pastie.org/227877

And the resulting output:

http://pastie.org/227878
Mikel Lindsaar Says:
July 13th, 2008 at 06:06 AM
Latest revision is nice.

Good work.
Joseph Wilk Says:
July 13th, 2008 at 06:06 AM
I agree with Pat Maddox on the missing of an expectation on save in POST create.

I converted a few of my specs to follow the structure in your latest
revision (which I really like). However I found it somewhat confusing to see:
```
 
def create 
  @image = Image.new params['image'] 
  redirect_to  image_path(@image) 
end
```
Was all that I needed to get the POST create to pass. When In my mind the most important behaviour of POST create is that it attempts to create(save) something.

Can I ask about the rational of leaving it out?
David Chelimsky Says:
July 13th, 2008 at 06:06 AM
@Joseph – I think Pat’s comment was about drawing attention to the save call for readability:
```
mock_model(:save => true)
```
I don’t think it was about setting an expectation for that. Now that you point it out, I’d say it’s missing. I’ll add it.

Thanks!
grosser Says:
August 19th, 2008 at 01:34 AM
```
it "gets index" do
    Account.expects(:find).with(:all).returns [mock_account]
    get :index
    it_renders :index
    it_assigns :accounts, [mock_account]
end
```
Thats 21 not very readable LOCs to 6 dead-simple.

That would be my way to roll(with mocha instead of default mock), kill be_success, it_renders will fail anyway… Use rspec_response_enhancers to get better feedback on request failure and your good to go.(http://pragmatig.wordpress.com/2008/04/20/rspec-responseshould-information-enhancers)

it_renders etc ==> http://pragmatig.wordpress.com/2008/08/17/readable-specs-it-renders-it-assigns-it-flashes
David Chelimsky Says:
August 20th, 2008 at 08:59 AM
@grosser

Agreed that mocha’s syntax is more concise. A year ago I mentioned the possibility of deprecating RSpec’s mock framework and the response was not pretty, and so it lives on.

Agreed that your code example is very readable. The only think I’d recommend doing differently is splitting it into two separate examples, following the one expectation per example rule.

The basic idea being that if something causes this example to fail at it_renders :index, you might think you’re fixing it only to discover that it still fails at it_assigns :accounts, so from a maintainability perspective it’s less headache if you limit each example to only one expectation.
Adam Salter Says:
August 29th, 2008 at 02:59 AM
This isn’t DRY. Read this: http://jamesgolick.com/2007/10/19/introducing-resource_controller-focus-on-what-makes-your-controller-special ... and never write these pointless tests again. (ie just test what you have added yourself).
David Chelimsky Says:
August 29th, 2008 at 06:17 PM
@adam – I’m not sure you understand the context of this post. The examples we’re talking about are those that rspec generates when you choose to use rails’ generators to generate your controllers. Those controllers have a wealth of untested code in them, and leaving them untested introduces a higher level of risk when you start changing them to suit your specific needs.

Using a library like resource_controller is a completely different animal. In that case, these examples would be pointless because the code they are describing isn’t your code. That code is wrapped up in a separate library that is hopefully well tested.

Generated code, however, is your code, and making changes to it bears all the same risks as if you had written it yourself. For that reason, my feeling is that generated code should be accompanied by a set of code examples that both describe how it’s supposed to work, and verifies it as well.

So, by all means, DRY up your application code by extracting common code to a separate object, or use a library like resource_controller. But if you’re going to generate a bunch of code, especially if you’re going to make any changes to it whatsoever, you’re going to be a lot happier making those changes if there’s a thorough set of examples alerting you when your changes break expected behaviour.

Hope that all makes some sense.
grosser Says:
September 18th, 2008 at 01:32 PM
@one example per test

in general yes, but for controllers i do not care. So far a assigns never ever failed me when everything else succeeded, and rendering is most of the time in the same block as the flash, so either both work or none

@this isnt dry

i use a generic test for all actions, except for those that do not fulfill rest conventions

it_should_behave_like ‘resource’

so i always notice when my oh-so-small change did break any default behavior