From 7b5096947e345b8f84c3084e738c8798b754a3d4 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Thu, 28 Oct 2021 12:57:49 +0200 Subject: [PATCH] Call Executor#wrap around each test It's `Rails.application.executor.wrap` that is responsible for clearing request/job local state such as `CurrentAttributes`. Instead of including an ad hoc helper to clear `CurrentAttributes` it's better to run the executor so we properly clear other states as well. However it means all executor hooks now need to be re-entrant. --- actionview/lib/action_view/cache_expiry.rb | 2 +- .../asynchronous_queries_tracker.rb | 3 +++ .../active_record/query_logs/test_helper.rb | 13 ------------ activerecord/lib/active_record/railtie.rb | 5 ----- activerecord/test/cases/query_logs_test.rb | 11 ++++++---- activesupport/CHANGELOG.md | 10 +++++++++- .../lib/active_support/execution_wrapper.rb | 20 ++++++++++++++++++- .../active_support/executor/test_helper.rb | 7 +++++++ activesupport/lib/active_support/railtie.rb | 11 ++++++++-- activesupport/test/current_attributes_test.rb | 12 ++++++++--- .../lib/rails/application/configuration.rb | 1 + .../new_framework_defaults_7_0.rb.tt | 6 ++++++ 12 files changed, 71 insertions(+), 30 deletions(-) delete mode 100644 activerecord/lib/active_record/query_logs/test_helper.rb create mode 100644 activesupport/lib/active_support/executor/test_helper.rb diff --git a/actionview/lib/action_view/cache_expiry.rb b/actionview/lib/action_view/cache_expiry.rb index 1909bc7531f5c..cab052b66bda4 100644 --- a/actionview/lib/action_view/cache_expiry.rb +++ b/actionview/lib/action_view/cache_expiry.rb @@ -4,7 +4,7 @@ module ActionView class CacheExpiry class Executor def initialize(watcher:) - @execution_lock = Concurrent::ReadWriteLock.new + @execution_lock = Concurrent::ReentrantReadWriteLock.new @cache_expiry = ViewModificationWatcher.new(watcher: watcher) do clear_cache end diff --git a/activerecord/lib/active_record/asynchronous_queries_tracker.rb b/activerecord/lib/active_record/asynchronous_queries_tracker.rb index 9b796f78ed80c..d9cd4117e8704 100644 --- a/activerecord/lib/active_record/asynchronous_queries_tracker.rb +++ b/activerecord/lib/active_record/asynchronous_queries_tracker.rb @@ -7,6 +7,9 @@ class << self def active? true end + + def finalize + end end end diff --git a/activerecord/lib/active_record/query_logs/test_helper.rb b/activerecord/lib/active_record/query_logs/test_helper.rb deleted file mode 100644 index 41249aeb84229..0000000000000 --- a/activerecord/lib/active_record/query_logs/test_helper.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module ActiveRecord::QueryLogs::TestHelper # :nodoc: - def before_setup - ActiveRecord::QueryLogs.clear_context - super - end - - def after_teardown - super - ActiveRecord::QueryLogs.clear_context - end -end diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index eabbf15b5ee89..950ac63eb6611 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -380,11 +380,6 @@ class Railtie < Rails::Railtie # :nodoc: app.reloader.before_class_unload { ActiveRecord::QueryLogs.clear_context } app.executor.to_run { ActiveRecord::QueryLogs.clear_context } app.executor.to_complete { ActiveRecord::QueryLogs.clear_context } - - ActiveSupport.on_load(:active_support_test_case) do - require "active_record/query_logs/test_helper" - include ActiveRecord::QueryLogs::TestHelper - end end end end diff --git a/activerecord/test/cases/query_logs_test.rb b/activerecord/test/cases/query_logs_test.rb index 8765d60b72c76..cc34f3a39b1c7 100644 --- a/activerecord/test/cases/query_logs_test.rb +++ b/activerecord/test/cases/query_logs_test.rb @@ -2,12 +2,8 @@ require "cases/helper" require "models/dashboard" -require "active_record/query_logs/test_helper" class QueryLogsTest < ActiveRecord::TestCase - # Automatically included in Rails apps via railtie but that don't run here. - include ActiveRecord::QueryLogs::TestHelper - fixtures :dashboards ActiveRecord::QueryLogs.taggings[:application] = -> { @@ -15,6 +11,10 @@ class QueryLogsTest < ActiveRecord::TestCase } def setup + # QueryLogs context is automatically reset in Rails app via an executor hooks set in railtie + # But not in Active Record's own test suite. + ActiveRecord::QueryLogs.clear_context + # Enable the query tags logging @original_transformers = ActiveRecord.query_transformers @original_prepend = ActiveRecord::QueryLogs.prepend_comment @@ -28,6 +28,9 @@ def teardown ActiveRecord.query_transformers = @original_transformers ActiveRecord::QueryLogs.prepend_comment = @original_prepend ActiveRecord::QueryLogs.tags = [] + # QueryLogs context is automatically reset in Rails app via an executor hooks set in railtie + # But not in Active Record's own test suite. + ActiveRecord::QueryLogs.clear_context end def test_escaping_good_comment diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 4c32bc867176a..771b46285b670 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,12 @@ +* `Rails.application.executor` hooks are now called around every tests. + + This helps to better simulate request or job local state being reset around tests and prevent state + to leak from one test to another. + + However it requires the executor hooks executed in the test environment to be re-entrant. + + *Jean Boussier* + * `ActiveSupport::DescendantsTracker` now mostly delegate to `Class#descendants` on Ruby 3.1 Ruby now provides a fast `Class#descendants` making `ActiveSupport::DescendantsTracker` mostly useless. @@ -6,7 +15,6 @@ - `ActiveSupport::DescendantsTracker.direct_descendants` - `ActiveSupport::DescendantsTracker#direct_descendants` - *Jean Boussier* * Fix the `Digest::UUID.uuid_from_hash` behavior for namespace IDs that are different from the ones defined on `Digest::UUID`. diff --git a/activesupport/lib/active_support/execution_wrapper.rb b/activesupport/lib/active_support/execution_wrapper.rb index ca810db5842b7..36e80b23c7ce8 100644 --- a/activesupport/lib/active_support/execution_wrapper.rb +++ b/activesupport/lib/active_support/execution_wrapper.rb @@ -91,6 +91,16 @@ def self.wrap end end + def self.perform # :nodoc: + instance = new + instance.run + begin + yield + ensure + instance.complete + end + end + class << self # :nodoc: attr_accessor :active end @@ -108,6 +118,10 @@ def self.active? # :nodoc: def run! # :nodoc: self.class.active[Thread.current] = true + run + end + + def run # :nodoc: run_callbacks(:run) end @@ -116,11 +130,15 @@ def run! # :nodoc: # # Where possible, prefer +wrap+. def complete! - run_callbacks(:complete) + complete ensure self.class.active.delete Thread.current end + def complete # :nodoc: + run_callbacks(:complete) + end + private def hook_state @_hook_state ||= {} diff --git a/activesupport/lib/active_support/executor/test_helper.rb b/activesupport/lib/active_support/executor/test_helper.rb new file mode 100644 index 0000000000000..97f489dc2e31f --- /dev/null +++ b/activesupport/lib/active_support/executor/test_helper.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module ActiveSupport::Executor::TestHelper # :nodoc: + def run(...) + Rails.application.executor.perform { super } + end +end diff --git a/activesupport/lib/active_support/railtie.rb b/activesupport/lib/active_support/railtie.rb index 76ff7cd28e89e..d2405bb2c2773 100644 --- a/activesupport/lib/active_support/railtie.rb +++ b/activesupport/lib/active_support/railtie.rb @@ -28,13 +28,20 @@ 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) + app.reloader.before_class_unload { ActiveSupport::CurrentAttributes.clear_all } app.executor.to_run { ActiveSupport::CurrentAttributes.reset_all } app.executor.to_complete { ActiveSupport::CurrentAttributes.reset_all } ActiveSupport.on_load(:active_support_test_case) do - require "active_support/current_attributes/test_helper" - include ActiveSupport::CurrentAttributes::TestHelper + if executor_around_test_case + require "active_support/executor/test_helper" + include ActiveSupport::Executor::TestHelper + else + require "active_support/current_attributes/test_helper" + include ActiveSupport::CurrentAttributes::TestHelper + end end end diff --git a/activesupport/test/current_attributes_test.rb b/activesupport/test/current_attributes_test.rb index 0196331d13cb6..088c041d5e0dd 100644 --- a/activesupport/test/current_attributes_test.rb +++ b/activesupport/test/current_attributes_test.rb @@ -1,11 +1,17 @@ # frozen_string_literal: true require_relative "abstract_unit" -require "active_support/current_attributes/test_helper" class CurrentAttributesTest < ActiveSupport::TestCase - # Automatically included in Rails apps via railtie but that don't run here. - include ActiveSupport::CurrentAttributes::TestHelper + # CurrentAttributes is automatically reset in Rails app via executor hooks set in railtie + # But not in Active Support's own test suite. + setup do + ActiveSupport::CurrentAttributes.reset_all + end + + teardown do + ActiveSupport::CurrentAttributes.reset_all + end Person = Struct.new(:id, :name, :time_zone) diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index 4d4ac85516d6e..a832ba253055a 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -214,6 +214,7 @@ def load_defaults(target_version) active_support.remove_deprecated_time_with_zone_name = true active_support.cache_format_version = 7.0 active_support.use_rfc4122_namespaced_uuids = true + active_support.executor_around_test_case = true end if respond_to?(:action_mailer) diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt index 65c452f37ad68..c54fe5c82b600 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt +++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt @@ -45,6 +45,12 @@ # and you have no plans to rollback. # Rails.application.config.active_support.cache_format_version = 7.0 +# Calls `Rails.application.executor.wrap` around test cases. +# This makes test cases behave closer to an actual request or job, several +# features that are normally disabled in test, such as Active Record query cache +# and asynchronous queries will then be enabled. +# Rails.application.config.active_support.executor_around_test_case = true + # Set both the `:open_timeout` and `:read_timeout` values for `:smtp` delivery method. # Rails.application.config.action_mailer.smtp_timeout = 5