Remove IntegrationTest::Behavior from SystemExampleGroup

[jrochkind]

To better mirror current Rails ActionDispatch::SystemTestCase. And resolve problems with inability to set ActiveJob queue type in systems example group, closing #2232

This one-line change has a lot of background to explain, that was a bit tricky to figure out...

Background

The Rspec SystemExampleGroup was based on the Rails ActionDispatch::SystemTestCase, which originally sub-classed Rails ActionDispatch::IntegrationTest.

However, in Rails6, it was noticed that System tests now refused to respect your ActiveJob::Base.queue_adapter settings, reported to Rails at rails/rails#37270

It was noted in a comment there that this had already been fixed in Rails master rails/rails#36283 , but needed to be backported to Rails 6-0-stable.

The relevant backport commit ended up at rails/rails@ea77dbf, and that's the easiest place to read what the fix was -- basically changing ActionDispatch::TestCase to inherit from the basic ActiveSupport::TestCase instead of ActionDispatch::IntegrationTest, cause the IntegrationTest stuff was messing with things.

That resolved things in Rails (I think in 6.0.1 release, although not sure why that backport commit doesn't show up as being in v6.0.1).

But a very similar problem still existed in rspec-rails, reported at #2232, still present with Rails 6.0.1 or 6.0.2.

Finding the Rails history, I wondered if making the analogous change -- removing the Rails IntegrationTest::Behavior from the rspec-rails SystemExampleGroup -- would resolve the problem in rspec-rails.

It seemed to!

The url_options business

The original rails commit in addition to changing the superclass, also adds a url_options method to SystemTestCase, intended to make it so Rails urls generated from a system test will default to host: Capybara.app_host.

Apparently this was the only behavior from Rails ActionDispatch::IntegrationTest that was of use, so it was copied over manually.

This PR does not do that for rspec. It would be nice to have this behavior in rspec-rails system tests. I have run into confusion and problems from not having it before myself.

However, I was unable to succesfully implement this in rspec-rails -- the default host to Capybara.app_host behavior. And I believe rspec-rails system tests did not have this behavior even prior to the change in this PR, it is not a regression to lose it, it was never there, even when including IntegrationTest::Behavior. It looks like default url options behavior is really hard for rspec-rails to handle consistently with rails minitest, this isn't the first time it's come up. I tried, but couldn't figure out how to get it this particular behavior, default host to Capybara.app_host to work in rspec-rails, before or after this PR.

The sadly missing tests.

It's not great that this one-liner change has no tests.

It seems maybe not a great sign that making this change did not cause any existing tests to fail. (Or maybe it means we're on the right path?).

But ideally I would have added a test that failed prior to this change, and was green after this change. But I was unable to write an automated test around setting ActiveJob::Base.queue_adapter to fail prior to this change.

So how do I know it works at all? Well, in my own real app, system tests involving manually changing ActiveJob::Base.queue_adapter were failing with Rails 6.0.2 and rspec-rails 4.0.0.beta3. And they pass in 4.0.0.beta3 with this one change. (They also pass in Rails 5.2.4 and rspec-rails 4.0.0.beta3, both with and without this change).

I'm not sure why I couldn't reproduce in a test what I'm seeing in my own app. This stuff is tricky. But maybe since all existing tests still pass, it's good enough to merge without a new test, with just the evidence of "matching what Rails is doing" and my report?

Otherwise, I'm probably going to need some help, I'm not sure I'm going to be able to come up with a failing test, although I can spend a few more hours trying.

[benoittgt]

Thanks @jrochkind

For the missing test. Why not starting from your reproduction script that you provide in #2232?

Interesting thoughts for url_options. I am tempted to look at it again and try to get rid of this amount of bad vibes... #1275 (comment)

[jrochkind]

@benoittgt yep, that is where I started, I couldn't get it to fail in the context of rspec-rails spec suite. There may be something subtly different in the environmental conditions.

If you had time and interest to take a stab at it yourself and see if a different set of eyes/hands can arrive somewhere else, that'd be super helpful!

re: the url_options business: I can say as just one anecdotal example, the lack of rspec-rails system examples doing this consistently with rails system tests did specifically trip me up, cause tests to fail, lead to confusion figuring out how to resolve it. It would be great if it could be resolved; and you're right, the thumbs down on that other issue suggests similar things are tripping other people up. But I tried to look into it and it's very confusing what's going on in rails land and it's interaction with rspec-land. It would be nice to fix, but I don't believe a blocker for solving this other problem, since this PR does not make that part any worse (or better).

(Warning: I am about to go on vacation for two weeks).

[JonRowe]

Can you rebase this from master?

[jrochkind]

@JonRowe Done!

Also, to be clear, I think I can reproduce the problem in a freshly generated otherwise empty Rails app (although I might not have time to produce such a demo/repro app until January, as I'm about to be on vacation). It's not unique to my app in particular.

I just couldn't manage to reproduce it in the context of rspec-rails spec suite -- I'm thinking there might be something different in environmental context between an independent app, and rspec-rails's embedded specs. Which might (or might not) mean it could be more easily reproduced in the parts of the rspec/cucumber suite that actually generate an independent app, but I haven't managed to figure out how those work yet.

[jrochkind]

any thoughts on this from maintainers, @JonRowe or @benoittgt ?

This is currently the only/last thing blocking me from upgrading my Rails app to Rails 6. I would love to get it taken care of one way or another. Sadly, I don't think there's any way to "monkey patch" the change in, so I'm relying on it being made in an rspec-rails release (unless I fork, I suppose!).

Thoughts welcome!

As I said, I am able to produce a demo app reproducing the failure, but have not been able to transform that into a failing test, despite spending time trying. I could make a demo app that uses rspec-rails v4.0.0.beta3 and Rails 6 and has a test that fails; and another demo app that is identical in all ways except for being Rails 5.2.x, where the test passes. Two demo apps, or one demo app you can manually switch the rails version on, whatever you'd like, if you think it'd be helpful.

[JonRowe]

@jrochkind if you can create a reproduction app that fails but would be fixed by this that'd be great, as we can clone it and verify it, and see if we can help with a test, its just a change I'm hesitant to make without some additional verification (and I'm busy with some personal stuff post holidays so time poor!)

[jrochkind]

Reasonable! Will do. I will make an app running under Rails 6 and rspec-rails 4.0.0.beta3, with a test that fails (and should pass), and which does pass if you either change Rails version to 5.2, or change rspec-rails version to this branch.

[JonRowe]

Yes perfect.

[benoittgt]

I worked a little bit on a reproduction case yesterday (with a feature test). I am happy if you can provide a reproduction case @jrochkind.

I will post my findings here today.

[benoittgt]

Hello

Here is a feature test in system spec that is red without this line removal and green when removed.

--- a/features/system_specs/system_specs.feature
+++ b/features/system_specs/system_specs.feature
@@ -48,3 +48,36 @@ Feature: System spec
         When I run `rspec spec/system/widget_system_spec.rb`
         Then the exit status should be 0
         And the output should contain "1 example, 0 failures"
+
+    Scenario: the ActiveJob queue_adapter can be changed
+      Given a file named "spec/system/some_job_system_spec.rb" with:
+      """ruby
+      require "rails_helper"
+
+      class SomeJob < ActiveJob::Base
+        cattr_accessor :job_ran
+
+        def perform
+          @@job_ran = true
+        end
+      end
+
+      RSpec.describe "spec/system/some_job_system_spec.rb", :type => :system do
+        describe "#perform_later" do
+          before do
+            ActiveJob::Base.queue_adapter = :inline
+          end
+
+          it "perform later SomeJob" do
+            expect(ActiveJob::Base.queue_adapter).to be_an_instance_of(ActiveJob::QueueAdapters::InlineAdapter)
+
+            SomeJob.perform_later
+
+            expect(SomeJob.job_ran).to eq(true)
+          end
+        end
+      end
+      """
+      When I run `rspec spec/system/some_job_system_spec.rb`
+      Then the example should pass

This not super beautiful but it's will validate your behavior.

[jrochkind]

Oh awesome, thanks @benoittgt! You think I should add that to this PR? And then it will be good to go? (Sounds like no need for me to prepare an independent/separate reproduction app anymore).

(I wasn't able to get a failing test in the rspec suite, but hadn't tried in the cucumber feature suite, cucumber is still pretty unfamilar to me. It may be the different setup in the cucumber land was necessary to repro. So thanks!)

[benoittgt]

Let's see how #2253 goes. 😊

[jrochkind]

Oh, I got confused what was going on. OK, so @benoittgt added a test case here -- thank you!!

Is this mergeable now? Or what are we waiting on? (Warning, once this is merged I'm gonna ask: So, how about releasing a 4.0.0.beta4 with this? Note also, except for this, rspec 4.0.0beta is working fine for my app I'm trying to get upgraded to rails 6, hooray).

[benoittgt]

@jrochkind Yep. Sorry for the weird move. We are waiting for a review from Jon or Phil and final merge from Jon.

For the release, Jon will be able to answer you.

[pirj]

No objections.
Thanks for your contribution!

[JonRowe]

Minor nit pick, but otherwise LGTM @benoittgt

[benoittgt]

No problem @jrochkind! Thanks for your work. Waiting for the CI now. 😊

[jrochkind]

Thank you @pirj @JonRowe @benoittgt !

Alas, it looks like this didn't make it into 4.0.0.beta4, released Jan 12. And thus my app still can't pass it's test suite on Rails 6. :(

Is there any chance of a 4.0.0.beta5? I would love to be able to actually move my app to rails6...

[JonRowe]

@jrochkind You can pull rspec-rails from Github until it's released.

[jrochkind]

@jrochkind You can pull rspec-rails from Github until it's released.

I tried that and ran into additional headaches I don't understand. Just adding github master reference to my Gemfile for rspec-rails resulted in this error on bundle:

Could not find gem 'rspec-core (= 3.10.0.pre)', which is required by gem 'rspec (~> 3.0)', in any of the sources.

Apparently master requires rspec-core 3.10.0.pre (and exactly that version, no more no less?!), even though the beta releases do not, they are happy with rspec-core ~> 3.9, they don't require a pre-release rspec-core.

I could probably fix that by altering my gemfile to allow the pre-release rspec-core... but A) I don't really want to deploy my app with rspec-core pre-release when I don't have to, the rspec-rails beta's generally work fine with existing rspec-core, and B) actually there is no rspec-core 3.10.0.pre released to rubygems either, I'd have to pull rspec-core directly from github too, even worse.

If I was willing to do that and try.. now it complains about a conflict with rspec, maybe I need to now get rspec directly from github instead of a release too.

There seem to be expanding confusing changes to my Gemfile I'd have to make in order to get rspec-rails from github, and while I'm willing to use a pre-release or even unreleased github rspec-rails even in production, I'm very nervous about letting that virally infect a bunch of other dependencies and be running unreleased master of all of them. 🤷‍♂

[pirj]

As a simple temporary solution, I suggest you point to your fork and get rid of this conditional statement @jrochkind . Sorry for the inconvenience.

[jrochkind]

Thanks @pirj , I guess I will fork if i have to. Too bad this one missed the beta release by a couple days, after 3 months of no releases, that's kinda frustrating, I don't know if that means it could be 3 months before another release.

I wish there were a way to 'monkey-patch' this in, but there really isn't.

Is there any expected time of next beta release, or even a final release? Looks like 4.0 has been in beta for about 8 months?

[JonRowe]

Thanks @pirj , I guess I will fork if i have to. Too bad this one missed the beta release by a couple days, after 3 months of no releases, that's kinda frustrating, I don't know if that means it could be 3 months before another release.

I was going to call this out as wrong, but yikes 3 was back in October, the thing is we're all volunteers and the holidays sucked a lot of time up.

I wish there were a way to 'monkey-patch' this in, but there really isn't.

We previously have recommended just switching all gems to Github, but as RSpec::Rails is stepping out of lock with other gems I'm investigating removing the requirement so at least pointing just rspec-rails at Github will work.

Is there any expected time of next beta release, or even a final release? Looks like 4.0 has been in beta for about 8 months?

Nope. Volunteer open source project means its hard to plan with any kind of certainty, I will probably ship another beta soon I'm just waiting on some more PRs to finish up, the final release was slated for last April would you believe, but as a major release theres a few more PRs to get in before shipping.

[jrochkind]

Does anyone know exactly what version of rspec-rails this PR landed in?

Normally I'd find out by looking at the commit in github, where it would list what tags included this commit (including version tags)... but in this case github says:

"This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository."

Maybe a rebase was involved somewhere?. I think this fix DID make it into rspec-rails.... in helping other people troubleshoot problems, it can be useful to know what version has this fix...

OK, talking to myself, I guess I can try to figure it out from the other end with git history on the file in master...

OK, I guess the commit in this PR at some point got rewritten into this one: 3e4c770

Which apears in v4.1.0 all the way up to v5.0.2, so okay! (although still weird that the covnersation here makes it look like it was going ot make it into 4.0.0 maybe it actually did too under a different commit hash? Anyway, a rough estimate!)

[JonRowe]

I believe this was a squash merge, or a rebase and merge, you can see the merge commit contains all your changes but is not the same commit as in the branch.