ActiveJob::QueueAdapters::TestAdapter is now a singleton
#19052
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is there any advantage of using a singleton? I know our current setup doesn't allow parallel test but in future we may change it. If we change it to a singleton now it will just make harder to implement parallel support, so unless we have a good reason to not use instances I'd like to keep it. |
| @@ -17,8 +17,6 @@ module ClassMethods | |||
| def queue_adapter=(name_or_adapter) | |||
| @@queue_adapter = \ | |||
| case name_or_adapter | |||
| when :test | |||
There was a problem hiding this comment.
@rafaelfranca the only advantage is that it removes this special handling, which is a weird snowflake.
I believe this doesn't really get us further away from parallel tests - if #16992 is merged, each test will be able to create its own anonymous subclass, which might actually be pretty simple to do. What do you think?
|
This needs a rebase |
Since `ActiveJob::TestHelper` globally sets `ActiveJob::Base.queue_adapter` on setup, there is no benefit in instantiating a new `TestAdapter` per tests. The original rationale was to allow parallel tests to run without interference, but since they'd all mutate the global `ActiveJob::Base.queue_adapter`, that was never realized.
|
@rafaelfranca resolved |
tamird
force-pushed
the
testing-singleton
branch
from
50c6bc0
to
802f855
Compare
rafaelfranca
added a commit
that referenced
this pull request
Feb 24, 2015
`ActiveJob::QueueAdapters::TestAdapter` is now a singleton
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since
ActiveJob::TestHelperglobally setsActiveJob::Base.queue_adapteron setup, there is no benefit ininstantiating a new
TestAdapterper tests. The original rationale wasto allow parallel tests to run without interference, but since they'd
all mutate the global
ActiveJob::Base.queue_adapter, that was neverrealized.
@cristianbica @jeremy @dhh