-
-
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
Rework reset behaviour of ActiveSupport::CurrentAttributes #2774
Conversation
# time their actual tests, or their test hooks ran. | ||
# | ||
RSpec.configure do | config | | ||
config.around(:each, uniquely_identifiable_metadata()) do | example | |
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.
You can use in_sub_process to avoid config leaks between examples.
To differentiate between our examples and examples under test, we usually let(:example_group) { RSpec.describe("test example group") }
in specs.
Otherwise, 👍
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.
Had no idea about in_sub_process
. About to head home for the day but I'll look into that soon as it's likely to be very much cleaner. Thanks!
Separately, I've addressed the RuboCop complaints.
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.
...Actually, that was easy. Change pushed already; @pirj that's ready for another review whenever you have some time for it.
… on 'let' method calls are vital for clarity, personally...)
@@ -136,6 +136,24 @@ def method_name | |||
end | |||
end | |||
|
|||
# @private | |||
# | |||
# Reset ActiveSupport::CurrentAttributes only *after* tests run, rather |
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/or respond_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.
Co-authored-by: Phil Pirozhkov <pirj@users.noreply.github.com>
Co-authored-by: Phil Pirozhkov <pirj@users.noreply.github.com>
Co-authored-by: Phil Pirozhkov <pirj@users.noreply.github.com>
Co-authored-by: Phil Pirozhkov <pirj@users.noreply.github.com>
Note that CI will fail. Committed changes as given but needed to add a missing (Edit: It was easy, heh. Just didn't see wood for trees on the first glance). |
@pirj please don't merge this until I've had a chance to look at it, might be a bit slow because I'm away this week |
@JonRowe Any update? |
end | ||
end | ||
|
||
expect( |
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.
Cosmetic
expect( | |
expect(group.run(failure_reporter)).to be(true), failure_reporter.exceptions |
The second arg sent to to
is a custom failure message.
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.
I don’t have the bjections to this approach. It may happen that a Current would leak from a Rails e.g to a one that does test rails code but is not marked as such. We could say that our users should mark those groups accordingly, but it’s not a rare situation when people don’t, and this may cause them surprises. Like any shared state, which this Current is.
I wonder if we could instruct the suite to include an config-level around hook for rails example groups. It would take injecting into the users’s rspec conf object. I can’t tell if we already do this, or leave configuration to the user completely.
Could we at least merge this in the mean time, so I don't have to keep my Gemfile locked to 6.1.2? |
+1 to @pond 's comment - we're currently pinned to 6.1.2 due to this issue and would love to upgrade soon! |
Its not forgotten I'd just like for rspec-rails to not be responsible here, I'm thinking about alternative approaches to integrating the minitest hooks that match up with our lifecycles, or preventing around hooks from calling these apis due to this error. |
@JonRowe I certainly like the idea of a new and more holistic approach, and I'm sure it would be a relief to step away from hook ordering issue whack-a-mole as maintainers! |
I note as time goes by that there are now things like conflicts arising, but I'm not minded to spend time "maintaining" this PR unless I know it's of any interest. It sounds like @JonRowe you're thinking of a different approach anyway - perhaps I should just stay locked to RSpec 6.1.2 until that arises, and maybe just close this PR? |
Compromise solution to #2773.
Fixes #2773
ActiveSupport::CurrentAttributes
after examples run, rather than both before and after.Consider a
spec_helper.rb
that uses something like ActsAsTenant to wrap all examples, or anything similar where a block is used in order for whatever it is you're calling to handle cleanup itself automatically when the block finishes executing.In RSpec Rails 6.1.3, inclusion of
ActiveSupport::CurrentAttributes::TestHelper
means that attributes get reset both (in Rails terms) before setup and after teardown. What this means for RSpec hooks is that the resets happen between the suite-wide configuration "around" hooks and any examples or contexts. So, if we subsequently have an innocent test like this:...then the test will work with RSpec Rails 6.1.2 or earlier, but fail with 6.1.3, because ActsAsTenant uses
ActiveSupport::CurrentAttributes
under the hood and its no-tenancy assertion will have been reset between the "around" hook and the test run.The most robust solution would to be to try and insert an RSpec around-hook that is at the very end of the chain, and resets after running the example. This is difficult to do cleanly within RSpec Rails and there is a desire to not drift too far from what Rails does itself. Therefore, the compromise suggested here uses the "adapters" module pattern in RSpec Rails to add an adapter for attribute reset that does the after-teardown reset only. The compromise here is that yes, this still happens after any examples are finished, but going back to that code above:
...once
example.run()
returned, code that cared to check would find attributes were now already reset. This is considered pretty unlikely, and a better compromise than the known and hard to fix use cases of things like asserting tenancy, or a logged in user suite-wide in an around-each hook. I'm not aware of cases where a suite-wide configured around-each hook would care after an example about attribute data that was set within it (except perhaps to verify that it was reset - and it will be!). Bear in mind that this behaviour is present in RSpec Rails 6.1.3 anyway since it also has the teardown callback; this patch just avoids the on-setup reset.Test coverage is included and it's a bit gnarly because we do have to simulate an RSpec-configured "around each" hook without accidentally polluting subsequent tests with that configuration change (whether this would be harmless or not, it's bad behaviour and would very fractionally slow down the execution speed of RSpec Rails's test suite).