ActiveJob: support per-worker queue name configuration

[victorpolko]

Reasoning

Sometimes it's necessary to be able to push some overridden worker jobs to a queue different from globally defined.

Current implementation of Support::Backends.enqueue_active_job method doesn't allow for specifying a queue name for a dedicated worker – the ActiveJob's queue_as setting gets ignored in favor of the globally-configured queue name:

# config/initializers/carrierwave_backgrounder.rb
CarrierWave::Backgrounder.configure do |c|
  c.backend :active_job, queue: :global_queue
end

# app/models/user.rb
class User < ApplicationRecord
  mount_uploader :avatar, AvatarUploader
  process_in_background :avatar, AvatarUploaderWorker
end

# app/uploaders/avatar_uploader.rb
class AvatarUploader < CarrierWave::Uploader::Base
  include ::CarrierWave::Backgrounder::Delay

  # some processing here
end

# app/workers/avatar_uploader_worker.rb
class AvatarUploaderWorker < ActiveJob::Base
  queue_as :worker_queue # <= is ignored
  queue_as { :worker_queue } # <= is ignored
end

# Processing happens in :global_queue!

Strategy

ActiveJob uses .set method to change the queueing settings.
With the rest, I mimicked the Support::Backends.enqueue_sidekiq logics: default or nil queue names are ignored for consistency, however, it's possible to force the default as queue name by using queue_as method with a block:

class AvatarUploaderWorker < ActiveJob::Base
  queue_as { 'default' } # <= will override whatever is configured in the initializer
end

Tests

Added 8 test cases, 4 of which are really related to the changes.

Extras

Also, this PR:

1. Fixes some typos in README.md.
2. Fixes a typo in backends_spec.rb.
3. Adds some newlines at EOF in several files.
4. Addresses [Question] How to set queue on custom worker? #223 by adding a sidekiq queue name configuration example to the README.md.
5. Answers Uploader specific queue #162 by adding queue name configuration examples to the README.md.

[lardawge]

In general, I am leaning into integration specs vs mocking/stubbing. I find it harder to maintain, and it relies on mocks being set up correctly as well as library apis never changing... which is never the case.

I am happy to merge this PR if you can find a way to use the integration specs.

[victorpolko]

@lardawge
Thank you!

I found several problems in my changes while working on the integration specs, so I had to dig deeper into this gem and active_job, and it was a nice trip.
Also added integration specs for Sidekiq backend.
I had to use some hacks to simplify the spec setup, but the result is stable.

[lardawge]

Thanks for getting this close. Much appreciated.

There are a couple of things that seem to have happened, which are adding to the complexity of testing.

The initial idea was to offload any switching of backends to separate test runs using env vars so that we didn't have to do any hackery to test the different scenarios, AJ vs Sidekiq.

That means the tests themselves must be agnostic to what needs to be loaded, and they should not care. All loading of appropriate files occurs in the config method, based on the backend. There is no scenario where one would need to switch back and forth between AR and Sidekiq in a real-world situation.

I understand the problem is trying to test custom workers. That is leading to an issue where the gymnastics need to happen. Another approach might be to require the files in rails_helper.rb when you know what backend is being tested. That should drastically simplify the approach to testing.

Let me know if that makes sense.

[victorpolko]

It's actually a surprise to me that tests have failed, because I mostly worked on them :)
I tested with BACKEND=active_job bundle exec rake, and BACKEND=sidekiq bundle exec rake, and they were all green.

I'd love to fix them, but I don't know how to make them fail in my local env.

[victorpolko]

Excluding Specs

I found a way to exclude certain specs based on the ENV variable.
For clarity, I moved the backend-specific specs to folders.

# spec/spec_helper.rb
RSpec.configure do |c|
  c.include WarningSuppression

  if ENV['BACKEND'] == 'active_job'
    c.exclude_pattern = 'spec/integrations/sidekiq/*'
  elsif ENV['BACKEND'] == 'sidekiq'
    c.exclude_pattern = 'spec/integrations/active_job/*'
  end
end

Failing Tests!

However, with this I started getting red tests!
Why now? I don't know.
The error is the one that's present in the CI output:

NameError:
  uninitialized constant CarrierWave::Workers::ActiveJob
# ./spec/support/dummy_app/app/jobs/document_uploader_active_job.rb:1:in '<top (required)>'
# /Users/vpolko/.rvm/gems/ruby-3.4.5/gems/zeitwerk-2.7.3/lib/zeitwerk/core_ext/kernel.rb:26:in 'Kernel#require'
# ./spec/support/dummy_app/app/models/admin.rb:16:in '<class:Admin>'
# ./spec/support/dummy_app/app/models/admin.rb:1:in '<top (required)>'

Reason

It happens during BACKEND=sidekiq run, because:

1. models/admin.rb has this overridden worker configuration with process_in_background :documents, DocumentUploaderActiveJob,
2. which means that this file has to be loaded, and so it tries to, but
3. ::CarrierWave::Workers::ActiveJob::ProcessAsset actually is not initialized, because
4. CarrierWave::Backgrounder.configure only requires default worker files if ENV['BACKEND'] is active_job, which is not the case

A Fix?

I can fix this with several approaches:

• Remove process_in_background :documents, DocumentUploaderActiveJob from models/admin.rb (leaving just process_in_background :documents) and set it using either Open Class or class_eval in the integrations/active_job/enqueuing.spec
• Require default worker files in rails_helper.rb with a condition BACKEND['ENV'] == 'sidekiq'
• Add a condition to only process_in_background :documents, DocumentUploaderActiveJob if BACKEND['ENV'] == 'active_job' in models/admin.rb
• Add a condition to set process_in_background :documents, DocumentUploaderActiveJob if BACKEND['ENV'] == 'active_job' using Admin.class_eval in rails_helper.rb
• Create a dedicated model for ActiveJob backend testing

All these options smell, so I'd love to know what's your opinion here @lardawge.

[lardawge]

I spent some time this morning taking a fresh stab at this feature. It is clear, or I believe this was attempted using AI (correct me if I'm wrong). It made a hash of it. I could not make heads or tails of the spec files, which I use and expect to be clear, given that it is documentation. There are so many changes in this PR that it was easier for me to tackle it separately.

The main challenges are that testing needs to be isolated with and without AJ. As you experienced, referencing files that are not loaded when running sidekiq in isolation becomes an issue. We need to set up a Rails app that does not necessarily mimic the real world.

I still am not 100% happy with the setup. For example, I am testing a config, but skipping the part about the actual config because of the limitations of Rails initializers and the inability to easily reload the app. I think it is fine since we know the initializer works and have other tests to prove it. We are then only testing the different scenarios... either way, take a look and let me know if it makes sense.

PR #335

[victorpolko]

The spec files are just covering all possible permutations on 3 levels for each backend type: global queue configuration, worker queue configuration and different corner-cases like passing nil, default or a block.

I also described ActiveJob's :queue_as method for clarity.
And then updated README.md a little.

Sorry for the many changes.

I don't use AI, it frightens me.

[lardawge]

I am closing this in favor of the less verbose version #335. I appreciate this PR as well as the effort put into it.
There are some changes in this branch around documentation that I would welcome as a separate PR. I would appreciate the usage of the main branch in the real world before I release. Cheers!