-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Active Job: Correctly use the desired test adapter in tests #48585
Conversation
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.
Seems like a solid approach to me and it would be great to see this fixed! I'd be happy to run the patch and verify once the test suite has been updated
f94467c
to
1c50e04
Compare
f35f0f7
to
1ee7198
Compare
…mediate` set This is an internal fix, not user facing. I noticed it while working on rails#48585. The `async` adapter has an `immediate` option, which should only be used in tests. This option should tell the adapter to run jobs inline. This works correctly with `perform_later`, but it does not work with `enqueue_at`, which is what other internal mechanisms such as `retry_job` use. This PR fixes this bug.
440fb35
to
c060ecf
Compare
2c72e41
to
4b297aa
Compare
@rafaelfranca can we get this merged? |
This would be a really welcome fix to a very confusing challenge in setting up your tests how you want. (I actually do want some test classes using :inline and others using :test). It would be great if some way could be found to get maintainer attention to make progress on this! |
I understand this is slated for 7.2. But for those of us who prefer to pin the rails dependency to a SHA, could this be merged ? |
I don't know if it'll be in 7.2. @byroot or @rafaelfranca do you have a verdict on that? If this is something you're conceptually okay with, I will work on the merge conflicts to get it mergable 👍 |
I don't have full context and wonder why it was done that way, but your point make sense to me. I'd say rebase and ping me when it's done. Rafael can revert if he disagrees. |
ba72b27
to
e846921
Compare
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.
@jonathanhefner in light of your recent docs changes, I'd love if you could review if the docs in this file all make sense for this PR 😍
e846921
to
491a095
Compare
|
||
|
||
class EnqueuedJobsTest < ActiveJob::TestCase | ||
if adapter_is?(:test) |
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.
This is needed becuase you can't skip
since #50188. Alternative would be to put a force_skip
in a setup
block.
d55bd17
to
9526d52
Compare
9526d52
to
82e4c80
Compare
82e4c80
to
2194a73
Compare
@@ -0,0 +1,217 @@ | |||
# frozen_string_literal: true |
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.
I would have liked to see this in railties/test/application/active_job/adapter_test.rb
, like #51036. That directory is kinda messy, and it'd be nice if they were organized by framework where applicable
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.
Ahh I didn't know that existed. Happy to move the test, or you can if you like.
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.
It's just a knit, but I think moving files around is cosmetic enough to not warrant a PR. Maybe next time? 🙏
Thanks so much for digging into this and fixing it!! Bumps to #37270 have been irritating my inbox for nearly 5 years! ❤️ |
This is really great and detailed research work, fixing a hard to follow problem. Thank you! 🙏 |
Motivation / Background
Currently if you set
config.active_job.queue_adapter = (anything)
inconfig/application.rb
orconfig/environments/test.rb
, this config will be respected by some test cases but not others.Specifically, in
ActionDispatch::IntegrationTest
,ActionMailer::TestCase
, andActiveJob::TestCase
, the test adapter will still beTestAdapter
, while in other test cases it will beInlineAdapter
.I think you'd expect that if you set the test adapter for an environment, that test adapter would be available everywhere in that environment. For example, you might want to use the Delayed Job test adapter in your test environment so that you can have your test code more closely match production. The feedback on #37270 echoes this, but the workaround suggested there of disabling the test adapter on specific klasses is not reliable.
Detail
The logic to determine which queue adapter a job uses is quite confusing.
Here is how it works on
main
currently:ActiveJob::TestHelper
and overridesqueue_adapter_for_test
, then thequeue_adapter_for_test
adapter is used.ActiveJob::TestHelper
, then the:test
adapter is used.self.queue_adapter
is set on the job class, that adapter is used.self.queue_adapter
is set on the job's superclass (eg.ApplicationJob
), that adapter is used.Rails.application.config.active_job.queue_adapter
as configured by the user.Rails.application.config.active_job.queue_adapter
defaults to:async
.:async
if no Rails config set (eg. using Active Job as a standalone).As noted above, only some of the built in test classes include
ActiveJob::TestHelper
. Also, setting a queue adapter on a specific job class is rare (typically you would not want different jobs to be performed by different backends). So in practice, in development/production, option 5 is used. And in test, options 2 and 5 are used, depending on the type of test.This PR changes the logic to work as follows:
self.queue_adapter
is set on the job class, that adapter is used.self.queue_adapter
is set on the job's superclass (eg.ApplicationJob
), that adapter is used.ActiveJob::TestHelper
and overridesqueue_adapter_for_test
, then thequeue_adapter_for_test
adapter is used.Rails.application.config.active_job.queue_adapter
as configured by the user.Rails.application.config.active_job.queue_adapter
defaults to:test
ifRails.env.test?
, otherwise it defaults to:async
.ActiveJob::TestHelper
, and no other adapter has been set (eg. using Active Job as a standalone), then use:test
.:async
.The key changes are:
queue_adapter
on a job class, that will always be respected.queue_adapter_for_test
override still works, but it no longer takes priority over a queue adapter set on a specific job class. Since I think both of these techniques are rare, and the impact of this change only exists in a test environment, I think it is a relatively safe change.:test
in thetest
environment, and:async
in all others.)The default rails environment templates suggest setting a specific queue adapter only in production. For users who do that, there will be no changes to behaviour with this PR.
For users who are more granular, by setting per-job queue adapters, things should continue to work as expected with this PR.
The real change is for users who set a default queue adapter across all environments (as in this issue: #37270). For them, this PR solves the issue where some tests use the desired queue adapter, and other tests don't.
Additional information
Fixes: #37270
ref: bensheldon/good_job#846
ref: #26360 - this PR changes the behavior here, which I think was overzealous.
I extracted out some other PRs to fix internal issues while working on this: #48623, #48626. I also extracted #48599 out as a non-controversial fix prior to this PR.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]