Support have_enqueued_sidekiq_job with no args

[3v0k4]

First of all, thanks for your work on rspec-sidekiq, it helped cleaning up our tests tremendously!

---

When I started using rspec-sidekiq, I assumed it would work similarly to the builtin matchers.

For example, the negation of raise_error works as follows:

expect { raise StandardError.new("arg") }.not_to raise_error(StandardError, 'arg') # expects StandardError with "arg"
expect { raise StandardError.new("arg") }.not_to raise_error(StandardError) # expects `StandardError`
expect { raise StandardError.new("arg") }.not_to raise_error # expects any exception

In the case of rspec-sidekiq, it's different:

expect(AwesomeJob).not_to have_enqueued_sidekiq_job # expects no jobs with no args (instead of any job)
expect(AwesomeJob).not_to have_enqueued_sidekiq_job("arg") # expects no jobs with "arg"

I believe the need for more documentation on this (603b9) stems from the confusing semantics.

It's especially important to provide loose matching for the negative case because it may give a sense of false confidence (I was bitten by this):

# Initially, I TDD the following code

expect(AwesomeJob).not_to have_enqueued_sidekiq_job

if false
  AwesomeJob.perform_async
end


# Later, I add some args to `AwesomeJob` and mess up the conditional

if true
  AwesomeJob.perform_async "arg"
end

# ⛔️ The test still passes (because it lacks `(any_args)` or `("arg")`)

I'd argue it's a better default to check for any jobs enqueued instead of jobs with no arguments.

This change applies to both expect(Job).not_to have_enqueued_sidekiq_job and expect(Job).to have_enqueued_sidekiq_job, but we could apply it only to the negative case if you prefer.

I'm happy to discuss alternatives if you think this is an issue but don't like the specific solution.

I guess in the longer term, enqueue_sidekiq_job and have_enqueued_sidekiq_job could have the same API:

enqueue_sidekiq_job == have_enqueued_sidekiq_job
enqueue_sidekiq_job.with("Awesome!") == have_enqueued_sidekiq_job.with("Awesome!")

[wspurgin]

👋 Thanks for the PR @3v0k4

In general, have_enqueued_sidekiq_job is wonky and is the old API. I personally recommend using the block syntax with enqueue_sidekiq_job as both the positive and negative cases are clearer.

I don't really want to modify the behavior of have_enqueued_sidekiq_job because 1) the goal is to deprecate it and move to just enqueue_sidekiq_job entirely and 2) while this behavior isn't great, it's what it has been for many years now.

The block syntax is better for exactly the reason you outlined above, it's more like the RSpec built-ins folks are used to:

expect { raise StandardError.new("arg") }.not_to raise_error # fails
expect { AwesomeJob.perform_async }.not_to enqueue_sidekiq_job # fails

Modifying the expected arguments coming into have_enqueued_sidkeiq_job this way will still lead to false positives. Imagine the scenario where you're testing that there really shouldn't be any arguments.

# should enqueue job without arguments
AwesomeJob.perform_async

# expects no argument
expect(AwesomeJob).to have_enqueue_sidekiq_job

# later an argument is added but maybe the job wasn't updated to handle it
AwesomeJob.peform_async "oh no"

# expectation still passes with the new `any_args` default
expect(AwesomeJob).to have_enqueue_sidekiq_job
# passes

While we could argue that default is "less bad", both are not great. That's why the documentation is as plain as possible about what the current default is and why the block syntax is preferred.

[3v0k4]

this way will still lead to false positives

Isn't it true also for enqueue_sidekiq_job?

def enqueue
  AwesomeJob.peform_async
end

expect { enqueue }.to enqueue_sidekiq_job # ✅

###

def enqueue
  AwesomeJob.peform_async "oh no"
end

expect { enqueue }.to enqueue_sidekiq_job # ✅

the goal is to deprecate it and move to just enqueue_sidekiq_job entirely

I would recommend against for a couple of reasons.

The block is evaluated after the expectation. Let's say I have a subject that creates a user and then schedules an email with that user's id:

expect { subject.call }.to enqueue_sidekiq_job(EmailWorker).with(User.last!.id)

User.last!.id will raise because subject did not run yet. Instead, the following works

subject.call

expect(EmailWorker).to have_enqueued_sidekiq_job(Organization.last!.id)

It's (IMO) sometimes cleaner to use have_enqueued_sidekiq_job.

Example testing multiple workers:

expect {
  expect {
    subject.call
  }.to enqueue_sidekiq_job(OneWorker)
}.to enqueue_sidekiq_job(OtherWorker)

# vs

subject.call

expect(OneWorker).to have_enqueued_sidekiq_job
expect(OtherWorker).to have_enqueued_sidekiq_job

Example testing multiple things:

expect { subject.call }.to enqueue_sidekiq_job(OneWorker)

expect(User.last.flag).to eq(true)

# vs

subject.call

expect(OneWorker).to have_enqueued_sidekiq_job
expect(User.last.flag).to eq(true)

You may argue those could be separate test examples, but I like that have_enqueued_sidekiq_job allows me to be flexible and put all the expectations at the end.

More in general, I think it makes sense to have both to satisfy the symmetry of enqueue_sidekiq_job/have_enqueued_sidekiq_job and receive/have_received.

[wspurgin]

🤔 All interesting perspectives and valuable feedback.

Quick points on multiple expectations (while it might be more of a preference thing) the enqueue matcher is composable:

expect do
  OneWorker.perform_async
  OtherWorker.perform_async
end.to enqueue_sidekiq_job(OneWorker).and enqueue_sidekiq_job(OtherWorker)

I hear you on the have_* usefulness even if I might argue the style of testing 😄 and the fact that it's similar to other built-ins (and even other libs like rspec-rails).

How about this as a compromise: The main thing that concerns me about the change is just that the current default has been the default. The behavior, even if less clear, is what it is, and updating this might break out from under folks without warning. I'd prefer that we have a period of time where we add a warning message for uses of have_enqueued_sidekiq_job without args will be changing to any_args style matching (like that of enqueue_sidekiq_job). We could then also have this behavior as opt-in?

Would you be open to that?

[3v0k4]

Good point on the composability! I always forget about that 🤦

Let me put together some code that incorporates what you suggested.

[3v0k4]

I propose we release the first commit of this PR as minor and the second one as major.

WDYT?

[wspurgin]

@3v0k4 Sounds reasonable. Are you okay with me cherry-picking your first commit onto main?

[3v0k4]

Sure. I can also open a separate PR if you prefer.

[wspurgin]

Let's do a separate PR so we can link discussion points from this PR into that so I have breadcrumbs to follow in the future 😄

[3v0k4]

I opened the separate PR.

If that works for you, I'll rebase onto main as soon as that one is merged and update this PR.

[3v0k4]

@wspurgin please give me a ping when you update the changelog on main and release the change from the other PR. I'll rebase and update CHANGES.md here.

[wspurgin]

Sorry @3v0k4 - had life come up in the last week that delayed me.

I've been piloting this with some friends and coworkers, and they suggested I add way to silence the warnings if they want to make use of the new default (rather than update all their specs to explicitly use any_args). As such I've added #217 to add that capability (also with this wiki page linked).

After testing that out with them today, I'll merge and release a minor version this week. If you have any thoughts on their LMK!

[wspurgin]

@3v0k4 4.2.0 released. Feel free to rebase and this will be part of 5.0 (along with me deprecating some ruby versions)

[3v0k4]

@wspurgin Rebased 🙏

[wspurgin]

Thanks again @3v0k4!

[pyromaniac]

Hello folks,

With this change, if I, say, prefer the have_enqueued_sidekiq_job matcher as it creates less indentation improving readability (the same way as have_received matcher as prefered over receive), how do I specify the negative case for a specific job?

Say, I need to test that some particular job has not been enqueued. Before I did expect(AwesomeJob).not_to have_enqueued_sidekiq_job and the AwesomeJob argument was respected. Now it is ignored and the matcher checks for any job enqueued.

How do I do it after v5 upgrade, please advice.

[wspurgin]

@pyromaniac negation is partially why we made this change, because most folks probably want "expect no job of [job class] to be enqueue". In v5 that is:

expect(AwesomeJob).not_to have_enqueue_sidekiq_job(any_args)

If however, you want to maintain the exact same behavior as ≤4, use:

expect(AwesomeJob).not_to have_enqueue_sidekiq_job(no_args)

⚠️ Caveat that the no_args above ☝️ means if you happen to enqueue a job with args, the test will pass.

[wspurgin]

Before I did expect(AwesomeJob).not_to have_enqueued_sidekiq_job and the AwesomeJob argument was respected. Now it is ignored and the matcher checks for any job enqueued.

@pyromaniac say more? It should still be respected in V5.

[pyromaniac]

@wspurgin hello,

I finally got to the bottom of this and you are right, everything works as you suggested. The new approach is way safer. Thanks!