On Wed, Jan 14, 2009 at 8:23 PM, Chris Kampmeier <[email protected]> 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
_______________________________________________
rspec-users mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users
