Make the test environment show rescuable exceptions in responses

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.

Author: jdufresne

actionmailbox/test/dummy/config/environments/test.rb

@@ -29,7 +29,7 @@
   config.cache_store = :null_store
 
   # Raise exceptions instead of rendering exception templates.
-  config.action_dispatch.show_exceptions = false
+  config.action_dispatch.show_exceptions = :rescuable
 
   # Disable request forgery protection in test environment.
   config.action_controller.allow_forgery_protection = false

actionpack/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Change `action_dispatch.show_exceptions` to one of `:all`, `:rescuable`, or
+    `:none`. `:all` and `:none` behave the same as the previous `true` and
+    `false` respectively. The new `:rescuable` option will only show exceptions
+    that can be rescued (e.g. `ActiveRecord::RecordNotFound`). `:rescuable` is
+    now the default for the test environment.
+
+    *Jon Dufresne*
+
 *   `config.action_dispatch.cookies_serializer` now accepts `:message_pack` and
     `:message_pack_allow_marshal` as serializers. These serializers require the
     [`msgpack` gem](https://rubygems.org/gems/msgpack) (>= 1.7.0).

actionpack/lib/action_dispatch/http/request.rb

@@ -191,13 +191,6 @@ def http_auth_salt
       get_header "action_dispatch.http_auth_salt"
     end
 
-    def show_exceptions? # :nodoc:
-      # We're treating `nil` as "unset", and we want the default setting to be
-      # `true`. This logic should be extracted to `env_config` and calculated
-      # once.
-      !(get_header("action_dispatch.show_exceptions") == false)
-    end
-
     # Returns a symbol form of the #request_method.
     def request_method_symbol
       HTTP_METHOD_LOOKUP[request_method]

actionpack/lib/action_dispatch/middleware/debug_exceptions.rb

@@ -40,7 +40,7 @@ def call(env)
       wrapper = ExceptionWrapper.new(backtrace_cleaner, exception)
 
       invoke_interceptors(request, exception, wrapper)
-      raise exception unless request.show_exceptions?
+      raise exception unless wrapper.show?(request)
       render_exception(request, exception, wrapper)
     end
 

actionpack/lib/action_dispatch/middleware/exception_wrapper.rb

@@ -174,6 +174,29 @@ def self.status_code_for_exception(class_name)
       Rack::Utils.status_code(@@rescue_responses[class_name])
     end
 
+    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
+      # once.
+      config = request.get_header("action_dispatch.show_exceptions")
+
+      # Include true and false for backwards compatibility.
+      case config
+      when :none
+        false
+      when :rescuable
+        rescue_response?
+      when true
+        ActionDispatch.deprecator.warn("Setting action_dispatch.show_exceptions to true is deprecated. Set to :all instead.")
+        true
+      when false
+        ActionDispatch.deprecator.warn("Setting action_dispatch.show_exceptions to false is deprecated. Set to :none instead.")
+        false
+      else
+        true
+      end
+    end
+
     def rescue_response?
       @@rescue_responses.key?(exception.class.name)
     end

actionpack/lib/action_dispatch/middleware/show_exceptions.rb

@@ -30,18 +30,18 @@ def call(env)
       @app.call(env)
     rescue Exception => exception
       request = ActionDispatch::Request.new env
-      if request.show_exceptions?
-        render_exception(request, exception)
+      backtrace_cleaner = request.get_header("action_dispatch.backtrace_cleaner")
+      wrapper = ExceptionWrapper.new(backtrace_cleaner, exception)
+      if wrapper.show?(request)
+        render_exception(request, wrapper)
       else
         raise exception
       end
     end
 
     private
-      def render_exception(request, exception)
-        backtrace_cleaner = request.get_header "action_dispatch.backtrace_cleaner"
-        wrapper = ExceptionWrapper.new(backtrace_cleaner, exception)
-        status  = wrapper.status_code
+      def render_exception(request, wrapper)
+        status = wrapper.status_code
         request.set_header "action_dispatch.exception", wrapper.unwrapped_exception
         request.set_header "action_dispatch.original_path", request.path_info
         request.set_header "action_dispatch.original_request_method", request.raw_request_method

actionpack/lib/action_dispatch/railtie.rb

@@ -9,7 +9,7 @@ class Railtie < Rails::Railtie # :nodoc:
     config.action_dispatch = ActiveSupport::OrderedOptions.new
     config.action_dispatch.x_sendfile_header = nil
     config.action_dispatch.ip_spoofing_check = true
-    config.action_dispatch.show_exceptions = true
+    config.action_dispatch.show_exceptions = :all
     config.action_dispatch.tld_length = 1
     config.action_dispatch.ignore_accept_header = false
     config.action_dispatch.rescue_templates = {}

actionpack/test/controller/new_base/render_action_test.rb

@@ -89,7 +89,7 @@ def setup
 
     test "rendering with layout => true" do
       assert_raise(ArgumentError) do
-        get "/render_action/basic/hello_world_with_layout", headers: { "action_dispatch.show_exceptions" => false }
+        get "/render_action/basic/hello_world_with_layout", headers: { "action_dispatch.show_exceptions" => :none }
       end
     end
 
@@ -109,7 +109,7 @@ def setup
 
     test "rendering with layout => 'greetings'" do
       assert_raise(ActionView::MissingTemplate) do
-        get "/render_action/basic/hello_world_with_custom_layout", headers: { "action_dispatch.show_exceptions" => false }
+        get "/render_action/basic/hello_world_with_custom_layout", headers: { "action_dispatch.show_exceptions" => :none }
       end
     end
   end

actionpack/test/controller/new_base/render_test.rb

@@ -79,7 +79,7 @@ class RenderTest < Rack::TestCase
         end
 
         assert_raises(AbstractController::DoubleRenderError) do
-          get "/render/double_render", headers: { "action_dispatch.show_exceptions" => false }
+          get "/render/double_render", headers: { "action_dispatch.show_exceptions" => :none }
         end
       end
     end
@@ -89,13 +89,13 @@ class TestOnlyRenderPublicActions < Rack::TestCase
     # Only public methods on actual controllers are callable actions
     test "raises an exception when a method of Object is called" do
       assert_raises(AbstractController::ActionNotFound) do
-        get "/render/blank_render/clone", headers: { "action_dispatch.show_exceptions" => false }
+        get "/render/blank_render/clone", headers: { "action_dispatch.show_exceptions" => :none }
       end
     end
 
     test "raises an exception when a private method is called" do
       assert_raises(AbstractController::ActionNotFound) do
-        get "/render/blank_render/secretz", headers: { "action_dispatch.show_exceptions" => false }
+        get "/render/blank_render/secretz", headers: { "action_dispatch.show_exceptions" => :none }
       end
     end
   end

actionpack/test/dispatch/content_security_policy_test.rb

@@ -331,7 +331,7 @@ def call(env)
       env["action_dispatch.content_security_policy"] = POLICY
       env["action_dispatch.content_security_policy_nonce_generator"] = proc { "iyhD0Yc0W+c=" }
       env["action_dispatch.content_security_policy_report_only"] = false
-      env["action_dispatch.show_exceptions"] = false
+      env["action_dispatch.show_exceptions"] = :none
 
       @app.call(env)
     end
@@ -481,7 +481,7 @@ def call(env)
       env["action_dispatch.content_security_policy"] = POLICY
       env["action_dispatch.content_security_policy_nonce_generator"] = proc { "iyhD0Yc0W+c=" }
       env["action_dispatch.content_security_policy_report_only"] = false
-      env["action_dispatch.show_exceptions"] = false
+      env["action_dispatch.show_exceptions"] = :none
 
       @app.call(env)
     end
@@ -597,7 +597,7 @@ def call(env)
       env["action_dispatch.content_security_policy"] = nil
       env["action_dispatch.content_security_policy_nonce_generator"] = nil
       env["action_dispatch.content_security_policy_report_only"] = false
-      env["action_dispatch.show_exceptions"] = false
+      env["action_dispatch.show_exceptions"] = :none
 
       @app.call(env)
     end
@@ -653,7 +653,7 @@ def call(env)
       env["action_dispatch.content_security_policy_nonce_generator"] = proc { "iyhD0Yc0W+c=" }
       env["action_dispatch.content_security_policy_report_only"] = false
       env["action_dispatch.content_security_policy_nonce_directives"] = %w(script-src)
-      env["action_dispatch.show_exceptions"] = false
+      env["action_dispatch.show_exceptions"] = :none
 
       @app.call(env)
     end
@@ -725,7 +725,7 @@ def call(env)
       env["action_dispatch.content_security_policy"] = POLICY
       env["action_dispatch.content_security_policy_nonce_generator"] = proc { "iyhD0Yc0W+c=" }
       env["action_dispatch.content_security_policy_report_only"] = false
-      env["action_dispatch.show_exceptions"] = false
+      env["action_dispatch.show_exceptions"] = :none
 
       @app.call(env)
     end