rspec / rspec-rails

RSpec for Rails 7+
https://rspec.info
MIT License
5.19k stars 1.04k forks source link

Add configuration to disable ActiveJob job signature matching check #2801

Closed smathieu closed 3 weeks ago

smathieu commented 2 months ago

Is your feature request related to a problem? Please describe.

The 7.0 version of RSpec Rails introduced https://github.com/rspec/rspec-rails/pull/2745 by @odlp which is overall a great addition, but doesn't provide a configuration to disable this check.

This is a problem for one of our app that uses before_enqueue and around_perform callbacks that modifies the arguments passed to the job.

Describe the solution you'd like

I would like a way to disable this check.

Describe alternatives you've considered

I'm not sure there are other solutions short of updating all the jobs in our app.

Additional context

Here are the callbacks being used:


class ApplicationJob < ActiveJob::Base
  # Automatically retry jobs that encountered a deadlock
  # retry_on ActiveRecord::Deadlocked

  # Most jobs are safe to ignore if the underlying records are no longer available
  discard_on ActiveJob::DeserializationError

  before_enqueue do |job|
    if Actor.current?
      job.arguments << { actor: Actor.current }
    end
  end

  around_perform do |job, block|
    arg = job.arguments.find { |a| a.is_a?(Hash) && a.key?(:actor) }
    if arg
      job.arguments.delete(arg)
      actor = arg[:actor]
    end

    if actor
      Actor.with(actor) do
        block.call
      end
    else
      block.call
    end
  end
end
os0x commented 2 months ago

I agree with you.

FYI: My workaround is allow_any_instance_of(RSpec::Rails::Matchers::ActiveJob::Base).to receive(:detect_args_signature_mismatch).and_return(nil)

odlp commented 2 months ago

Sorry to hear the signature-match check is causing issues for you both – adding an escape hatch sounds reasonable.

FWIW one can also store arbitrary data on an ActiveJob job by overriding #serialize and #deserialize to store/retrieve an extra value on the job payload hash. This would allow you to avoid adding then deleting a job argument, and allow the signature-match check to pass.

So in the example above you'd override #serialize to store the actor, #deserialize to populate an @actor instance variable, which can then be referenced in your around_perform hook...

https://github.com/rails/rails/blob/a5d1e10449c60f731a37298ba1912dbd58b1a9c4/activejob/lib/active_job/core.rb#L105-L162

smathieu commented 2 months ago

Overriding #serialize is a much cleaner implementation and we'll switch over to that.

Given this is an edge cases and there are documented workaround in this ticket, I think we can close this.

JonRowe commented 2 months ago

I'm leaving this open because I think this should respect the existing configuration for verifying doubles, we should check to see if we should verify arguments.

davidenglishmusic commented 1 month ago

I am likely experiencing a similar issue. I have a test against a mailer where I am checking the queue:

expect { mailer.deliver_later }.to have_enqueued_job.on_queue('low_priority')

It fails with the following message:

Incorrect arguments passed to ActionMailer::MailDeliveryJob: Missing required keyword arguments: args

Deep down in the rspec-support gem, I found that check seems be comparing an array of strings with an array of symbols: https://github.com/rspec/rspec-support/blob/main/lib/rspec/support/method_signature_verifier.rb#L74

[:args] - ["args", "_aj_ruby2_keywords"] #=> [:args]

If I change the following line to convert to symbols https://github.com/rspec/rspec-support/blob/main/lib/rspec/support/method_signature_verifier.rb#L376

args.pop.keys.map(&:to_sym)

it fixes it but then I get an invalid keyword error:

Incorrect arguments passed to ActionMailer::MailDeliveryJob: Invalid keyword arguments provided: _aj_ruby2_keywords

Should that be an allowed argument?

I am running: ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [arm64-darwin24] Rails 7.0.8.5

pirj commented 1 month ago

Hey @davidenglishmusic . I wonder if you could use a more specialized have_enqueued_mail matcher?

davidenglishmusic commented 1 month ago

Thanks @pirj - switching expect { mailer.deliver_later }.to have_enqueued_job.on_queue('low_priority') to expect { mailer.deliver_later }.to have_enqueued_mail(described_class).on_queue('low_priority') worked.

odlp commented 1 month ago

I'm leaving this open because I think this should respect the existing configuration for verifying doubles, we should check to see if we should verify arguments.

I've taken a first pass at this over in #2808. If verify_partial_doubles is configured to be false, or #without_partial_double_verification is being used then we'll skip the signature matching check.