correctly set test adapter when configure the queue adapter on a per job #26690
Conversation
|
r? @arthurnn (@rails-bot has picked a reviewer for you, use r? to override) |
y-yagi
force-pushed
the
load_job_class_before_run_test
branch
from
5b0f708
to
2e487db
Compare
| @@ -10,6 +10,7 @@ module TestHelper | |||
|
|
|||
| def before_setup # :nodoc: | |||
| test_adapter = queue_adapter_for_test | |||
| self.class.to_s.gsub(/Test\z/, "").safe_constantize | |||
There was a problem hiding this comment.
This line is confusing, as we constantize something, and don't assign it to any variable.
Would be nice to add a comment in it, to make it more clear.
There was a problem hiding this comment.
I'd write this as self.class.name.chomp('Test').safe_constantize.
What happens if users call tests SomethingUnconventional?
I found the original issue tough to grasp, but is this the best way we can find to solve the issue?
There was a problem hiding this comment.
What happens if users call tests SomethingUnconventional?
ActiveJob::TestCase does not have tests method. So, calling tests will an error.
but is this the best way we can find to solve the issue?
I think your concern is correct. I think again about the solution.
There was a problem hiding this comment.
ActiveJob::TestCasedoes not havetestsmethod
People could include ActiveJob::TestHelper into an ActiveSupport::TestCase class and then it'd be there, iirc.
I think your concern is correct. I think again about the solution.
❤️
The `ActiveJob::TestHelper` replace the adapter to test adapter in `before_setup`. It gets the target class using the `descendants`, but if the test target job class is not loaded, will not be a replacement of the adapter. Therefore, instead of replacing with `before_setup`, modified to replace when setting adapter. Fixes rails#26360
y-yagi
force-pushed
the
load_job_class_before_run_test
branch
from
2e487db
to
e94aee7
Compare
y-yagi
changed the title
|
I changed the solution. |
|
@kaspth thoughts? |
|
Just avoid to use the conflict fix button on github because it creates an extra commit 😢 |
I rebased/squash in the end =) |
Summary
The
ActiveJob::TestHelperreplace the adapter to test adapter inbefore_setup.It gets the target class using the
descendants, but if the test target jobclass is not loaded, will not be a replacement of the adapter.
To avoid this, modified to load the test target class.
Fixes #26360