DEV: Run jobs sequentially in test mode (PR #9897)

When running jobs in tests, we use Jobs.run_immediately!. This means that jobs are run synchronously when they are enqueued. Jobs sometimes enqueue other jobs, which are also executed synchronously. This means that the outermost job will block until the inner jobs have finished executing. In some cases (e.g. process_post with hotlinked images) this can lead to a deadlock.

This commit changes the behavior slightly. Now we will never run jobs inside other jobs. Instead, we will queue them up and run them sequentially in the order they were enqueued. As a whole, they are still executed synchronously. Consider the example

class Jobs::InnerJob < Jobs::Base
  def execute(args)
    puts "Running inner job"
  end
end

class Jobs::OuterJob < Jobs::Base
  def execute(args)
    puts "Starting outer job"
    Jobs.enqueue(:inner_job)
    puts "Finished outer job"
  end
end

Jobs.enqueue(:outer_job)
puts "All jobs complete"

The old behavior would result in:

Starting outer job
Running inner job
Finished outer job
All jobs complete

The new behavior will result in:

Starting outer job
Finished outer job
Running inner job
All jobs complete

GitHub

Did you check out if this affects the performance of tests? The original reason we ran jobs this way was because at a previous job of mine we did not and it was PAINFULLY slow.

What specifically was different about how it was done at your old job? I don’t think this change should have any impact on performance. We still do the same amount of work.

This PR is a very tiny change to the behaviour. Jobs are still run synchronously, and this still only applies if you explicitly Jobs.run_immediately!. The only difference is the execution order - an example showing that is in the description above.

@davidtaylorhq it had to do with many more jobs being queued rather than simply run. In this case there is only a tiny overhead so it might be similar. Just something to keep in mind!

That makes sense, thanks @eviltrout. Confirming that it doesn’t look like it has had an effect on run times. I ran a few times locally and couldn’t see any difference. And GitHub runs are no worse. This is a recent run on master:

Screenshot 2020-05-27 at 19 09 32

And on this PR:

Screenshot 2020-05-27 at 19 09 35

@davidtaylorhq thank you for doing this :clap: this frustrated me a lot when trying to test PullHotlinkedImages a while back but I didn’t know what the solution was.

I wonder if extending enqueue to accept a opts[:klass] would be better here. Less playing around with declaring constants and removing them feels like a cleaner approach to me.

I like this idea - much cleaner. I allowed passing a class in the job parameter rather than adding a new option (since the job parameter is required, and I didn’t want to change the method signature). I updated the specs to use anonymous class definitions