-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework reset behaviour of ActiveSupport::CurrentAttributes #2774
Closed
pond
wants to merge
10
commits into
rspec:main
from
RIPAGlobal:feature/reset-current-attributes-improvement
Closed
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
1b3ac4b
Compromise solution to #2773
pond f3707d4
Remove verbose comments in favour of PR reference
pond 7b364d2
Probably shouldn't leave that debug 'puts' in there!
pond beda807
Address RuboCop warnings (it made some good calls, tho I think parens…
pond c35bb77
Use 'in_sub_process' to properly manage global state mutation
pond 8d32e03
Update spec/rspec/rails/example/rails_example_group_spec.rb
pond 5be2227
Update spec/rspec/rails/example/rails_example_group_spec.rb
pond f8e09f5
Update spec/rspec/rails/example/rails_example_group_spec.rb
pond cc2a7f3
Update spec/rspec/rails/example/rails_example_group_spec.rb
pond 388c0a9
Figured out test issues & fixed
pond File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,3 +1,5 @@ | ||||||
require 'rspec/support/spec/in_sub_process' | ||||||
|
||||||
module RSpec::Rails | ||||||
RSpec.describe RailsExampleGroup do | ||||||
it 'supports tagged_logger', if: ::Rails::VERSION::MAJOR >= 7 do | ||||||
|
@@ -56,5 +58,81 @@ class CurrentSample < ActiveSupport::CurrentAttributes | |||||
group.run(failure_reporter) ? true : failure_reporter.exceptions | ||||||
).to be true | ||||||
end | ||||||
|
||||||
context 'with suite-level around-example hooks configured', if: ::Rails::VERSION::MAJOR >= 7 do | ||||||
let(:uniquely_identifiable_metadata) do | ||||||
{ configured_around_example_hook: true } | ||||||
end | ||||||
|
||||||
# rubocop:disable Lint/ConstantDefinitionInBlock | ||||||
class CurrentAttrsBetweenHooks < ActiveSupport::CurrentAttributes | ||||||
attribute :request_id | ||||||
end | ||||||
# rubocop:enable Lint/ConstantDefinitionInBlock | ||||||
|
||||||
# This dirties global state, so tests *MUST* remember to use | ||||||
pirj marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
# "in_sub_process". | ||||||
# | ||||||
before :each do | ||||||
pond marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
# Client code might legitimately want to wrap examples to ensure | ||||||
# all-conditions tidy-up, e.g. "ActsAsTenant.without_tenant do...", | ||||||
# wherein an "around" hook is the only available solution, often used | ||||||
# in the overall suite via "config.around". Tests would not expect | ||||||
# anything set in CurrentAttributes here to suddenly be reset by the | ||||||
# time their actual tests, or their test hooks ran. | ||||||
# | ||||||
RSpec.configure do | config | | ||||||
config.around(:each, uniquely_identifiable_metadata) do | example | | ||||||
pond marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
CurrentAttrsBetweenHooks.request_id = '123' | ||||||
example.run | ||||||
end | ||||||
end | ||||||
end | ||||||
|
||||||
it 'does not reset ActiveSupport::CurrentAttributes before examples' do | ||||||
pond marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
in_sub_process do | ||||||
pond marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
group = | ||||||
RSpec::Core::ExampleGroup.describe('A group', uniquely_identifiable_metadata) do | ||||||
include RSpec::Rails::RailsExampleGroup | ||||||
|
||||||
it 'runs normally' do | ||||||
expect(CurrentAttrsBetweenHooks.request_id).to eq('123') | ||||||
end | ||||||
end | ||||||
|
||||||
expect( | ||||||
group.run(failure_reporter) ? true : failure_reporter.exceptions | ||||||
).to be true | ||||||
end | ||||||
end | ||||||
|
||||||
it 'does not reset ActiveSupport::CurrentAttributes before before-each hooks' do | ||||||
in_sub_process do | ||||||
group = | ||||||
RSpec::Core::ExampleGroup.describe('A group', uniquely_identifiable_metadata) do | ||||||
include RSpec::Rails::RailsExampleGroup | ||||||
|
||||||
# Client code will often have test setup blocks within "*_spec.rb" | ||||||
# files that might set up data or other environmental factors for a | ||||||
# group of tests in e.g. a "before" hook, but would reasonably expect | ||||||
# suite-wide 'around' settings to remain intact and not be reset. | ||||||
# | ||||||
before :each do | ||||||
expect(CurrentAttrsBetweenHooks.request_id).to eq('123') | ||||||
CurrentAttrsBetweenHooks.request_id = '234' | ||||||
end | ||||||
|
||||||
it 'runs normally' do | ||||||
expect(CurrentAttrsBetweenHooks.request_id).to eq('234') | ||||||
end | ||||||
end | ||||||
|
||||||
expect( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cosmetic
Suggested change
The second arg sent to |
||||||
group.run(failure_reporter) ? true : failure_reporter.exceptions | ||||||
).to be true | ||||||
end | ||||||
end | ||||||
end | ||||||
end | ||||||
end |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern with this approach is if some code called in a non-Rails example group is setting current attributes, and this would leak, because we reset only for Rails example groups.
Potentially, leak non-empty current attributes into a Rails example group that doesn't expect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. That is a shortcoming of the approach used in 6.1.3 already, of course, since it only includes the ActiveSupport helper inside RailsExampleGroup.
Do you have a "feel" for where else in the hierarchy we might include this module to achieve a wider area of effect? It can be put just about anywhere, especially with some
defined?
and/orrespond_to?
guards, and should be pretty bomb-proof. Once we've figured that out, we can decide if the adapter module's chosen namespace, as a Rails adapter, is correct or if it should also move elsewhere in the code base.