Make the test environment show rescuable exceptions in responses

[jdufresne]

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.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[rafaelfranca]

Feature makes sense to me. this is one of things that annoyed me for too long. Implementation can be improved. commented inline on how.

[rafaelfranca]

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.

[jdufresne]

@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.

[jdufresne]

Thanks @skipkayhil I applied both suggestions in the latest push.

[rafaelfranca]

Explore if we can really move this logic to env_config.

[skipkayhil]

Previous exploration: #46609 (comment)

Ah, I had looked at making it configurable on middleware initialization and not just setting on env_config...

[st0012]

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 debug gem.

Because I'm a maintainer of sentry-ruby and this change actually broke sentry-rails due to the removal of show_exception?, I'll use the broken request test as an example.

(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)
    end

And with this change, it breaks as

 1) Sentry::Rails::Tracing with traces_sample_rate set records transaction with exception
     Failure/Error: expect(transport.events.count).to eq(2)
     
       expected: 2
            got: 0
     
       (compared using ==)
     # ./spec/sentry/rails/tracing_spec.rb:27:in `block (3 levels) in <top (required)>'
     # /Users/hung-wulo/.gem/ruby/3.2.2/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:124:in `block in run'
     # /Users/hung-wulo/.gem/ruby/3.2.2/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:110:in `loop'
     # /Users/hung-wulo/.gem/ruby/3.2.2/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:110:in `run'
     # /Users/hung-wulo/.gem/ruby/3.2.2/gems/rspec-retry-0.6.2/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # /Users/hung-wulo/.gem/ruby/3.2.2/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:37:in `block (2 levels) in setup'

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 sentry-rails stopped capturing exception for some reason. I suspected it's because a broken API inside the SDK.

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)
    end

They enable the debug gem's ExceptionTracer before sending the request, and then disable it afterward.

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 ruby/tracer gem based on the debug gem's tracers. And with it, we can do it with:

Tracer.trace_exception do
  get "/posts"
end

And it will output very similar output:

I think by utilising the tracer gem in development/testing environment, we may be able to improve developers' debugging experience even more.

[maricavor]

in my Rails 7 I had to change this to :all, because I use a custom error handler using:
config.exceptions_app = lambda do |env| ExceptionsController.action(:handle_error).call(env) end
But now when I run rspec request test i do not get even WebMock::NetConnectNotAllowedError anymore?