Rework reset behaviour of ActiveSupport::CurrentAttributes

[pond]

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

[pond]

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]

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

@JonRowe Any update?

[pond]

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

[pond]

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]

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

[JonRowe]

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]

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

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?