Reset CurrentAttributes on each rails example #2752
Conversation
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 rspec#2712 can be revisited.
This recreates rspec#2712 since the GHA logs are no longer available which we need to address this issue. We should be wrapping the Rails examples with the executor to mimic what Rails v7 does (as noted in rspec#2713) to properly reset state. This has been tested in a test suite for an app but we need to address what issues there is within rspec-rails. This change request originated from rspec#2503 and rspec#2752 which the latter is meant as a temporary fix since this is the expected way to reset state.
Copied this from Rails source but I forgot to update before committing originally.
Now that we've confirmed the failing test, by including RailsExampleGroup, it should reset CurrentAttributes as expected.
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.
CI reported on line 2.
|
@pirj this is ready for review. Considering the current state of rspec-rails, I figure its best to start here since its basically missing this CurrentAttributes test helper inclusion to ensure parity with Rails, pre-executor. I've added tests here that I confirmed failed first and now pass with the updated module. |
| @@ -1,6 +1,10 @@ | |||
| module RSpec::Rails | |||
| RSpec.describe RailsExampleGroup do | |||
| if ::Rails::VERSION::MAJOR >= 7 | |||
| class CurrentSample < ActiveSupport::CurrentAttributes | |||
There was a problem hiding this comment.
What fails exactly without this?
At a glance it may looks that it’s the following example that check for tagged logger, but I guess it’s not, is it?
| @@ -32,5 +36,18 @@ module RSpec::Rails | |||
|
|
|||
| expect(results).to all be true | |||
| end | |||
|
|
|||
| describe 'CurrentAttributes', order: :defined, if: ::Rails::VERSION::MAJOR >= 7 do | |||
There was a problem hiding this comment.
The concept is hard to absorb at first, but here there are two types of groups, the first that is part of our spec suite, that is first evaluated when you run rspec over rspec-rails specs, but before running any specs. The second is defined during specs runtime, like RSpec.describe you can see above. I wouldn’t mix in RailsExampleGroup into the former, but it can be freely done with the latter. See example above for the reference.
There was a problem hiding this comment.
@pirj I didn't get to this in time. Seeing Jon's update, I'm just not familiar with how specs are written here and struggled a bit. I'll know how to better approach it in the future. Thanks!
|
Thanks for tackling this, I took the liberty of refactoring the spec to what we'd usually do I hope you don't mind. |
Reset CurrentAttributes on each rails example
This recreates rspec#2712 since the GHA logs are no longer available which we need to address this issue. We should be wrapping the Rails examples with the executor to mimic what Rails v7 does (as noted in rspec#2713) to properly reset state. This has been tested in a test suite for an app but we need to address what issues there is within rspec-rails. This change request originated from rspec#2503 and rspec#2752 which the latter is meant as a temporary fix since this is the expected way to reset state.
|
@JonRowe I don't mind, I was just trying to figure it out and contribute as best I could. Thank you again! |
|
Released in 6.1.3 |
|
This was a great idea and well intentioned but regrettably it has introduced a serious bug. Pleas see #2773. |
We need to include the
ActiveSupport::CurrentAttributes::TestHelperin the rails example group, otherwise rspec does not resetActiveSupport::CurrentAttributesaround each example unless being reset manually. We need to mimic what Rails v7 does for test setup for the "else" case until #2712 can be revisited. Related to #2713. This should address #2503 properly.In the meantime, I've created #2753 which recreates #2712 with some new changes since the GHA logs for the original are no longer available, we can't address without a new attempt. Ideally, #2753 would replace this with the suggestion from #2753 (comment) if it would work.