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

Can not change ActiveJob::Base.queue_adapter in Rails 6 system spec #2232

Closed
jrochkind opened this issue Dec 16, 2019 · 10 comments
Closed

Can not change ActiveJob::Base.queue_adapter in Rails 6 system spec #2232

jrochkind opened this issue Dec 16, 2019 · 10 comments

Comments

@jrochkind
Copy link
Contributor

Rails 6.0.2 (or 6.0.1), rspec-rails 4.0.0.beta3, rspec-core 3.9.0.

I am used to being able to set ActiveJob::Base.queue_adapter to :inline in some tests and :test in others -- depending on nature of test, one or the other may be more convenient.

In particular in system tests (which I understand are now recommended by rspec-rails over the older rspec-specific feature tests), it is often convenient to use :inline, to get actual behavior.

However, under Rails 6, I am unable to set ActiveJob::Base.queue_adapter in a system test, it remains set to the Rails TestAdapter.

This spec would be expected to pass, but fails:

require 'rails_helper'

describe "Testing Jobs System Spec", type: :system do
  class MyJob < ApplicationJob
    queue_as :default

    def perform(*args)
    end
  end


  it "does something" do
    expect_any_instance_of(MyJob).to receive(:perform)

    ActiveJob::Base.queue_adapter = :inline
    #puts ActiveJob::Base.queue_adapter
    MyJob.perform_later("one")
  end
end

Above fails. However, it passes if you change to type: :model or type: :job (or probably any type but :system).

It is a regression in the sense that this identical test will also pass under Rails 5.2.3 , rspec-rails 5.0.0.beta3 (or rspec-rails 4.x), rspec-core 3.9.0.

It is confusing how rspec-rails relates to ordinary rails here. It's possible this is a bug/change in Rails itself, but i haven't yet figured out how to reproduce it for a report to Rails in a way that makes sense.

Either way, I believe this action (setting ActiveJob::Base.queue_adapter on a per-test basis, specifically in system tests) is something rspec users will want to do, and were able to do in Rails 5.x, but no seem to be in Rails 6.x. I believe it is a problem.

Some related context may be provided in: rails/rails#37270 Although the original reporter there says their problem was resolved by Rails 6.0.2 -- however, the problem being reported here still reproduces in Rails 6.0.2.

@jrochkind
Copy link
Contributor Author

My attempt to make a reproduction in straight Rails does not reproduce, it does appear to be a problem specific to rspec-rails.

This following Rails system test passes in Rails 6.0.2, demonstrating it is possible to change ActiveJob::Base.queue_adapter in a Rails system test using standard rails testing.

require "application_system_test_case"

class JobsSystemsTest < ApplicationSystemTestCase
  class MyJob < ApplicationJob
    queue_as :default

    def perform(*args)
      $my_job_recieved_args << args
    end
  end

  setup do
    ActiveJob::Base.queue_adapter = :test
    $my_job_recieved_args ||= []
  end

  test "setting :inline adapter" do
    ActiveJob::Base.queue_adapter = :inline

    MyJob.perform_later("one")

    assert_includes $my_job_recieved_args, ["one"]
    assert_instance_of ActiveJob::QueueAdapters::InlineAdapter, ActiveJob::Base.queue_adapter
  end
end

@jrochkind
Copy link
Contributor Author

jrochkind commented Dec 16, 2019

It's appears to have something to do with ActiveJob::TestHelper.

I can't figure out what would have changed related to that in Rails 6. I can see how some of what's in there seems problematic, but it seemed problematic in Rails 5.2 also. There is some discussion of related in rails/rails#37270 , but I don't fully understand the discussion there; there was apparently some change in Rails 6.0.2 intended to fix somewhat similar problems, but it does not fix above problem, which does reproduce in Rails 6.0.2 as well.

I can't quite figure out where ActiveJob::TestHelper gets included into the class in various versions. But to confirm it does, I check ActiveJob::Base.method(:queue_adapter).source_location, and see if it's ActiveJob::TestHelper or not. It does seem to be included in Rails 5.2.x and 6.0.x, both rspec and Rails test case versions.

I can't figure out what would have changed to make behavior different with rspec and Rails 6 as above.

I've also been trying to figure out a workaround involving explicitly calling ActiveJob::TestHelper's enable_test_adapter, but haven't gotten that to work reliably yet -- although I can make it work in simple cases, I can't make it work in more realistic cases where I try to set adapter back to an original after test.

@JonRowe
Copy link
Member

JonRowe commented Dec 16, 2019

We don't tamper with this setting so its extremely odd if its something we're doing, it's more likely an interaction with ordering either of helpers or module inclusion et al, if you can find how they fixed it for minitest for 6.0.2 its likely that change can be applied here to help rspec-rails users.

@jrochkind
Copy link
Contributor Author

Thanks for reply. I can't really figure out what's going on at all.

I understand it's not something you're doing, but it's still something that breaks with rspec-rails under Rails 6.0, and makes something useful/necessary to do no longer possible. I suspect many people other than me are going to run into this.

Part of the confusion is that the way you do a number of things related to jobs and testing in Rails MiniTest seems pretty different than in rspec-rails. Combined with the fact that there is some weird old legacy code involved in ActiveJob::TestHelper.

, if you can find how they fixed it for minitest for 6.0.2 its likely that change can be applied here to help rspec-rails users.

So, I'm not sure the "it" is the same, I'm not sure they fixed "it" in minitest/rails. I know that rails/rails#37270 is related somehow, but I'm not certain it's the same issue reported. I'm also not sure it's fixed, it is still open in Rails. This is all very confusing. This is the PR mentioned in rails/rails#37270 as a fix... but I can't figure out why it wouldn't just apply to rspec-rails too, there's nothing obvious to me in there to port to rspec-rails.

If there was anyone else who had time to take a look, especially if they have some experience with this code, it would be very helpful. I'm pretty lost/blocked on it.

@jrochkind
Copy link
Contributor Author

OK wait. That rails change makes Rails SystemTestCase "Inherit from ActiveSupport::TestCase instead of ActionDispatch::IntegrationTest"

Would that be a corresponding change needed in rspec-rails SystemExampleGroup?

It looks like it's still got:

include ActionDispatch::Integration::Runner

And

other.include ActionDispatch::IntegrationTest::Behavior

Does one of or both of those need to be changed corresponding to rails/rails#36283 ?

Figuring out how Rails test classes map to Rspec classes and setup is new to me, I'm not totally following all of this.

@jrochkind
Copy link
Contributor Author

Okay, actually, just removing the line other.include ActionDispatch::IntegrationTest::Behavior from

other.include ActionDispatch::IntegrationTest::Behavior
makes my specs pass again.

That might corerspond to rails/rails#37270 SystemTestCase "Inherit from ActiveSupport::TestCase instead of ActionDispatch::IntegrationTest"?

Not sure if that's inconsistent with the include ActionDispatch::Integration::Runner that's still there.

I can make a branch run all rspec-rails tests to see if any break with that.

I could use at least a sanity check from someone more familiar with this code and with rspec-rails SystemExampleGroup specifically. Looks like that's maybe @penelopezone and @JonRowe I'm kind of just twiddling bits here, I don't totally follow the principles behind rspec-rails adaptation of rails test classes.

@jrochkind
Copy link
Contributor Author

jrochkind commented Dec 16, 2019

OK, the specific rails commit that was a backport in rails to 6-0-stable branch, that probably needs to be done equivalently in rspec-rails is rails/rails@ea77dbf

It looks like maybe if IntegrationTest::Behavior is removed, some custom url_options stuff might need to e done?

I am trying to prepare an rspec-rails PR, but not familiar with what's going on, having trouble figuring out what's going on in rspec-rails own tests, like can't figure out how/if these specs ever get run, and I'm not generally familiar with cucumber. I continue to work on it, but definitely appreciate any tips or ideas, for what's going on in general.

@benoittgt
Copy link
Member

Thanks for the detailed report @jrochkind. I will be happy to help you if I can in your PR.

@benoittgt
Copy link
Member

Closed by #2242

@akostadinov
Copy link

For anybody looking to fix their tests, also this can be used in all test types:

ActiveJob::Base.disable_test_adapter

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

No branches or pull requests

4 participants