Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify ActiveJob job signature matches arguments #2745

Merged
merged 4 commits into from Apr 10, 2024

Conversation

odlp
Copy link
Contributor

@odlp odlp commented Mar 15, 2024

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
  • Current outcome: Spec passes
  • Desired outcome: ArgumentError: Missing required keyword arguments: a, b

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.

Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, I'd love to see the mailer implementation included in this as well.

@odlp odlp force-pushed the pr-verify-job-perform-signature branch from f1bbb84 to ae305b0 Compare March 19, 2024 11:50
@odlp
Copy link
Contributor Author

odlp commented Mar 19, 2024

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
Copy link
Member

JonRowe commented Mar 19, 2024

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
Copy link
Contributor Author

odlp commented Mar 20, 2024

@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

Guards against false-positives where both the implementation
and have_enqueued_job or have_been_enqueued matchers are passed
the wrong arguments which don't match the job's signature.

Prior to this change it was possible for the implmentation and
spec to pass whilst expecting the wrong number of arguments,
or mixing up the positional/keyword arguments. This resulted
in a passing test and the implementation failing at runtime.
Guards against false-positives where both the implementation
and have_enqueued_mail matchers are passed the wrong arguments
which don't match the mailer's signature.
Preferable for a matcher to fail instead of directly raising
ArgumentError when a mismatch is detected.

#2745 (comment)
@JonRowe JonRowe force-pushed the pr-verify-job-perform-signature branch from 232ea6b to d3bbe58 Compare April 10, 2024 07:44
@JonRowe JonRowe merged commit 08e210f into rspec:main Apr 10, 2024
17 checks passed
JonRowe added a commit that referenced this pull request Apr 10, 2024
@JonRowe
Copy link
Member

JonRowe commented Apr 10, 2024

Thanks so much for this

@odlp
Copy link
Contributor Author

odlp commented Apr 10, 2024

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.

@odlp odlp deleted the pr-verify-job-perform-signature branch April 10, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants