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

Wrap tests in Rails executor when configured #2753

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

javierjulio
Copy link
Contributor

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.

@javierjulio javierjulio changed the title Wrap tests in Rails executor when configured or on Rails 7 by default Wrap tests in Rails executor when configured Apr 6, 2024
@javierjulio
Copy link
Contributor Author

javierjulio commented Apr 6, 2024

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 >= 7
  if ::Rails.configuration.active_support.executor_around_test_case
    included do |_other|
      around do |example|
        ::Rails.application.executor.perform { example.call }
      end
    end
  else
    require 'active_support/current_attributes/test_helper'
    include ActiveSupport::CurrentAttributes::TestHelper

    require 'active_support/execution_context/test_helper'
    include ActiveSupport::ExecutionContext::TestHelper
  end
end

@javierjulio
Copy link
Contributor Author

Thank you for approving CI. I've checked many of the runs and they are failing for the same error.

     NameError:
       uninitialized constant RSpec::Rails::OpenStruct
     # ./spec/rspec/rails/example/view_example_group_spec.rb:190:in `controller'
     # ./spec/rspec/rails/example/view_example_group_spec.rb:196:in `block (3 levels) in <module:Rails>'

I figure this has to do with a new json gem version as I encountered an OpenStruct error with turbo_tests recently. I just don't know how to resolve it here since I don't see any Gemfile.lock files in this repo to confirm. In the meantime, I'll push an update to require ostruct in the reported file.

@javierjulio
Copy link
Contributor Author

@pirj thank you. This is ready for another test run.

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.

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
Copy link
Contributor Author

@pirj yes, I will reuse the tests from #2752 here. I need help with handling the executor_around_test_case config since there are multiple cases here that I think we need to handle, unless you feel otherwise.

Cases to consider:

  • Rails 7+ and executor_around_test_case is true (or load_defaults is 7.0)
  • Rails 7+ and executor_around_test_case is false
  • Rails 6.1 and below

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
Copy link
Member

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
Copy link
Member

JonRowe commented Apr 10, 2024

I've rebased this

@JonRowe JonRowe force-pushed the patch-2 branch 2 times, most recently from 883d87a to 13958cc Compare April 10, 2024 09:24
@ngan
Copy link

ngan commented Apr 18, 2024

We tried this branch on our monolith and tests were failing everywhere. I think the .wrap {...} needs to be the first thing that happens. In Rails, when the option is enabled, the mixin overrides the run method on MiniTest. This means that the wrap happens before all of the before hooks:
https://ruby-doc.org/3.2.2/gems/minitest/Minitest/Test.html#method-i-run

@ngan
Copy link

ngan commented Apr 18, 2024

This worked for us:

RSpec::Core::Example.prepend(
  Module.new do
    def run(...)
      Rails.application.executor.perform { super }
    end
  end
)

I think we'd have to do the same here since context-level before hooks run before around hooks. Essentially, modify Example#run.

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.

None yet

4 participants