Make the test environment show rescuable exceptions in responses#45867
Make the test environment show rescuable exceptions in responses#45867rafaelfranca merged 2 commits intorails:mainfrom jdufresne:show-rescuable-exceptions
Conversation
rails-bot
bot
added
actionmailbox
actionpack
actiontext
activestorage
docs
railties
labels
rafaelfranca
left a comment
rafaelfranca
left a comment
There was a problem hiding this comment.
Feature makes sense to me. this is one of things that annoyed me for too long. Implementation can be improved. commented inline on how.
There was a problem hiding this comment.
Request should not know about the existence of the ExceptionWrapper. Isn't the exception already wrapped when we call this method? I think it can be since we wrap it in invoke_interceptors and also later in render_exception. We should just ask to the exception if it is a rescue_response?. I'd even move this entire implementation out of Request since it belongs more to the ExceptionWrapper than to the request. We should just read the get_header("action_dispatch.show_exceptions") from the request.
There was a problem hiding this comment.
@rafaelfranca Thanks for the review and sorry for the late reply. I have applied this suggestion in the latest revision. Please let me know if it is what you had in mind. All additional feedback is welcome.
|
Thanks @skipkayhil I applied both suggestions in the latest push. |
|
Thanks for all the reviews @zzak! I believe I have addressed all feedback with the latest revision. |
|
Anything I can do to help move this work along? I am motivated and open to additional reviews and feedback. Thanks. |
Background ---------- During integration tests, it is desirable for the application to respond as closely as possible to the way it would in production. This improves confidence that the application behavior acts as it should. In Rails tests, one major mismatch between the test and production environments is that exceptions raised during an HTTP request (e.g. `ActiveRecord::RecordNotFound`) are re-raised within the test rather than rescued and then converted to a 404 response. Setting `config.action_dispatch.show_exceptions` to `true` will make the test environment act like production, however, when an unexpected internal server error occurs, the test will be left with a opaque 500 response rather than presenting a useful stack trace. This makes debugging more difficult. This leaves the developer with choosing between higher quality integration tests or an improved debugging experience on a failure. I propose that we can achieve both. Solution -------- Change the configuration option `config.action_dispatch.show_exceptions` from a boolean to one of 3 values: `:all`, `:rescuable`, `:none`. The values `:all` and `:none` behaves the same as the previous `true` and `false` respectively. What was previously `true` (now `:all`) continues to be the default for non-test environments. The new `:rescuable` value is the new default for the test environment. It will show exceptions in the response only for rescuable exceptions as defined by `ActionDispatch::ExceptionWrapper.rescue_responses`. In the event of an unexpected internal server error, the exception that caused the error will still be raised within the test so as to provide a useful stack trace and a good debugging experience.
|
|
||
| def show?(request) | ||
| # We're treating `nil` as "unset", and we want the default setting to be | ||
| # `:all`. This logic should be extracted to `env_config` and calculated |
There was a problem hiding this comment.
Explore if we can really move this logic to env_config.
There was a problem hiding this comment.
Previous exploration: #46609 (comment)
Ah, I had looked at making it configurable on middleware initialization and not just setting on env_config...
The `request.show_exceptions?` method will be removed in Raiils 7.1 PR: rails/rails#45867 So this commit adopts the new API to check whether to show exception if the Rails version is 7.1 or later.
|
I don't have strong feeling about the change, but I want to share how we can debug such problem, perhaps in a even better way, by utilising the Because I'm a maintainer of (BTW this is how I actually pinned down the problem and found this PR.) This is essentially how the test looks like: it "test something" do
get "/posts"
expect(response).to have_http_status(:internal_server_error)
expect(transport.events.count).to eq(2)
endAnd with this change, it breaks as As described in the PR, it's a request spec so there's not much information when the test failed. What the failure means is that So I added these 2 lines it "test something" do
+ debugger(do: "trace exception")
get "/posts"
+ debugger(do: "trace off")
expect(response).to have_http_status(:internal_server_error)
expect(transport.events.count).to eq(2)
endThey enable the And here's the output:
As you can see, with this feature, we get to see all exceptions raised during the request, which significantly increases its visibility for debugging. I also have revived the Tracer.trace_exception do
get "/posts"
endAnd it will output very similar output:
I think by utilising the |
* Update sentry-rails' show_exception check The `request.show_exceptions?` method will be removed in Raiils 7.1 PR: rails/rails#45867 So this commit adopts the new API to check whether to show exception if the Rails version is 7.1 or later. * Update changelog
* Update 7_1_release_notes.md For context see #45867 Include in release notes the deprecation of `true` and `false` values for `config.action_dispatch.show_exceptions` in favor of `:all`, `:rescuable` and `:none`. * Update 7_1_release_notes.md Co-authored-by: Hartley McGuire <[email protected]> --------- Co-authored-by: Hartley McGuire <[email protected]> Co-authored-by: Rafael Mendonça França <[email protected]>
|
in my Rails 7 I had to change this to :all, because I use a custom error handler using: |
In Rails 7.1, `action_dispatch.show_exceptions` should be set to `:none` instead of `false` per rails/rails#45867. This is similar to doorkeeper-gem/doorkeeper#1742
In Rails 7.1, `action_dispatch.show_exceptions` should be set to `:none` instead of `false` per rails/rails#45867. This is similar to doorkeeper-gem/doorkeeper#1742
6 participants
Background
During integration tests, it is desirable for the application to respond
as closely as possible to the way it would in production. This improves
confidence that the application behavior acts as it should.
In Rails tests, one major mismatch between the test and production
environments is that exceptions raised during an HTTP request (e.g.
ActiveRecord::RecordNotFound) are re-raised within the test ratherthan rescued and then converted to a 404 response.
Setting
config.action_dispatch.show_exceptionstotruewill make thetest environment act like production, however, when an unexpected
internal server error occurs, the test will be left with a opaque 500
response rather than presenting a useful stack trace. This makes
debugging more difficult.
This leaves the developer with choosing between higher quality
integration tests or an improved debugging experience on a failure.
I propose that we can achieve both.
Solution
Change the configuration option
config.action_dispatch.show_exceptionsfrom a boolean to one of 3 values:
:all,:rescuable,:none. Thevalues
:alland:nonebehaves the same as the previoustrueandfalserespectively. What was previouslytrue(now:all) continuesto be the default for non-test environments.
The new
:rescuablevalue is the new default for the test environment.It will show exceptions in the response only for rescuable exceptions as
defined by
ActionDispatch::ExceptionWrapper.rescue_responses. In theevent of an unexpected internal server error, the exception that caused
the error will still be raised within the test so as to provide a useful
stack trace and a good debugging experience.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]