[cucumber] lib/cucumber/rails/world.rb redefining ActionController::Base#rescue_action

[Matt Patterson-4]

Evening.

I've got a question about the redefinition of  
ActionController::Base#rescue_action. Mainly, why? From the comment in  
the source it looks there was a pain point that this solved, but I  
can't figure out what that might be.

My problem is this: I make heavy use of rescue_from to trap  
exceptional cases so I can 404, 401 or 500 as appropriate. I've got a  
Scenario which looks like this ('deleted' blog posts in this app  
aren't erased from the DB, they're marked deleted for auditing):

   Scenario: Attempting to visit a deleted blog post
     Given there is a blog post entitled "Look Ma no hands"
     And the blog post "Look Ma no hands" has been marked as deleted
     When I attempt to visit the Blog post page for "Look Ma no hands"
     Then the response should be a 404 error

That looks like a perfectly reasonable scenario to me, except that it  
blows up at "When I attempt to visit". The source for that step is:

When /^I attempt to visit the Blog post page for "(.+)"$/ do |title|
   visit entry_path(Entry.find_by_url_slug!
(Digest::MD5.hexdigest(title)))
end

[Excuse the MD5 - it's just a way of maintaining readability in the  
scenarios without resorting to fixtures (I'm undecided about it as an  
approach, btw)]

visit is exploding when my controller raises a FourOhFour error  
because a non-staff non-logged-in user shouldn't be able to see the  
page. I use rescue_from to render a special 404 template, but that's  
never reached because of the redefinition of rescue_action.

This seems really counterintuitive to me: I expect that the  
integration tests will use the full stack, and even in my isolated  
controller specs I have to explicitly request that the normal  
rescue_action is bypassed. If there's a genuine pain point here that  
re-raising and not handling the exception solves then I'm sure I can  
find a different idiom for these kinds of scenarios, but I've spent 4  
hours today trying to figure out what was broken: first in my app code  
then breaking out rdebug to find out what was really happening, which  
suggests to me that it's a non-obvious and confusing intervention.

Anyway, sorry to sound so whingey. Like I said: if there's a genuine  
problem this approach solves, I'll find a new idiom and shut up.  
Otherwise, can this be changed?

Thanks,

Matt

--
   Matt Patterson | Design & Code
   <matt at reprocessed org> | http://www.reprocessed.org/



_______________________________________________
rspec-devel mailing list
rspec-devel@...
http://rubyforge.org/mailman/listinfo/rspec-devel

[Aslak Hellesoy]

On Tue, Mar 24, 2009 at 10:46 PM, Matt Patterson <matt-lists@...> wrote:

Evening.

I've got a question about the redefinition of ActionController::Base#rescue_action. Mainly, why? From the comment in the source it looks there was a pain point that this solved, but I can't figure out what that might be.

My problem is this: I make heavy use of rescue_from to trap exceptional cases so I can 404, 401 or 500 as appropriate. I've got a Scenario which looks like this ('deleted' blog posts in this app aren't erased from the DB, they're marked deleted for auditing):

 Scenario: Attempting to visit a deleted blog post
   Given there is a blog post entitled "Look Ma no hands"
   And the blog post "Look Ma no hands" has been marked as deleted
   When I attempt to visit the Blog post page for "Look Ma no hands"
   Then the response should be a 404 error

That looks like a perfectly reasonable scenario to me, except that it blows up at "When I attempt to visit". The source for that step is:

When /^I attempt to visit the Blog post page for "(.+)"$/ do |title|
 visit entry_path(Entry.find_by_url_slug!(Digest::MD5.hexdigest(title)))
end

[Excuse the MD5 - it's just a way of maintaining readability in the scenarios without resorting to fixtures (I'm undecided about it as an approach, btw)]

visit is exploding when my controller raises a FourOhFour error because a non-staff non-logged-in user shouldn't be able to see the page. I use rescue_from to render a special 404 template, but that's never reached because of the redefinition of rescue_action.

This seems really counterintuitive to me: I expect that the integration tests will use the full stack, and even in my isolated controller specs I have to explicitly request that the normal rescue_action is bypassed. If there's a genuine pain point here that re-raising and not handling the exception solves then I'm sure I can find a different idiom for these kinds of scenarios, but I've spent 4 hours today trying to figure out what was broken: first in my app code then breaking out rdebug to find out what was really happening, which suggests to me that it's a non-obvious and confusing intervention.

Anyway, sorry to sound so whingey. Like I said: if there's a genuine problem this approach solves, I'll find a new idiom and shut up. Otherwise, can this be changed?

I understand your situation - it's perfectly valid. Still, I think you are the first person to bring this up. The vast majority of Cucumber users/Rails developers don't rely on this kind of error handling, so raising errors seems to be a sensible default.

There is currently no way to turn off that default. Can you provide a patch (or a suggestion) that will let users optionally turn off the current behaviour?

Aslak
 

Thanks,

Matt

--
 Matt Patterson | Design & Code
 <matt at reprocessed org> | http://www.reprocessed.org/



_______________________________________________
rspec-devel mailing list
rspec-devel@...
http://rubyforge.org/mailman/listinfo/rspec-devel

_______________________________________________
rspec-devel mailing list
rspec-devel@...
http://rubyforge.org/mailman/listinfo/rspec-devel

[Zach Dennis-2]

2009/3/24 aslak hellesoy <aslak.hellesoy@...>:

>
>
> On Tue, Mar 24, 2009 at 10:46 PM, Matt Patterson
> <matt-lists@...> wrote:
>>
>> Evening.
>>
>> I've got a question about the redefinition of
>> ActionController::Base#rescue_action. Mainly, why? From the comment in the
>> source it looks there was a pain point that this solved, but I can't figure
>> out what that might be.
>>
>> My problem is this: I make heavy use of rescue_from to trap exceptional
>> cases so I can 404, 401 or 500 as appropriate. I've got a Scenario which
>> looks like this ('deleted' blog posts in this app aren't erased from the DB,
>> they're marked deleted for auditing):
>>
>>  Scenario: Attempting to visit a deleted blog post
>>    Given there is a blog post entitled "Look Ma no hands"
>>    And the blog post "Look Ma no hands" has been marked as deleted
>>    When I attempt to visit the Blog post page for "Look Ma no hands"
>>    Then the response should be a 404 error
>>
>> That looks like a perfectly reasonable scenario to me, except that it
>> blows up at "When I attempt to visit". The source for that step is:
>>
>> When /^I attempt to visit the Blog post page for "(.+)"$/ do |title|
>>  visit entry_path(Entry.find_by_url_slug!(Digest::MD5.hexdigest(title)))
>> end
>>
>> [Excuse the MD5 - it's just a way of maintaining readability in the
>> scenarios without resorting to fixtures (I'm undecided about it as an
>> approach, btw)]
>>
>> visit is exploding when my controller raises a FourOhFour error because a
>> non-staff non-logged-in user shouldn't be able to see the page. I use
>> rescue_from to render a special 404 template, but that's never reached
>> because of the redefinition of rescue_action.
>>
>> This seems really counterintuitive to me: I expect that the integration
>> tests will use the full stack, and even in my isolated controller specs I
>> have to explicitly request that the normal rescue_action is bypassed. If
>> there's a genuine pain point here that re-raising and not handling the
>> exception solves then I'm sure I can find a different idiom for these kinds
>> of scenarios, but I've spent 4 hours today trying to figure out what was
>> broken: first in my app code then breaking out rdebug to find out what was
>> really happening, which suggests to me that it's a non-obvious and confusing
>> intervention.
>>
>> Anyway, sorry to sound so whingey. Like I said: if there's a genuine
>> problem this approach solves, I'll find a new idiom and shut up. Otherwise,
>> can this be changed?

I usually comment those things out in rails/world.rb in cucumber.

>
> I understand your situation - it's perfectly valid. Still, I think you are
> the first person to bring this up.

Nope, he's not:

 http://www.ruby-forum.com/topic/177887

> The vast majority of Cucumber users/Rails
> developers don't rely on this kind of error handling, so raising errors
> seems to be a sensible default.

I'm not the vast, but I rely on, and I know several other Rails devs
who rely on it to. Although, we may still be the minority.

>
> There is currently no way to turn off that default. Can you provide a patch
> (or a suggestion) that will let users optionally turn off the current
> behaviour?

A discussion was originally brought up here:

 http://rspec.lighthouseapp.com/projects/16211/tickets/49-cucumber-and-rails-controller-error-handling

I apologize for not being proactive on my end, so far taking out the
offending lines on rails/world.rb has worked fine.

>
> Aslak
>
>>
>> Thanks,
>>
>> Matt
>>
>> --
>>  Matt Patterson | Design & Code
>>  <matt at reprocessed org> | http://www.reprocessed.org/
>>
>>
>>
>> _______________________________________________
>> rspec-devel mailing list
>> rspec-devel@...
>> http://rubyforge.org/mailman/listinfo/rspec-devel
>
>
> _______________________________________________
> rspec-devel mailing list
> rspec-devel@...
> http://rubyforge.org/mailman/listinfo/rspec-devel
>

--
Zach Dennis
http://www.continuousthinking.com
http://www.mutuallyhuman.com
_______________________________________________
rspec-devel mailing list
rspec-devel@...
http://rubyforge.org/mailman/listinfo/rspec-devel

[Matt Patterson-4]

On 24 Mar 2009, at 23:04, Zach Dennis wrote:

> 2009/3/24 aslak hellesoy <aslak.hellesoy@...>:
>>
>> On Tue, Mar 24, 2009 at 10:46 PM, Matt Patterson
>> <matt-lists@...> wrote:
>>>
>>> Anyway, sorry to sound so whingey. Like I said: if there's a genuine
>>> problem this approach solves, I'll find a new idiom and shut up.  
>>> Otherwise,
>>> can this be changed?
>
> I usually comment those things out in rails/world.rb in cucumber.

That's what I wound up doing

>> There is currently no way to turn off that default. Can you provide  
>> a patch
>> (or a suggestion) that will let users optionally turn off the current
>> behaviour?
>
> A discussion was originally brought up here:
>
> http://rspec.lighthouseapp.com/projects/16211/tickets/49-cucumber-and-rails-controller-error-handling
>
> I apologize for not being proactive on my end, so far taking out the
> offending lines on rails/world.rb has worked fine.

Interesting - I think it should definitely be configurable from  
Cucumber, rather than reuse the RSpec config - I want rescue_action  
bypassed in RSpec and honoured in Cucumber...

I've had a bit of a poke around and think I can do something along the  
lines of Cucumber::Rails.use_transactional_fixtures

I'll submit a patch to that Lighthouse ticket in the morning.

Matt


--
   Matt Patterson | Design & Code
   <matt at reprocessed org> | http://www.reprocessed.org/



_______________________________________________
rspec-devel mailing list
rspec-devel@...
http://rubyforge.org/mailman/listinfo/rspec-devel

[Aslak Hellesoy]

Den 25. mars. 2009 kl. 00.04 skrev Zach Dennis <zach.dennis@...>:

> 2009/3/24 aslak hellesoy <aslak.hellesoy@...>:
>>
>>
>> On Tue, Mar 24, 2009 at 10:46 PM, Matt Patterson
>> <matt-lists@...> wrote:
>>>
>>> Evening.
>>>
>>> I've got a question about the redefinition of
>>> ActionController::Base#rescue_action. Mainly, why? From the  
>>> comment in the
>>> source it looks there was a pain point that this solved, but I  
>>> can't figure
>>> out what that might be.
>>>
>>> My problem is this: I make heavy use of rescue_from to trap  
>>> exceptional
>>> cases so I can 404, 401 or 500 as appropriate. I've got a Scenario  
>>> which
>>> looks like this ('deleted' blog posts in this app aren't erased  
>>> from the DB,
>>> they're marked deleted for auditing):
>>>
>>>  Scenario: Attempting to visit a deleted blog post
>>>    Given there is a blog post entitled "Look Ma no hands"
>>>    And the blog post "Look Ma no hands" has been marked as deleted
>>>    When I attempt to visit the Blog post page for "Look Ma no hands"
>>>    Then the response should be a 404 error
>>>
>>> That looks like a perfectly reasonable scenario to me, except that  
>>> it
>>> blows up at "When I attempt to visit". The source for that step is:
>>>
>>> When /^I attempt to visit the Blog post page for "(.+)"$/ do |title|
>>>  visit entry_path(Entry.find_by_url_slug!
>>> (Digest::MD5.hexdigest(title)))
>>> end
>>>
>>> [Excuse the MD5 - it's just a way of maintaining readability in the
>>> scenarios without resorting to fixtures (I'm undecided about it as  
>>> an
>>> approach, btw)]
>>>
>>> visit is exploding when my controller raises a FourOhFour error  
>>> because a
>>> non-staff non-logged-in user shouldn't be able to see the page. I  
>>> use
>>> rescue_from to render a special 404 template, but that's never  
>>> reached
>>> because of the redefinition of rescue_action.
>>>
>>> This seems really counterintuitive to me: I expect that the  
>>> integration
>>> tests will use the full stack, and even in my isolated controller  
>>> specs I
>>> have to explicitly request that the normal rescue_action is  
>>> bypassed. If
>>> there's a genuine pain point here that re-raising and not handling  
>>> the
>>> exception solves then I'm sure I can find a different idiom for  
>>> these kinds
>>> of scenarios, but I've spent 4 hours today trying to figure out  
>>> what was
>>> broken: first in my app code then breaking out rdebug to find out  
>>> what was
>>> really happening, which suggests to me that it's a non-obvious and  
>>> confusing
>>> intervention.
>>>
>>> Anyway, sorry to sound so whingey. Like I said: if there's a genuine
>>> problem this approach solves, I'll find a new idiom and shut up.  
>>> Otherwise,
>>> can this be changed?
>
> I usually comment those things out in rails/world.rb in cucumber.
>
>>
>> I understand your situation - it's perfectly valid. Still, I think  
>> you are
>> the first person to bring this up.
>
> Nope, he's not:
>
> http://www.ruby-forum.com/topic/177887

Doh. Forgot.

>
>
>> The vast majority of Cucumber users/Rails
>> developers don't rely on this kind of error handling, so raising  
>> errors
>> seems to be a sensible default.
>
> I'm not the vast, but I rely on, and I know several other Rails devs
> who rely on it to. Although, we may still be the minority.
>
>>
>> There is currently no way to turn off that default. Can you provide  
>> a patch
>> (or a suggestion) that will let users optionally turn off the current
>> behaviour?
>
> A discussion was originally brought up here:
>
> http://rspec.lighthouseapp.com/projects/16211/tickets/49-cucumber-and-rails-controller-error-handling
>
> I apologize for not being proactive on my end, so far taking out the
> offending lines on rails/world.rb has worked fine.
>
>>
>> Aslak
>>
>>>
>>> Thanks,
>>>
>>> Matt
>>>
>>> --
>>>  Matt Patterson | Design & Code
>>>  <matt at reprocessed org> | http://www.reprocessed.org/
>>>
>>>
>>>
>>> _______________________________________________
>>> rspec-devel mailing list
>>> rspec-devel@...
>>> http://rubyforge.org/mailman/listinfo/rspec-devel
>>
>>
>> _______________________________________________
>> rspec-devel mailing list
>> rspec-devel@...
>> http://rubyforge.org/mailman/listinfo/rspec-devel
>>
>
>
>
> --
> Zach Dennis
> http://www.continuousthinking.com
> http://www.mutuallyhuman.com
> _______________________________________________
> rspec-devel mailing list
> rspec-devel@...
> http://rubyforge.org/mailman/listinfo/rspec-devel

_______________________________________________
rspec-devel mailing list
rspec-devel@...
http://rubyforge.org/mailman/listinfo/rspec-devel

[Zach Dennis-2]

On Tue, Mar 24, 2009 at 8:11 PM, Matt Patterson
<matt-lists@...> wrote:

> On 24 Mar 2009, at 23:04, Zach Dennis wrote:
>
>> 2009/3/24 aslak hellesoy <aslak.hellesoy@...>:
>>>
>>> On Tue, Mar 24, 2009 at 10:46 PM, Matt Patterson
>>> <matt-lists@...> wrote:
>>>>
>>>> Anyway, sorry to sound so whingey. Like I said: if there's a genuine
>>>> problem this approach solves, I'll find a new idiom and shut up.
>>>> Otherwise,
>>>> can this be changed?
>>
>> I usually comment those things out in rails/world.rb in cucumber.
>
> That's what I wound up doing
>
>>> There is currently no way to turn off that default. Can you provide a
>>> patch
>>> (or a suggestion) that will let users optionally turn off the current
>>> behaviour?
>>
>> A discussion was originally brought up here:
>>
>>
>> http://rspec.lighthouseapp.com/projects/16211/tickets/49-cucumber-and-rails-controller-error-handling
>>
>> I apologize for not being proactive on my end, so far taking out the
>> offending lines on rails/world.rb has worked fine.
>
>
> Interesting - I think it should definitely be configurable from Cucumber,
> rather than reuse the RSpec config - I want rescue_action bypassed in RSpec
> and honoured in Cucumber...
>
> I've had a bit of a poke around and think I can do something along the lines
> of Cucumber::Rails.use_transactional_fixtures
>
> I'll submit a patch to that Lighthouse ticket in the morning.
>

I don't know if you have already, but be sure to read through the
related RSpec ticket:

   http://rspec.lighthouseapp.com/projects/5645/tickets/85-11818-have-mode-for-rails-error-handling

The discussion on that ticket evolved over time and has some good
thoughts that ultimately ended up in RSpec. I think some of those same
considerations would do a Cucumber patch good.


--
Zach Dennis
http://www.continuousthinking.com
http://www.mutuallyhuman.com
_______________________________________________
rspec-devel mailing list
rspec-devel@...
http://rubyforge.org/mailman/listinfo/rspec-devel

[Matt Patterson-4]

On 25 Mar 2009, at 01:03, Zach Dennis wrote:

>>
>> I've had a bit of a poke around and think I can do something along  
>> the lines
>> of Cucumber::Rails.use_transactional_fixtures
>>
>> I'll submit a patch to that Lighthouse ticket in the morning.
>
> I don't know if you have already, but be sure to read through the
> related RSpec ticket:
>
>   http://rspec.lighthouseapp.com/projects/5645/tickets/85-11818-have-mode-for-rails-error-handling
>
> The discussion on that ticket evolved over time and has some good
> thoughts that ultimately ended up in RSpec. I think some of those same
> considerations would do a Cucumber patch good.

I hadn't seen the ticket, but I had looked at bypass_rescue in the  
source. I've looked over the ticket now and I think that what I'm  
proposing is broadly in line with it.

Matt


--
   Matt Patterson | Design & Code
   <matt at reprocessed org> | http://www.reprocessed.org/



_______________________________________________
rspec-devel mailing list
rspec-devel@...
http://rubyforge.org/mailman/listinfo/rspec-devel

[Matt Patterson-4]

On 25 Mar 2009, at 08:04, Matt Patterson wrote:

> I hadn't seen the ticket, but I had looked at bypass_rescue in the  
> source. I've looked over the ticket now and I think that what I'm  
> proposing is broadly in line with it.


I've got a patched Cucumber fork which moves the rescue-related  
redefinitions to a method (Cucumber::Rails.bypass_rescue), and updated  
the ticket here: http://rspec.lighthouseapp.com/projects/16211-cucumber/tickets/49-cucumber-and-rails-controller-error-handling

Matt


--
   Matt Patterson | Design & Code
   <matt at reprocessed org> | http://www.reprocessed.org/



_______________________________________________
rspec-devel mailing list
rspec-devel@...
http://rubyforge.org/mailman/listinfo/rspec-devel