rspec users - Jan 2009 - New rescue_from handling in rspec-rails 1.1.12

Howdy,

This change to rspec-rails in 1.1.12 tripped me up:
http://github.com/dchelimsky/rspec-rails/commit/ef6d2fc15a
(or, see the relevant Lighthouse ticket, here:
http://rspec.lighthouseapp.com/projects/5645/tickets/85)

Basically, we were depending on that "bug" in 1.1.11, since it seemed
to
make sense to us. Here''s some sample code... I tried to simplify it as
much
as possible:

(Or, here''s a syntax-highlighted pastie: http://pastie.org/361114)


# spec/controllers/application.rb
class AccessDeniedError < StandardError; end
class ApplicationController < ActionController::Base
  rescue_from AccessDeniedError, :with => :handle_access_denied

  protected

  def handle_access_denied(exception)
    redirect_to "/"
  end
end


# app/controllers/posts_controller.rb
class PostsController < ApplicationController
  def destroy
    @post = Post.find(params[:id])
    raise AccessDeniedError unless @post.destroyable_by?(current_user)

    @post.destroy
    redirect_to posts_url
  end
end


# spec/controllers/posts_controller_spec.rb
describe PostsController do
  describe "handling DELETE /posts/2" do
    it "raises AccessDeniedError if the post isn''t destroyable by
the
current user" do
      user = mock_model(User)
      post = mock_model(Post, :destroyable_by? => false)

      controller.stub!(:current_user).and_return(user)
      Post.stub!(:find).and_return(post)

      post.should_not_receive(:destroy)
      lambda { delete :destroy, :id => ''2'' }.should
raise_error(AccessDeniedError)
    end
  end
end


# spec/controllers/appcliation_controller_spec.rb
class ApplicationTestController < ApplicationController
  def access_denied_action
    raise AccessDeniedError
  end
end

describe ApplicationTestController, "#handle_not_found" do
  before do
    controller.use_rails_error_handling!
  end

  it "redirects to the front page" do
    get :access_denied_action
    response.should redirect_to("/")
  end
end


So we''d check that the controller action raises a given error, even
thought
it might be rescued somewhere else (not using
controller.use_rails_error_handling!). And then in a different file,
we''d
specifically check the ApplicationController to make sure that it rescued
properly (*with* controller.use_rails_error_handling!). We figured that
it''s
not PostsController''s responsibility to do the rescue, so it
shouldn''t be
tested there.

This also kept our tests DRY; if we changed the rescue behavior, we
wouldn''t
need to go through all our controller examples and e.g. change the redirect.
It also felt nice and "unit test"-like to me, in that the
implementation of
PostsController clearly raises AccessDeniedError, and we''d check for
that in
the spec. This impedence mismatch bothers me:

implementation: raise AccessDeniedError
specification: response.should redirect_to("/")

I''m guessing that the argument _for_ the new behavior (in which
rescue_from
is always used) is that my PostsController inherits from
ApplicationController, and therefore, inherits its behavior. So when
I''m
testing the behavior, it would violate the POLS if the inherited behavior
was somehow missing. For example, I''d sure be confused if a Rails model
was
missing a feature of ActiveRecord::Base when used under test.

Anyway, what should I really be doing here? I could use shared examples to
maintain DRY (it_should_behave_like :access_denied); or I could just repeat
myself, because that''s the behavior I''m expecting:
response.should
redirect_to("/"). Or, I could alias_method_chain
ActionController::Rescue#rescue_action and hack the old behavior back in. I
don''t really want to do that, though--somebody who was familiar with
RSpec
but hadn''t seen our code before would be mighty confused.

Thanks for reading! That was a lot.

Chris Kampmeier
http://shiftcommathree.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20090114/42f33e6e/attachment.html>

On Wed, Jan 14, 2009 at 8:23 PM, Chris Kampmeier <chris at kampers.net>
wrote:
> Howdy,
>
> This change to rspec-rails in 1.1.12 tripped me up:
> http://github.com/dchelimsky/rspec-rails/commit/ef6d2fc15a
> (or, see the relevant Lighthouse ticket, here:
> http://rspec.lighthouseapp.com/projects/5645/tickets/85)
>
> Basically, we were depending on that "bug" in 1.1.11, since it
seemed to
> make sense to us. Here''s some sample code... I tried to simplify
it as much
> as possible:
>
> (Or, here''s a syntax-highlighted pastie: http://pastie.org/361114)
>
>
> # spec/controllers/application.rb
> class AccessDeniedError < StandardError; end
> class ApplicationController < ActionController::Base
>   rescue_from AccessDeniedError, :with => :handle_access_denied
>
>   protected
>
>   def handle_access_denied(exception)
>     redirect_to "/"
>   end
> end
>
>
> # app/controllers/posts_controller.rb
> class PostsController < ApplicationController
>   def destroy
>     @post = Post.find(params[:id])
>     raise AccessDeniedError unless @post.destroyable_by?(current_user)
>
>     @post.destroy
>     redirect_to posts_url
>   end
> end
>
>
> # spec/controllers/posts_controller_spec.rb
> describe PostsController do
>   describe "handling DELETE /posts/2" do
>     it "raises AccessDeniedError if the post isn''t
destroyable by the
> current user" do
>       user = mock_model(User)
>       post = mock_model(Post, :destroyable_by? => false)
>
>       controller.stub!(:current_user).and_return(user)
>       Post.stub!(:find).and_return(post)
>
>       post.should_not_receive(:destroy)
>       lambda { delete :destroy, :id => ''2'' }.should
> raise_error(AccessDeniedError)
>     end
>   end
> end
>
>
> # spec/controllers/appcliation_controller_spec.rb
> class ApplicationTestController < ApplicationController
>   def access_denied_action
>     raise AccessDeniedError
>   end
> end
>
> describe ApplicationTestController, "#handle_not_found" do
>   before do
>     controller.use_rails_error_handling!
>   end
>
>   it "redirects to the front page" do
>     get :access_denied_action
>     response.should redirect_to("/")
>   end
> end
>
>
> So we''d check that the controller action raises a given error,
even thought
> it might be rescued somewhere else (not using
> controller.use_rails_error_handling!). And then in a different file,
we''d
> specifically check the ApplicationController to make sure that it rescued
> properly (*with* controller.use_rails_error_handling!). We figured that
it''s
> not PostsController''s responsibility to do the rescue, so it
shouldn''t be
> tested there.
>
> This also kept our tests DRY; if we changed the rescue behavior, we
wouldn''t
> need to go through all our controller examples and e.g. change the
redirect.
> It also felt nice and "unit test"-like to me, in that the
implementation of
> PostsController clearly raises AccessDeniedError, and we''d check
for that in
> the spec. This impedence mismatch bothers me:
>
> implementation: raise AccessDeniedError
> specification: response.should redirect_to("/")
Don''t forget that the spec should come first :) Also, you''re
spec''ing
behaviour, not implementation. So for me, what you''ve got is this:

specification: response.should redirect_to("/")
behaviour: response.should redirect_to("/")
> I''m guessing that the argument _for_ the new behavior (in which
rescue_from
> is always used) is that my PostsController inherits from
> ApplicationController, and therefore, inherits its behavior. So when
I''m
> testing the behavior, it would violate the POLS if the inherited behavior
> was somehow missing. For example, I''d sure be confused if a Rails
model was
> missing a feature of ActiveRecord::Base when used under test.
Exactly!
> Anyway, what should I really be doing here? I could use shared examples to
> maintain DRY (it_should_behave_like :access_denied);
Personally, I''ve grown weary of it_should_behave_like. I''d
write a
macro so you could say something like:

describe PostsController do
  context "handling DELETE /posts/2" do
    denies_access_when "the post isn''t destroyable by the current
user" do
      post = mock_model(Post, :destroyable_by? => false)
      Post.stub!(:find).and_return(post)
      post.should_not_receive(:destroy)
    end
  end
end
>or I could just repeat
> myself, because that''s the behavior I''m expecting:
response.should
> redirect_to("/"). Or, I could alias_method_chain
> ActionController::Rescue#rescue_action and hack the old behavior back in. I
> don''t really want to do that, though--somebody who was familiar
with RSpec
> but hadn''t seen our code before would be mighty confused.
A great reason not to do it :)
> Thanks for reading! That was a lot.
Sure - hope these responses are helpful.

Cheers,
David
> Chris Kampmeier
> http://shiftcommathree.com

On Wed, Jan 14, 2009 at 7:30 PM, David Chelimsky <dchelimsky at
gmail.com>wrote:
> On Wed, Jan 14, 2009 at 8:23 PM, Chris Kampmeier <chris at
kampers.net>
> wrote:
>
> > (Or, here''s a syntax-highlighted pastie:
http://pastie.org/361114)
>
> Don''t forget that the spec should come first :) Also,
you''re spec''ing
> behaviour, not implementation. So for me, what you''ve got is this:
>
> specification: response.should redirect_to("/")
> behaviour: response.should redirect_to("/")
>
Thanks, definitely helpful. I think the main thing I needed to get in my
head is that testing Rails controllers with rspec-rails is more about
"behavior," unlike a typical unit-style, in that you''re
really testing the
whole controller stack, and not just an isolated method. It''s a little
more
high-level, since you really need the whole setup (request, response, etc.)
to get anything done.


> > Anyway, what should I really be doing here? I could use shared
examples
> to
> > maintain DRY (it_should_behave_like :access_denied);
>
>
> Personally, I''ve grown weary of it_should_behave_like.
I''d write a
> macro so you could say something like:
>
> describe PostsController do
>  context "handling DELETE /posts/2" do
>    denies_access_when "the post isn''t destroyable by the
current user" do
>       post = mock_model(Post, :destroyable_by? => false)
>       Post.stub!(:find).and_return(post)
>      post.should_not_receive(:destroy)
>     end
>  end
> end

Ooh, that''s interesting. I see how you would implement that, but
I''d never
thought of using a "macro" block helper like that, myself. More
generally,
could you post a bit on why you''ve stopped using shared examples, and
what
it is about the syntax above that resonates with you? Looking at the two
alternatives, it_should_behave_like seems like a more natural approach to
me--it seems very declarative, and clear--but that might just be because
I''m
used to it.

Thanks again!
Chris Kampmeier
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20090115/72909d4d/attachment.html>