Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1k
Wrap tests in Rails executor when configured#2753
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Conversation
javierjulio commented Apr 6, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
To properly handle the Rails config switch and still reset state in either case, I figure what really needs to happen here is the following but wasn't sure if this would work (the config conditional) at that level. If it does, then I can update to see where test suite fails. if ::Rails::VERSION::MAJOR >= 7if ::Rails.configuration.active_support.executor_around_test_caseincludeddo |_other| arounddo |example| ::Rails.application.executor.perform{example.call}endendelserequire'active_support/current_attributes/test_helper'includeActiveSupport::CurrentAttributes::TestHelperrequire'active_support/execution_context/test_helper'includeActiveSupport::ExecutionContext::TestHelperendend |
javierjulio commented Apr 8, 2024
Thank you for approving CI. I've checked many of the runs and they are failing for the same error. I figure this has to do with a new |
javierjulio commented Apr 8, 2024
@pirj thank you. This is ready for another test run. |
Uh oh!
There was an error while loading. Please reload this page.
pirj left a comment
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.
Looks good, do you kind adding a spec that would prevent regressions?
I guess setting something Current and two examples in the same context with oder: :defined should do it.
javierjulio commented Apr 9, 2024
@pirj yes, I will reuse the tests from #2752 here. I need help with handling the Cases to consider:
This could handle the first and third case, but the second one is tricky because how would we include/use the two test helpers inside the around block? |
pirj commented Apr 9, 2024
I see, good point. This leads me to a thought if we can rely that this config will be correctly loaded up by the time we define the RailsExampleGroup. And what is inside those two TestHelpers we include. |
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 now matches what Rails 7+ does
JonRowe commented Apr 10, 2024
I've rebased this |
883d87a to 13958ccCompareUh oh!
There was an error while loading. Please reload this page.
ngan commented Apr 18, 2024
We tried this branch on our monolith and tests were failing everywhere. I think the |
ngan commented Apr 18, 2024
This worked for us: RSpec::Core::Example.prepend(Module.newdodefrun(...)Rails.application.executor.perform{super}endend)I think we'd have to do the same here since context-level before hooks run before around hooks. Essentially, modify |
This recreates #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 #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 #2503 and #2752 which the latter is meant as a temporary fix since this is the expected way to reset state.