From 0e72a0537a1b083e75ae0fe694a0a6cf90e46267 Mon Sep 17 00:00:00 2001 From: Javier Julio Date: Sat, 6 Apr 2024 10:31:18 -0400 Subject: [PATCH 1/8] Reset CurrentAttributes on each rails example This needs to be included otherwise RSpec does not reset ActiveSupport::CurrentAttributes around each example. We need to mimic what Rails does in: https://github.com/rails/rails/blob/v7.0.0/activesupport/lib/active_support/railtie.rb#L50-L61 for the "else" case until #2712 can be revisited. --- lib/rspec/rails/example/rails_example_group.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/rspec/rails/example/rails_example_group.rb b/lib/rspec/rails/example/rails_example_group.rb index 0643b9ae1..d469f1148 100644 --- a/lib/rspec/rails/example/rails_example_group.rb +++ b/lib/rspec/rails/example/rails_example_group.rb @@ -3,6 +3,7 @@ require 'rspec/rails/matchers' if ::Rails::VERSION::MAJOR >= 7 + require "active_support/current_attributes/test_helper" require 'active_support/execution_context/test_helper' end @@ -18,6 +19,7 @@ module RailsExampleGroup include RSpec::Rails::FixtureSupport if ::Rails::VERSION::MAJOR >= 7 include RSpec::Rails::TaggedLoggingAdapter + include ActiveSupport::CurrentAttributes::TestHelper include ActiveSupport::ExecutionContext::TestHelper end end From d829a02c586bef0a138a954a9d17a51ed8cc0445 Mon Sep 17 00:00:00 2001 From: Javier Julio Date: Sat, 6 Apr 2024 10:56:24 -0400 Subject: [PATCH 2/8] Match formatting for require statement Copied this from Rails source but I forgot to update before committing originally. --- lib/rspec/rails/example/rails_example_group.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rspec/rails/example/rails_example_group.rb b/lib/rspec/rails/example/rails_example_group.rb index d469f1148..ed343de35 100644 --- a/lib/rspec/rails/example/rails_example_group.rb +++ b/lib/rspec/rails/example/rails_example_group.rb @@ -3,7 +3,7 @@ require 'rspec/rails/matchers' if ::Rails::VERSION::MAJOR >= 7 - require "active_support/current_attributes/test_helper" + require 'active_support/current_attributes/test_helper' require 'active_support/execution_context/test_helper' end From 91167a02a557e35f8a41a1702d82d8ec1751b5ea Mon Sep 17 00:00:00 2001 From: Javier Julio Date: Tue, 9 Apr 2024 10:56:12 -0400 Subject: [PATCH 3/8] Add spec --- .../rails/example/rails_example_group_spec.rb | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/spec/rspec/rails/example/rails_example_group_spec.rb b/spec/rspec/rails/example/rails_example_group_spec.rb index 7faac1646..90eab2ab1 100644 --- a/spec/rspec/rails/example/rails_example_group_spec.rb +++ b/spec/rspec/rails/example/rails_example_group_spec.rb @@ -32,5 +32,40 @@ module RSpec::Rails expect(results).to all be true end + + describe 'CurrentAttributes' do + let(:current_class) do + Class.new(ActiveSupport::CurrentAttributes) do + attribute :request_id + end + end + + it 'does not leak current attributes between example groups', if: ::Rails::VERSION::MAJOR >= 7 do + groups = + [ + RSpec::Core::ExampleGroup.describe("A group") do + include RSpec::Rails::RailsExampleGroup + specify { expect(current_class.attributes).to eq({}) } + end, + RSpec::Core::ExampleGroup.describe("A controller group", type: :controller) do + specify do + current_class.request_id = "123" + expect(current_class.attributes).to eq(request_id: "123") + end + end, + RSpec::Core::ExampleGroup.describe("Another group") do + include RSpec::Rails::RailsExampleGroup + specify { expect(current_class.attributes).to eq({}) } + end + ] + + results = + groups.map do |group| + group.run(failure_reporter) ? true : failure_reporter.exceptions + end + + expect(results).to all be true + end + end end end From 9e35e007928feaabf70d5471370bad66bbc7289c Mon Sep 17 00:00:00 2001 From: Javier Julio Date: Tue, 9 Apr 2024 11:33:57 -0400 Subject: [PATCH 4/8] Add spec --- .../rails/example/rails_example_group_spec.rb | 42 ++++++------------- 1 file changed, 12 insertions(+), 30 deletions(-) diff --git a/spec/rspec/rails/example/rails_example_group_spec.rb b/spec/rspec/rails/example/rails_example_group_spec.rb index 90eab2ab1..8c31785c6 100644 --- a/spec/rspec/rails/example/rails_example_group_spec.rb +++ b/spec/rspec/rails/example/rails_example_group_spec.rb @@ -33,38 +33,20 @@ module RSpec::Rails expect(results).to all be true end - describe 'CurrentAttributes' do - let(:current_class) do - Class.new(ActiveSupport::CurrentAttributes) do - attribute :request_id - end + describe 'CurrentAttributes', order: :defined do + class Current < ActiveSupport::CurrentAttributes + attribute :request_id end - it 'does not leak current attributes between example groups', if: ::Rails::VERSION::MAJOR >= 7 do - groups = - [ - RSpec::Core::ExampleGroup.describe("A group") do - include RSpec::Rails::RailsExampleGroup - specify { expect(current_class.attributes).to eq({}) } - end, - RSpec::Core::ExampleGroup.describe("A controller group", type: :controller) do - specify do - current_class.request_id = "123" - expect(current_class.attributes).to eq(request_id: "123") - end - end, - RSpec::Core::ExampleGroup.describe("Another group") do - include RSpec::Rails::RailsExampleGroup - specify { expect(current_class.attributes).to eq({}) } - end - ] - - results = - groups.map do |group| - group.run(failure_reporter) ? true : failure_reporter.exceptions - end - - expect(results).to all be true + it 'sets a current attribute' do + Current.request_id = '123' + expect(Current.request_id).to eq('123') + expect(Current.attributes).to eq(request_id: '123') + end + + it 'does not leak current attributes' do + expect(Current.request_id).to eq(nil) + expect(Current.attributes).to eq({}) end end end From 808fef7b4e4fd3f2758b57d40b2e3129f6dcbe02 Mon Sep 17 00:00:00 2001 From: Javier Julio Date: Tue, 9 Apr 2024 11:40:12 -0400 Subject: [PATCH 5/8] Include RailsExampleGroup in spec Now that we've confirmed the failing test, by including RailsExampleGroup, it should reset CurrentAttributes as expected. --- spec/rspec/rails/example/rails_example_group_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/rspec/rails/example/rails_example_group_spec.rb b/spec/rspec/rails/example/rails_example_group_spec.rb index 8c31785c6..78b205937 100644 --- a/spec/rspec/rails/example/rails_example_group_spec.rb +++ b/spec/rspec/rails/example/rails_example_group_spec.rb @@ -34,6 +34,8 @@ module RSpec::Rails end describe 'CurrentAttributes', order: :defined do + include RSpec::Rails::RailsExampleGroup + class Current < ActiveSupport::CurrentAttributes attribute :request_id end From b27a5b9cc199cb8ab48036ffcfeb36515dc1442a Mon Sep 17 00:00:00 2001 From: Javier Julio Date: Tue, 9 Apr 2024 11:53:14 -0400 Subject: [PATCH 6/8] Address spec failures The Rubocop failure I was unsure about but it seems from other specs here that a class can be defined in the top level block so we'll try that. The spec now only runs in Rails 7+ and only asserts against the defined attribute as that should suffice. --- .../rails/example/rails_example_group_spec.rb | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/spec/rspec/rails/example/rails_example_group_spec.rb b/spec/rspec/rails/example/rails_example_group_spec.rb index 78b205937..b1647f12c 100644 --- a/spec/rspec/rails/example/rails_example_group_spec.rb +++ b/spec/rspec/rails/example/rails_example_group_spec.rb @@ -1,6 +1,10 @@ module RSpec::Rails - RSpec.describe RailsExampleGroup do + RSpec.describe RailsExampleGroup do if ::Rails::VERSION::MAJOR >= 7 + class CurrentSample < ActiveSupport::CurrentAttributes + attribute :request_id + end + it 'supports tagged_logger' do expect(described_class.private_instance_methods).to include(:tagged_logger) end @@ -33,22 +37,16 @@ module RSpec::Rails expect(results).to all be true end - describe 'CurrentAttributes', order: :defined do + describe 'CurrentAttributes', order: :defined, if: ::Rails::VERSION::MAJOR >= 7 do include RSpec::Rails::RailsExampleGroup - class Current < ActiveSupport::CurrentAttributes - attribute :request_id - end - it 'sets a current attribute' do - Current.request_id = '123' - expect(Current.request_id).to eq('123') - expect(Current.attributes).to eq(request_id: '123') + CurrentSample.request_id = '123' + expect(CurrentSample.request_id).to eq('123') end it 'does not leak current attributes' do - expect(Current.request_id).to eq(nil) - expect(Current.attributes).to eq({}) + expect(CurrentSample.request_id).to eq(nil) end end end From d1a4c0d9f38fc97ccdbf1c046ab99de4f105badb Mon Sep 17 00:00:00 2001 From: Javier Julio Date: Tue, 9 Apr 2024 11:56:18 -0400 Subject: [PATCH 7/8] Remove trailing spaces CI reported on line 2. --- spec/rspec/rails/example/rails_example_group_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/rspec/rails/example/rails_example_group_spec.rb b/spec/rspec/rails/example/rails_example_group_spec.rb index b1647f12c..17c3fa380 100644 --- a/spec/rspec/rails/example/rails_example_group_spec.rb +++ b/spec/rspec/rails/example/rails_example_group_spec.rb @@ -1,5 +1,5 @@ module RSpec::Rails - RSpec.describe RailsExampleGroup do + RSpec.describe RailsExampleGroup do if ::Rails::VERSION::MAJOR >= 7 class CurrentSample < ActiveSupport::CurrentAttributes attribute :request_id From 301345972b9700488e187bc6414e67487434030f Mon Sep 17 00:00:00 2001 From: Jon Rowe Date: Wed, 10 Apr 2024 08:54:58 +0100 Subject: [PATCH 8/8] Refactor spec for current attributes --- .../rails/example/rails_example_group_spec.rb | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/spec/rspec/rails/example/rails_example_group_spec.rb b/spec/rspec/rails/example/rails_example_group_spec.rb index 17c3fa380..7dbe13788 100644 --- a/spec/rspec/rails/example/rails_example_group_spec.rb +++ b/spec/rspec/rails/example/rails_example_group_spec.rb @@ -1,13 +1,7 @@ module RSpec::Rails RSpec.describe RailsExampleGroup do - if ::Rails::VERSION::MAJOR >= 7 - class CurrentSample < ActiveSupport::CurrentAttributes - attribute :request_id - end - - it 'supports tagged_logger' do - expect(described_class.private_instance_methods).to include(:tagged_logger) - end + it 'supports tagged_logger', if: ::Rails::VERSION::MAJOR >= 7 do + expect(described_class.private_instance_methods).to include(:tagged_logger) end it 'does not leak context between example groups', if: ::Rails::VERSION::MAJOR >= 7 do @@ -37,17 +31,30 @@ class CurrentSample < ActiveSupport::CurrentAttributes expect(results).to all be true end - describe 'CurrentAttributes', order: :defined, if: ::Rails::VERSION::MAJOR >= 7 do - include RSpec::Rails::RailsExampleGroup + it 'will not leak ActiveSupport::CurrentAttributes between examples', if: ::Rails::VERSION::MAJOR >= 7 do + group = + RSpec::Core::ExampleGroup.describe("A group", order: :defined) do + include RSpec::Rails::RailsExampleGroup - it 'sets a current attribute' do - CurrentSample.request_id = '123' - expect(CurrentSample.request_id).to eq('123') - end + # rubocop:disable Lint/ConstantDefinitionInBlock + class CurrentSample < ActiveSupport::CurrentAttributes + attribute :request_id + end + # rubocop:enable Lint/ConstantDefinitionInBlock + + it 'sets a current attribute' do + CurrentSample.request_id = '123' + expect(CurrentSample.request_id).to eq('123') + end + + it 'does not leak current attributes' do + expect(CurrentSample.request_id).to eq(nil) + end + end - it 'does not leak current attributes' do - expect(CurrentSample.request_id).to eq(nil) - end + expect( + group.run(failure_reporter) ? true : failure_reporter.exceptions + ).to be true end end end