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

Pat Maddox pergesu at gmail.com
Tue Dec 26 12:08:05 EST 2006


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

Pat


More information about the rspec-users mailing list