Fixup have_enqueued_job matcher on job retries

[ojab]

Previously we were checking only job counts, so if one job was
performed and one job was added - matcher failed. Check by unique
id (#hash) instead.
We cannot use job['job_id'] here, because job retains job_id on retry.

This works only with rails >= 6.1, because retry_on was introduced in 6.0 and apparently test queue adapter is broken for retries in 6.0: it adds executed job back with added exception_executions along with actual retried job. So dunno how to proceed here, please advise /shrug

Test CI run: https://github.com/ojab/rspec-rails/runs/5183906574?check_suite_focus=true

Also:
fixes #2668

[ojab]

hmm.. Actually looking @ CI, 6.0 is green, so probably failure is rails-6.0 <-> ruby-3.1 compat locally. And 5.2 fails with unknown keyword queue. so retry_on is there.
I'll check tomorrow and I guess would get green CI without any workarounds 😅

[ojab]

It became more complicated due to rails-6.0 pushing jobs with the same #hash to the queue, but CI is green https://github.com/ojab/rspec-rails/runs/5198727860?check_suite_focus=true
Cleaned up, still two additional commits: first fixing rspec dependencies version in gemspec, second restricting rails-5.2 version for ruby-2.2 compat (filled rails/rails#44435 about it).

[JonRowe]

We've merged build fixes for Rails and updated the minimum versions so this should be ok to be rebased without your ci changes

[ojab]

🙇 done

[pirj]

Thanks!

[ojab]

😿

[pirj]

It appears to me that storing by the hash key is not necessary.
I've added a couple more jobs of the same kind, and they have a different job_id assigned, resulting in a different hash.

As a result, count is always a 1.

Below, a -= 1 and a further .positive? with Array.new and a compact is basically subtraction of previously enqueued jobs from all enqueued jobs.

I dared pushing a commit that compares with a Set#-.

Can you write a failing spec to prove this approach is insufficient?

[pirj]

Needs a test that would fail without changes, so that changes would make it green.

[pirj]

The previous implementation was:

1. Save the number of currently enqueued jobs
2. Run the block passed to expect { ... } that can:

• perform previously scheduled jobs and remove them from enqueued
• enqueue more jobs

3. Remove the number of previously enqueued jobs from the list of currently enqueued jobs

Of course, since the matcher is scoped to one specific job class, it's quite an edge case.

This (previous, before the PR's) implementation was not considering jobs that were at the bottom of the list, and moved to the top of the list, e.g.:

- job with args [1]
- job with args [2]
+ job with args [1]
+ job with args [2]

And expect { ... }.to have_enqueued_jobs would fail because it would think that no jobs were (re-)enqueued.

Equally, if the order of jobs would change, or if some jobs succeed, or if more jobs succeed than fail and a re-scheduled, the expectation would fail.

I believe we can leave some of those cases for follow-up improvements, but one I'd love to see is:

# some setup
expect { ... }.to have_enqueued_job(MyJob)

that would fail if we just compare the counts of matching jobs.

It is unfair to do so with counts because yes, one job was performed, and one enqueued.
If you scope it to the :retry queue, the expectation would pass. Because there were no jobs on that queue, and now there's one. But this is unfair, too, as it works fine with the previous count-only implementation, too.

How about an example with a job that was on :retry queue already and got re-enqueued again?

[JonRowe]

This looks fine to me, but I defer to @pirj as we do want a failing spec this fixes :)

[pirj]

Previously we were checking only job counts, so if one job was performed and one job was added - matcher failed.

This is an incorrect assumption.

The "before" for the matcher is everything prior to what is inside the expect { ... } block.
I am not aware of an option to perform jobs with the test adapter that were enqueued previously.
queue_adapter.perform_enqueued_jobs = true and perform_enqueued_jobs { ... } do not perform previously enqueued jobs.

So if a job is enqueued, performed and schedules itself all inside expect { ... }, all previously enqueued jobs are out of equation with

in_block_jobs = queue_adapter.enqueued_jobs.drop(original_enqueued_jobs_count)

and all the jobs that are left enqueued (e.g. when perform_enqueued_jobs = true but perform_enqueued_at_jobs = false) like in the case with job retries, are still detected. This is what your newly added example shows, as it is passing just fine with the current implementation.

Even though I'd love to merge my shiny refactoring involving Set that considers if the same job gets re-enqueued, I have no good idea how it can affect a real-world spec.

I failed to write an example that would break the existing implementation, and with this I'm more inclined to close this PR.

Please do not hesitate to reopen if you can write a failing spec.

[pirj]

Actually, a spec might be possible to write with a direct non-block usage of perform_enqueued_jobs as here.

[pirj]

@ojab Does this look correct to you?
Please accept my apologies for nitpicking and ignorance of the block-less form of perform_enqueued_jobs that was needed to reproduce the issue.

[ojab]

@pirj oh, right, sorry for bogus spec. I'll update the PR tomorrow to use non-block form and will check that it fails properly in master.

[ojab]

Checked the commit where monkeypatch was introduced into local codebase, it's

retried_job.perform_later

expect  { perform_enqueued_jobs }.to 

so yeah, spec is indeed doesn't mirror what was broken 🤦
Changed/verified that it breaks with rails-6.1 on main and works with changes in this PR/pushed.

[pirj]

Which monkey patch?

How does this work?
We set the adapter to perform jobs, then schedule a job.
The job should immediately execute, fail and be re-scheduled for later, right?
What does the second perform_enqueued_jobs do, performs it once again, and then it is re-scheduled once again?

        queue_adapter.perform_enqueued_jobs = true
      end

      it "passes with reenqueued job" do
        time = Time.current.change(usec: 0)

        retried_job.perform_later

        travel_to time do
          expect  { perform_enqueued_jobs }.to

I strongly prefer the solution with Set, as it's significantly simpler.
Does it solve the same issue?

[pirj]

Thank you, @ojab !
I went with my implementation approach for the fix. If you feel that yours is superior, please don't hesitate to open a PR with a failing spec.

[JonRowe]

Released in 6.0.2