Merge pull request rails#43734 from ghiculescu/ac-testcase-executor

Wrap `ActionController::TestCase` with Rails executor

Author: byroot

actionpack/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   `Rails.application.executor` hooks can now be called around every request in a `ActionController::TestCase`
+
+    This helps to better simulate request or job local state being reset between requests and prevent state
+    leaking from one request to another.
+
+    To enable this, set `config.active_support.executor_around_test_case = true` (this is the default in Rails 7).
+
+    *Alex Ghiculescu*
+
 *   Consider onion services secure for cookies.
 
     *Justin Tracey*

actionpack/lib/action_controller/railtie.rb

@@ -122,5 +122,11 @@ class Railtie < Rails::Railtie # :nodoc:
         end
       end
     end
+
+    initializer "action_controller.test_case" do |app|
+      ActiveSupport.on_load(:action_controller_test_case) do
+        ActionController::TestCase.executor_around_each_request = app.config.active_support.executor_around_test_case
+      end
+    end
   end
 end

actionpack/lib/action_controller/test_case.rb

@@ -333,6 +333,8 @@ def load!
   #
   #  assert_redirected_to page_url(title: 'foo')
   class TestCase < ActiveSupport::TestCase
+    class_attribute :executor_around_each_request, default: false
+
     module Behavior
       extend ActiveSupport::Concern
       include ActionDispatch::TestProcess
@@ -578,10 +580,19 @@ def setup_request(controller_class_name, action, parameters, session, flash, xhr
           end
         end
 
+        def wrap_execution(&block)
+          if executor_around_each_request && defined?(Rails.application) && Rails.application
+            Rails.application.executor.wrap(&block)
+          else
+            yield
+          end
+        end
+
         def process_controller_response(action, cookies, xhr)
           begin
             @controller.recycle!
-            @controller.dispatch(action, @request, @response)
+
+            wrap_execution { @controller.dispatch(action, @request, @response) }
           ensure
             @request = @controller.request
             @response = @controller.response

activesupport/CHANGELOG.md

@@ -28,13 +28,15 @@
 
     *Sean Doyle*
 
-*   `Rails.application.executor` hooks are now called around every tests.
+*   `Rails.application.executor` hooks can now be called around every test
 
-    This helps to better simulate request or job local state being reset around tests and prevent state
-    to leak from one test to another.
+    This helps to better simulate request or job local state being reset around tests and prevents state
+    leaking from one test to another.
 
     However it requires the executor hooks executed in the test environment to be re-entrant.
 
+    To enable this, set `config.active_support.executor_around_test_case = true` (this is the default in Rails 7).
+
     *Jean Boussier*
 
 *   `ActiveSupport::DescendantsTracker` now mostly delegate to `Class#descendants` on Ruby 3.1

activesupport/lib/active_support/railtie.rb

@@ -40,7 +40,7 @@ class Railtie < Rails::Railtie # :nodoc:
     end
 
     initializer "active_support.reset_all_current_attributes_instances" do |app|
-      executor_around_test_case = app.config.active_support.delete(:executor_around_test_case)
+      executor_around_test_case = app.config.active_support.executor_around_test_case
 
       app.reloader.before_class_unload { ActiveSupport::CurrentAttributes.clear_all }
       app.executor.to_run              { ActiveSupport::CurrentAttributes.reset_all }

railties/test/application/action_controller_test_case_integration_test.rb

@@ -0,0 +1,110 @@
+# frozen_string_literal: true
+
+require "isolation/abstract_unit"
+require "rack/test"
+
+class SharedSetup < ActionController::TestCase
+  class_attribute :executor_around_each_request
+
+  include ActiveSupport::Testing::Isolation
+
+  setup do
+    build_app
+
+    app_file "app/models/current.rb", <<-RUBY
+      class Current < ActiveSupport::CurrentAttributes
+        attribute :customer
+
+        resets { Time.zone = "UTC" }
+
+        def customer=(customer)
+          super
+          Time.zone = customer&.time_zone
+        end
+      end
+    RUBY
+
+    app_file "app/models/customer.rb", <<-RUBY
+      class Customer < Struct.new(:name)
+        def time_zone
+          "Copenhagen"
+        end
+      end
+    RUBY
+
+    remove_from_config '.*config\.load_defaults.*\n'
+    add_to_config "config.active_support.executor_around_test_case = #{self.class.executor_around_each_request}"
+
+    app_file "app/controllers/customers_controller.rb", <<-RUBY
+      class CustomersController < ApplicationController
+        layout false
+
+        def get_current_customer
+          render :index
+        end
+
+        def set_current_customer
+          Current.customer = Customer.new("david")
+          render :index
+        end
+      end
+    RUBY
+
+    app_file "app/views/customers/index.html.erb", <<-RUBY
+      <%= Current.customer&.name || 'noone' %>,<%= Time.zone.name %>
+    RUBY
+
+    require "#{app_path}/config/environment"
+
+    @controller = CustomersController.new
+    @routes = ActionDispatch::Routing::RouteSet.new.tap do |r|
+      r.draw do
+        get "/customers/:action", controller: :customers
+      end
+    end
+  end
+
+  teardown :teardown_app
+end
+
+class ActionControllerTestCaseWithExecutorIntegrationTest < SharedSetup
+  self.executor_around_each_request = true
+
+  test "current customer is cleared after each request" do
+    assert Rails.application.config.active_support.executor_around_test_case
+    assert ActionController::TestCase.executor_around_each_request
+
+    get :get_current_customer
+    assert_response :ok
+    assert_match(/noone,UTC/, response.body)
+
+    get :set_current_customer
+    assert_response :ok
+    assert_match(/david,Copenhagen/, response.body)
+
+    get :get_current_customer
+    assert_response :ok
+    assert_match(/noone,UTC/, response.body)
+  end
+end
+
+class ActionControllerTestCaseWithoutExecutorIntegrationTest < SharedSetup
+  self.executor_around_each_request = false
+
+  test "current customer is not cleared after each request" do
+    assert_not Rails.application.config.active_support.executor_around_test_case
+    assert_not ActionController::TestCase.executor_around_each_request
+
+    get :get_current_customer
+    assert_response :ok
+    assert_match(/noone,UTC/, response.body)
+
+    get :set_current_customer
+    assert_response :ok
+    assert_match(/david,Copenhagen/, response.body)
+
+    get :get_current_customer
+    assert_response :ok
+    assert_match(/david,Copenhagen/, response.body)
+  end
+end