1

I've been writing tests with instance_doubles to stand in for message chains when I need more granularity in the midst of the chain. But, I'm wondering if I'm doing things the hard way.

Here's the method I want to test:

def run_produceable_job
  # Delete any jobs that exist, but haven't started, in favor of this new job
  Delayed::Job.where(queue: 'produceable', locked_at: nil).delete_all

  ProduceableJob.perform_later
end

For the Delayed::Job call, it's important I check that the queue name is as expected. I also want to make sure that Delayed::Job is receiving .delete_all at the end

I would like to do something like this:

expect(Delayed::Job).to receive(:where).with(queue: 'produceable', locked_at: nil).and_then_receive(:delete_all)
                                                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Does RSpec offer some sort of chaining for receive? I've skimmed the docs, but can't find anything that specifically talks about adding multiple receives.

Or am I going to have to do it the long way?

ar_relation = instance_double ActiveRecord::Relation
allow(Delayed::Job).to receive(:where).with(queue: 'produceable', locked_at: nil).and_return(ar_relation)
allow(ar_relation).to receive(:delete_all)

expect(Delayed::Job).to receive(:where).with(queue: 'produceable', locked_at: nil)
expect(ar_relation).to receive(:delete_all)
Chiperific
  • 4,428
  • 3
  • 21
  • 41
  • 4
    IMHO you have to go the long way. Regardless of that, I would recommend you overthink your testing strategy. At the moment you test that a very specific combination of methods is called but not if these method calls are actually doing what you want. Instead, I would create an example record that should be deleted (and perhaps a couple that should not be deleted), then run the job and afterward test that only the expected record was deleted. The rule of thumb is to test the expected outcome, not the specific implementation because the implementation might change or break in future versions. – spickermann Nov 25 '21 at 05:09
  • 3
    ^ A main reason for writing tests is that it gives us the safety to refactor our code. But if your test enforces a specific implementation, it becomes worthless for refactoring purposes – changing the code means breaking the test. – Stefan Nov 25 '21 at 08:36
  • 1
    @spickermann You should add this as an answer :) – Holger Just Nov 25 '21 at 12:07

1 Answers1

3

IMHO you have to go the long way. There is no shorter way to describe it.

Regardless of that, I would recommend you overthink your testing strategy. At the moment you test that a very specific combination of methods is called but not if these method calls are actually doing what you want them to do.

Instead, I would create an example record that should be deleted (and perhaps a couple that should not be deleted), then run the job and afterward test that only the expected record was deleted.

For example like this:

let!(:record_to_be_deleted) { 
  Delayed::Job.create!(queue: 'produceable', locked_at: nil) 
}
let!(:records_to_stay) do
  [ 
    Delayed::Job.create!(queue: 'produceable', locked_at: Time.current),
    Delayed::Job.create!(queue: 'default', locked_at: nil)
  ]
end

it "should remove only expected records" do
  expect {
    instance.run_produceable_job
  }.to chance { Delayed::Job.count }.from(3).to(2)

  expect { 
    record_to_be_deleted.reload
  }.to raise_error(ActiveRecord::RecordNotFound)
end

The rule of thumb is to test the expected outcome, not the specific implementation. Because the implementation might change, will be refactored or might break in future versions.

spickermann
  • 100,941
  • 9
  • 101
  • 131
  • Thanks, to both you and @stefan. I see where you're coming from and I've written tests like this in the past (much slower tests because records are created, not stubbed). But, I've also heard it said to keep your tests specific to scope and concern. That you should stub or mock everything outside. E.g. I shouldn't test Delayed::Job's abilities within this job. That should be tested by a system spec. Is this just a difference of opinion? – Chiperific Nov 26 '21 at 13:56