rspec / rspec-rails

RSpec for Rails 6+
https://rspec.info
MIT License
5.14k stars 1.03k forks source link

Verify ActiveJob job signature matches arguments #2745

Closed odlp closed 2 months ago

odlp commented 3 months ago

Hello,

I'd like to suggest an improvement to the have_enqueued_job and have_been_enqueued matchers – ensuring the arguments passed by the subject/caller actually match the job's method signature.

Motivation

At the moment it's possible to write an implementation and spec which calls & expects the wrong number of arguments, or mix up the positional/keyword arguments. This results in a passing test, but the implementation will fail at runtime.

This improvement would guard against these false-positives. This approach was inspired by RSpec's awesome verifying doubles.

Example

class KeywordArgsJob < ActiveJob::Base
  def perform(a:, b:) # expecting keyword arguments
  end
end

RSpec.describe "ensuring job arguments match the job signature" do
  it "throws an exception on mismatch" do
    expect { KeywordArgsJob.perform_later(1, 2) } # incorrectly passing positional args
      .to have_enqueued_job(KeywordArgsJob).with(1, 2) # incorrectly expecting positional args
  end
end

Notes / questions

I wasn't sure if this behaviour should initially be opt-in, then become the default in the next major version. For the initial PR I've not made the verifying arguments behaviour configurable because it seems like any resulting failures will always be underlying bugs which should be fixed.

odlp commented 3 months ago

Hmm... I think the CI failures are unrelated flakes:

expected that command "bin/rspec spec/system/widget_system_spec.rb" has exit status of "0", but has "9". (RSpec::Expectations::ExpectationNotMetError)
features/system_specs/system_specs.feature:109:in `the exit status should be 0'

Matrix is passing on my fork: https://github.com/odlp/rspec-rails/actions/runs/8342458003

JonRowe commented 3 months ago

Thanks, I have one last thing I'd like to change which is I'm not sure that it should raise an ArgumentError, this is a matcher and I think it should fail the matcher instead

odlp commented 3 months ago

@JonRowe switched to the matcher failing in 232ea6b. Turns out this is much neater when it fails:

Failures:

  1) ActiveJob matchers throws an exception on mismatch
     Failure/Error:
       expect { KeywordArgsJob.perform_later(1, 2) } # incorrectly passing positional args
         .to have_enqueued_job(KeywordArgsJob).with(1, 2) # incorrectly expecting positional args

       Incorrect arguments passed to KeywordArgsJob: Missing required keyword arguments: a, b
     # ./spec/rspec/rails/matchers/active_job_spec.rb:51:in `block (2 levels) in <top (required)>'
     # ./spec/rspec/rails/matchers/active_job_spec.rb:46:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:74:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:68:in `block (2 levels) in <top (required)>'

Thanks for the suggestion

JonRowe commented 2 months ago

Thanks so much for this

odlp commented 2 months ago

It was a pleasure - glad to make a small contribution back to a tool I use almost every day. Thanks for you reviews & guidance @JonRowe.