Cop Idea: Force to use "allow to receive" with "with" to match arguments

[hubertjakubiak]

Example:

Good: allow(Job).to receive(:enqueue).with(a, b, c)
Bad: allow(Job).to receive(:enqueue)

to avoid scenarios when tests passed and there's incorrect number of arguments on production :D

[pirj]

What if enqueue does not accept any arguments?

This kind of goes against the recommendation from Effective testing with RSpec book to be as loose with expectations as possible. And this is not even an expectation, it's an allowance.

Do you have a better example in mind? What is the rationale behind this proposal?

[hubertjakubiak]

if no arguments then .with(no_args) should be used.

[hubertjakubiak]

Example:

Code in controller:

Job.enqueue(user_id)

Spec:

before do
  allow(Job).to receive(:enqueue)
end

it do
  expect(Job).to receive(:enqueue)
end

Result: Specs passed even if Job requires two arguments like Job.enqueue(user_id, bank_id).

[pirj]

I could recommend using enqueue_job rspec-rails matcher. Or does it have the same problem?
enqueue doesn't seem to be Active Job or Sidekiq. Where is it from?

Aside from this, why do you both allow and expect the same thing?
allow comes handy when you use have_received after performing the action.
expect(...).to receive expectation should be set before the action. With that in mind I'd argue with the original title.

[hubertjakubiak]

We use https://github.com/que-rb/que

before is necessary for tests when there is no the same expect.

before do
  allow(Job).to receive(:enqueue)
end

it do
  expect(Job).to receive(:enqueue)
end

it "something else" do
  ...
end

[pirj]

For the cop

In this case, insisting that receive has with should be sufficient for expect, it would detect signature mismatch already, won't it?

This boils down to a "test boundaries" recommendation.
What if your test would expect enqueue was called with arguments, but those arguments were not the ones the job expects? E.g.

class MyJob < Que::Job
  def run(credit_card_id, user_id)
...

expect(MyJob).to receive(:enqueue).with('4111 1111 1111 1111', user.id) # boom, job expects not a card number, but the card record id

but the spec would pass.

There's one more thing to keep in mind. with can be passed to receive several times, one for each subsequent call of a stubbed method. The cop should insist on explicitly setting the number of expected calls in this case:

expect(Job).to receive(:enqueue).twice.with(...)
#  ^^^ Add a `with` expectation constraint for the second call to `enqueue`

I can't remember off the top of my head how RSpec would constrain the arguments of a second call, would it use the same constraints from with, or would it let calling with any arguments.
And, to my best recollection there are numerous issues in there, see rspec/rspec-mocks#1426

Do you want to shave this yak?

For your specific case

Que's testing doc section states:

You may want to set Que::Job.run_synchronously = true, which will cause JobClass.enqueue to simply execute the job's logic synchronously, as if you'd run JobClass.run(*your_args). Or, you may want to leave it disabled so you can assert on the job state once they are stored in the database.

Is run_synchronously turned on in your spec helper?

There's also rspec-que which seams to let enqueue to run, and checks the database for enqueued jobs. To me, it seems to be a reasonable approach to testing controllers.

[hubertjakubiak]

In this case, insisting that receive has with should be sufficient for expect, it would detect signature mismatch already, won't it?

It was not enough in our case. expect was written to receive two arguments, background job required three arguments, but spec passed because of allow(Job).to receive(:enqueue).

[pirj]

expect was written to receive two arguments, background job required three arguments, but spec passed because of allow(Job).to receive(:enqueue).

Would adding with with two arguments help you to detect the problem?

[hubertjakubiak]

for allow or expect?

[pirj]

Would adding with with two arguments to allow help you to detect the problem?

[pirj]

Ok, let's start from the very beginning.
Can you please clarify your case?

class A
  def self.three_args(a, b, c)
  end
end

RSpec.describe A do
  context 'when allow has no `with` qualifier' do
    before { allow(A).to receive(:three_args) }

    it 'fails to detect the problem with an incorrect number of arguments passed to the method call' do
      expect(A).to receive(:three_args).with(1, 2)
      A.three_args(1, 2)
    end
  end
end

results in:

Failures:

  1) A when allow has no `with` qualifier fails to detect the problem with an incorrect number of arguments passed to the method call
     Failure/Error: expect(A).to receive(:three_args).with(1, 2)
       Wrong number of arguments. Expected 3, got 2.

I believe this is due to verify_partial_doubles configuration option. Is it on in your spec_helper.rb?

  config.mock_with :rspec do |mocks|
    mocks.verify_partial_doubles = true
  end

I highly recommend turning it on. It will be turned on by default in RSpec 4.

Full example with verify_partial_doubles turned off:

class A
  def self.three_args(a, b, c)
  end
end

RSpec.describe A do
  context 'when allow has no `with` qualifier' do
    before do
      allow(A).to receive(:three_args)
    end

    it 'fails to detect the problem with an incorrect number of arguments passed to the method call' do
      expect(A).to receive(:three_args).with(1, 2)
      A.three_args(1, 2)
    end
  end

  context 'when allow has a comparably incorrect `with` qualifier' do
    before do
      allow(A).to receive(:three_args)
        .with(1, 2) # Here it is what the cop would recommend adding. And it does not solve the problem
    end

    it 'fails to detect the problem with an incorrect number of arguments passed to the method call' do
      expect(A).to receive(:three_args).with(1, 2)
      A.three_args(1, 2)
    end
  end
end

2 examples, 0 failures

💥 cop is useless. Use verify_partial_doubles.

[pirj]

I'm not convinced this will make a useful cop, at least for what it was originally purposed to inspect.
Thanks for understanding.