new controller examples

July 1st, 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

Posted by David Filed in Uncategorized

16 Responses to “new controller examples”

  1. Pat Maddox Says:
    July 1st, 2008 at 12:02 am

    I like it. A couple comments:

    <p>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.

    <p>I really like the ordering you&#8217;ve used though.  It does seem to tell a

    story to me:

    <p>&#8220;What should the accounts index do?&#8221;

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

    <p>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 =&gt; true)
end

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

it "should assign the created account for the view" do
  Account.stub!(:new).and_return(successful_account)
  post :create, :account =&gt; {}
  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 =&gt; {}
  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>p.s. your blog seems to not let me post my email and <span class="caps">URL</span></p>
  2. David Chelimsky Says:
    July 1st, 2008 at 12:02 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?

  3. Kerry Buckley Says:
    July 1st, 2008 at 12:02 am

    I like the examples, but I have a comment on the ‘describe’ labels.

    <p>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.

    <p>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.

  4. Mikel Lindsaar Says:
    July 1st, 2008 at 12:02 am

    Nice David.

    <p>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.

    <p>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.

    <p>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.

    <p>I recently had to bring someone up from <em>zero</em> 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.

    <p>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.

    <p>Anyway, my 2c.</p>


<p><a href="http://lindsaar.net/">Mikel</a></p>
  5. Dan Manges Says:
    July 1st, 2008 at 12:02 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

    <p>I think the examples that would come out of TDDing the controller would be better examples.</p>
  6. David Chelimsky Says:
    July 1st, 2008 at 12:02 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.

  7. Dan Manges Says:
    July 1st, 2008 at 12:02 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.

  8. David Chelimsky Says:
    July 1st, 2008 at 12:02 am

    OK – here’s another crack, taking @Kerry’s concerns into account (so to speak):

    <p>http://pastie.org/227877</p>


<p>And the resulting output:</p>


<p>http://pastie.org/227878</p>
  9. Mikel Lindsaar Says:
    July 1st, 2008 at 12:02 am

    Latest revision is nice.

    <p>Good work.</p>
  10. Joseph Wilk Says:
    July 1st, 2008 at 12:02 am

    I agree with Pat Maddox on the missing of an expectation on save in POST create.

    <p>I converted a few of my specs to follow the structure in your latest</p>

    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.

    <p>Can I ask about the rational of leaving it out?</p>
  11. David Chelimsky Says:
    July 1st, 2008 at 12:02 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.

    <p>Thanks!</p>
  12. grosser Says:
    July 1st, 2008 at 12:02 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.

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


<p>it_renders etc ==&gt; <a href="http://pragmatig.wordpress.com/2008/08/17/readable-specs-it-renders-it-assigns-it-flashes">http://pragmatig.wordpress.com/2008/08/17/readable-specs-it-renders-it-assigns-it-flashes</a></p>
  13. David Chelimsky Says:
    July 1st, 2008 at 12:02 am

    @grosser

    <p>Agreed that mocha&#8217;s syntax is more concise. A year ago I mentioned the possibility of deprecating RSpec&#8217;s mock framework and the response was not pretty, and so it lives on.</p>


<p>Agreed that your code example is very readable. The only think I&#8217;d recommend doing differently is splitting it into two separate examples, following the <a href="http://techblog.daveastels.com/?s=expectation">one expectation per example</a> rule.</p>


<p>The basic idea being that if something causes this example to fail at <code>it_renders :index</code>, you might <strong>think</strong> you&#8217;re fixing it only to discover that it still fails at <code>it_assigns :accounts</code>, so from a maintainability perspective it&#8217;s less headache if you limit each example to only one expectation.</p>
  14. Adam Salter Says:
    July 1st, 2008 at 12:02 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).

  15. David Chelimsky Says:
    July 1st, 2008 at 12:02 am

    @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.

    <p>Using a library like resource_controller is a completely different animal. In that case, these examples <strong>would</strong> be pointless because the code they are describing isn&#8217;t <strong>your</strong> code. That code is wrapped up in a separate library that is hopefully well tested.</p>


<p>Generated code, however, <strong>is</strong> 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&#8217;s supposed to work, and verifies it as well.</p>


<p>So, by all means, <span class="caps">DRY</span> up your application code by extracting common code to a separate object, or use a library like resource_controller. But if you&#8217;re going to generate a bunch of code, especially if you&#8217;re going to make any changes to it whatsoever, you&#8217;re going to be a lot happier making those changes if there&#8217;s a thorough set of examples alerting you when your changes break expected behaviour.</p>


<p>Hope that all makes some sense.</p>
  16. grosser Says:
    July 1st, 2008 at 12:02 am

    @one example per test

    <p>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</p>


<p>@this isnt dry</p>


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


<p>it_should_behave_like &#8216;resource&#8217;</p>


<p>so i always notice when my oh-so-small change did break any default behavior</p>