rspec users - Dec 2006 - What do you think of this controller spec?

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!

Pat

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.

I''ll give this a more thorough look over later in the week.

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

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

Thanks for the example controller spec Pat,
I''m just starting to play a bit with RSpec and I''ve got some
hard time
finding my way in the controller specs.
This is just the example I need.

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
>
> Pat
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>

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
>

On Dec 26, 2006, at 12:08 PM, Pat Maddox 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 the code with the incorporated changes:
> spec_helper.rb: http://pastie.caboo.se/29577
> books_controller_spec.rb: http://pastie.caboo.se/29578
>
I''m trying to follow your example, but the include command
isn''t
extending what ever class my context runs in.

On 1/18/07, Bryan Liles <bryan at osesm.com>
wrote:
>
> On Dec 26, 2006, at 12:08 PM, Pat Maddox 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 the code with the incorporated changes:
> > spec_helper.rb: http://pastie.caboo.se/29577
> > books_controller_spec.rb: http://pastie.caboo.se/29578
> >
>
> I''m trying to follow your example, but the include command
isn''t
> extending what ever class my context runs in.
See
http://rubyforge.org/tracker/index.php?func=detail&aid=7461&group_id=797&atid=3151