Skip to content
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

Conversation

pond
Copy link

@pond pond commented Jul 4, 2024

Compromise solution to #2773.
Fixes #2773

  • TL;DR - only resets 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.

config.around :each do | example |
  ActsAsTenant.without_tenant do
    example.run()
  end
end

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:

it 'does something that requires ActsAsTenant.without_tenant to be asserted' do
  some_operation()
end

...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:

config.around :each do | example |
  ActsAsTenant.without_tenant do
    example.run()
  end
end

...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).

# time their actual tests, or their test hooks ran.
#
RSpec.configure do | config |
config.around(:each, uniquely_identifiable_metadata()) do | example |
Copy link
Member

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, 👍

Copy link
Author

@pond pond Jul 4, 2024

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.

Copy link
Author

@pond pond Jul 4, 2024

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.

@@ -136,6 +136,24 @@ def method_name
end
end

# @private
#
# Reset ActiveSupport::CurrentAttributes only *after* tests run, rather
Copy link
Member

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.

Copy link
Author

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.

spec/rspec/rails/example/rails_example_group_spec.rb Outdated Show resolved Hide resolved
spec/rspec/rails/example/rails_example_group_spec.rb Outdated Show resolved Hide resolved
@pond
Copy link
Author

pond commented Jul 7, 2024

Note that CI will fail.

Committed changes as given but needed to add a missing def and getting a test failure now anyway. The new code is not equivalent to the old. This may take me substantial time to work out as I'm a bit confused by how that method is meant to work, if being called inside an example, so it can't possibly set a before-each or around-all since the example is already running; but then again, it's running that group example separately, so maybe that's now.

(Edit: It was easy, heh. Just didn't see wood for trees on the first glance).

@JonRowe
Copy link
Member

JonRowe commented Jul 9, 2024

@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

@pond
Copy link
Author

pond commented Jul 24, 2024

@JonRowe Any update?

@pond
Copy link
Author

pond commented Aug 7, 2024

@JonRowe / @pirj Just bumping this as it looks like it might have been forgotten.

end
end

expect(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetic

Suggested change
expect(
expect(group.run(failure_reporter)).to be(true), failure_reporter.exceptions

The second arg sent to to is a custom failure message.

Copy link
Member

@pirj pirj left a 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.

@pond
Copy link
Author

pond commented Aug 26, 2024

Could we at least merge this in the mean time, so I don't have to keep my Gemfile locked to 6.1.2?

@its-fern
Copy link

+1 to @pond 's comment - we're currently pinned to 6.1.2 due to this issue and would love to upgrade soon!

@JonRowe
Copy link
Member

JonRowe commented Aug 28, 2024

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.

@pond
Copy link
Author

pond commented Aug 28, 2024

@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!

@pond
Copy link
Author

pond commented Sep 3, 2024

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?

@JonRowe JonRowe closed this Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resetting ActiveSupport::CurrentAttributes can be inconsistent due to hook ordering
4 participants