[rspec-users] What do you think of this controller spec?

David Chelimsky dchelimsky at gmail.com
Wed Dec 27 11:21:00 EST 2006


On 12/26/06, Pat Maddox <pergesu at gmail.com> wrote:
> On 12/25/06, David Chelimsky <dchelimsky at gmail.com> wrote:
> > On 12/24/06, Pat Maddox <pergesu at gmail.com> wrote:
> > > Here's a controller spec I wrote, it's for a very simple RESTful
> > > controller (well, aren't all RESTful controllers simple? :)
> > >
> > > I've created a couple spec helper methods to refactor some of the
> > > common code...for example, require_login_and_correct_user creates two
> > > specifications: one for when the user isn't logged in, and one when
> > > the user is logged in but requests someone else's book.  do_request
> > > automatically calls do_get/post/put/delete depending on what's
> > > implemented.
> > >
> > > Here's all the code:
> > >
> > > books_controller.rb: http://pastie.caboo.se/29469
> > > books_controller_spec.rb: http://pastie.caboo.se/29470
> > > spec_helper.rb: http://pastie.caboo.se/29471
> > >
> > > The specs just feel somewhat unwieldy to me...maybe it's because the
> > > controller code is very simple and elegant, and the spec looks big and
> > > bloated in comparison.  Also, it feels like it's taking me a lot
> > > longer to write the controller specs with all these individual
> > > verifications.
> > >
> > > Am I just tripping for no reason, and the specs look fine?  What would
> > > you change?  Also I know that the controller code could be improved by
> > > using the finder on the association, this is just how I have it for
> > > now.  I'm interested in how the specs could be better.
> > >
> > > I'll be understanding if I don't get a response for a few days :)
> > > Merry Christmas everybody!
> >
> > Quick observations:
> >
> > 1. At first glance I like the structure of wrapping specify blocks in
> > methods like this.
> >
> > 2. Instead of having free floating methods I'd wrap them in a module
> > and include the module in the context:
> >
> > module MyHelperMethods
> >   def login(user, pw)
> >     ...
> >   end
> > end
> >
> > context "Given blah...." do
> >   include MyHelperMethods
> >   setup {..}
> >   specify "a logged in user..." do
> >     login(@user, @pw)
> >     ..
> >   end
> > end
> >
> > 3. The extractions are inconsistent. For example:
> >
> > setup do
> >   create_user
> >   login_as @mock_user
> > end
> >
> > The reference to @mock_user is confusing here. Where does it come from?
> >
> > To make it easier to understand, I might phrase this like this:
> >
> > setup do
> >   @mock_user = create_user
> >   login_as @mock_user
> > end
> >
> > or, more terse:
> >
> > module MyHelpers
> >   def mock_user
> >     @mock_user ||= mock("user")
> >   end
> >   def login_as(user)
> >     ..
> >   end
> > end
> >
> > context ...
> >   setup do
> >     login_as mock_user
> >   end
> > end
> >
> > Using a mock_user method like that could make the specs look more
> > consistent (i.e. not sometimes referencing @mock_user and sometimes
> > referencing mock_user).
> >
> > 4. The magic involved in having should_find_user call do_get is a bit
> > troubling for me. I appreciate the need to indicate what to call, but
> > I'd do it more explicitly:
> >
> > should_find_user_on :get
> >
> > ... or something like that. Then you can eliminate do_request and the
> > spec becomes a bit more explicit.
> >
> > ====================
> > Pat, this is great stuff in general. The challenge is to eliminate
> > duplication in such a way that each spec is still completely
> > understandable without having to go read the other helpers, etc. I
> > hope my suggestions here help a bit.
>
> Hey David,
>
> Thanks a lot for the suggestions.  I agree with all of them, I think
> they make the code clearer.  Things are more explicit and consistent.
>
> Here's the code with the incorporated changes:
> spec_helper.rb: http://pastie.caboo.se/29577
> books_controller_spec.rb: http://pastie.caboo.se/29578

Cool Pat. I have a couple more observations. These are admittedly
REALLY nit-picky, so take them w/ a grain of salt. I'm just sharing
how I would approach it from here.

1. There is an imbalance between @mock_book (instance var) and
mock_user (method). I'd add a mock_book method like the mock_user
method to make the setups feel more consistent. For example:

setup do
  login_as mock_user
  mock_book.stub!(:mark_as_read)
end

2. I'd call the modules something like BookSpecs and UserSpecs. I
think that describes better what they represent (specs that later get
applied in different contexts).

3. I'm almost tempted to take ALL of the specs and define them in the
modules. Then the contexts read like this:

context "Given a call to :update using PUT, the BooksController" do
  include UserSpecHelpers
  extend BookHelperMethods
  controller_name :books
  setup do
    login_as mock_user
    mock_book.stub!(:mark_as_read)
    mock_book.stub!(:to_param).and_return("1")
  end

  def do_put(options = {})
    put :update, options.merge({ :user_id => "1", :id => "1" })
  end

  should_find_user_on :put
  should_find_book_on :put
  should_not_update_the_book_as_read_if_not_specified
  should_update_the_book_as_read_if_specified
  should_redirect_to_the_book_just_updated
end

I have some mixed feelings about that because it pushes more towards
the sort of indirection confusion that I so want to avoid in specs. On
the other hand, it makes the specs read very consistently.

WDYT?

David

>
> Pat
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>


More information about the rspec-users mailing list