Skip to content

Add a regression test for a removed method in 6.1 #2455

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

Merged
merged 1 commit into from
Feb 2, 2021

Conversation

pirj
Copy link
Member

@pirj pirj commented Feb 2, 2021

This method has been moved in rails/rails@3cece0b We do not include ActiveSupport::Testing::Assertion.

This resulted in failures when perform_enqueued_jobs do was called from specs.

Method was added back in rails/rails#40780, and was released in Rails 6.1.1

6.1.1 does not fix the issue in a spec that contains the following, though:

    perform_enqueued_jobs do
      SomeJob.perform_later
    end

We never included perform_enqueued_jobs method to be available in specs. Everywhere in our docs we recommend setting ActiveJob::Base.queue_adapter.perform_enqueued_jobs to true instead for those specs that need to perform jobs inline.

To make sure that assert_nothing_raised call doesn't blow up, we need to call perform_enqueued_jobs and for that - include ActiveJob::TestHelper that contains both of those definitions. And this is what Rails 6.1.1 has fixed.

Fixes #2410
Originally from #2412

Side note: It's not uncommon to perform jobs inline and check their side effects. Calling perform directly is not a direct replacement, because in this case argument serialization/deserialization is not covered.

This method has been moved in rails/rails@3cece0b
We do not include ActiveSupport::Testing::Assertion.

This resulted in failures when `perform_enqueued_jobs do` was called
from specs.

Method was added back in rails/rails#40780, and
was released in Rails 6.1.1
@pirj pirj self-assigned this Feb 2, 2021
@pirj pirj requested review from benoittgt and JonRowe February 2, 2021 09:39
@pirj pirj added this to the 4.1 milestone Feb 2, 2021
@JonRowe JonRowe merged commit ba5bab2 into rails-6-1-dev Feb 2, 2021
@JonRowe JonRowe deleted the add-assert_nothing_raised-regression-test branch February 2, 2021 10:52
@JonRowe
Copy link
Member

JonRowe commented Feb 2, 2021

To make sure that assert_nothing_raised call doesn't blow up, we need to call perform_enqueued_jobs and for that - include ActiveJob::TestHelper that contains both of those definitions. And this is what Rails 6.1.1 has fixed.

I'm confused, did 6.1.1 fix:

perform_enqueued_jobs do
  SomeJob.perform_later
end

Or not?

@pirj
Copy link
Member Author

pirj commented Feb 2, 2021

6.1.1 did fix that perform_enqueued_jobs and assert_nothing_raised are defined in the same ActiveJob::TestHelper, so it's sufficient to include ActiveJob::TestHelper to use both.
In 6.1.0, ActiveJob::TestHelper only defined perform_enqueued_jobs that was calling assert_nothing_raised itself and caused the failure.

We don't include ActiveJob::TestHelper ourselves, however. So if anyone was going to use assert_nothing_raised directly, they have to include ActiveJob::TestHelper, just like they do in order to be able to use perform_enqueued_jobs`.

@JonRowe
Copy link
Member

JonRowe commented Feb 2, 2021

Ah understood, can you add a spec demonstrating that? e.g. that if you include it you can use the block form?

@pirj
Copy link
Member Author

pirj commented Feb 2, 2021

@JonRowe It's already there:

      expect {
        perform_enqueued_jobs do
          :foo
        end
      }.to_not raise_error

@JonRowe
Copy link
Member

JonRowe commented Feb 2, 2021

Oh, 🤦

@benoittgt
Copy link
Member

Thanks for the finding @pirj

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.

3 participants