-
-
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
Wrap tests in executor when configured or on Rails 7 by default #2712
Conversation
when nil then ::Rails::VERSION::MAJOR >= 7 | ||
when true then true | ||
when false then false | ||
end |
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.
There is no need to do this. "Default in Rails 7" means it is set as true when calling config.load_defaults 7.0
. Otherwise, it should stay disabled.
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.
Do you mean it’s unnecessary to default to true on Rails 7, and it’s sufficient to just?
use_exectuor = ::Rails.configuration.active_support.executor_around_test_case
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.
Yes, exactly same as it is used in Rails
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.
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.
No description provided.