Batch callbacks don't fire properly when testing

[mperham]

The example code in the wiki doesn't work properly:

class SomeCallback
  def on_success(status, options)
    $count += 1
  end
end
$count = 0
Sidekiq::Testing.server_middleware.add Sidekiq::Batch::Server
Sidekiq::Testing.inline! do
  b = Sidekiq::Batch.new
  b.on(:success, SomeCallback)
  b.jobs do
    5.times { HardWorker.perform_async }
  end
end
assert_equal 1, $count

When inline, each job is pushed through the middleware and run, meaning the batch thinks it has a single job and runs the callbacks after the first job, not the fifth.

Currently I don't see any way to fix this, we may need to backpedal on the ability to test batches like this.

[ryansch]

Agreed, the testing harness for middleware only works for middleware that doesn't need the job stuff in redis. We use it to test things specific to our application. I'd still like to take a crack at a 'real' harness for batches at some point.

[mperham]

No plans to fix. The batch feature requires a lot of system integration; I believe it's a smell for tests to use it. I'll rework the wiki docs.

[lefty313]

@mperham

I believe it's a smell for tests to use it. I'll rework the wiki docs.

what about case where you want to test full integration of worker which is internally using batch.

[mperham]

@lefty313 You can call the batch api in the worker without issue. If you want the entire pipeline of async operations to execute to verify a workflow, that's a staging environment.

[lefty313]

If you want the entire pipeline of async operations to execute to verify a workflow, that's a staging environment.

The whole point of using batches is building workflow with it isn't it ? Currently callback is fired after first job so I can't test everything properly.

Simplest example ever would be

• In first batch fetch 100 facebook posts
• In complete callback generate report for it

class FacebookPostReport
  def perform(...)
    batch = Sidekiq::Batch.new
    batch.on(:complete, "FacebookPostReport#synced_posts")
    batch.jobs do
      FetchFacebookPostWorker.perform_async(...)
    end
  end

  def synced_posts(status, options)
    # generate report
  end
end

## in spec

Sidekiq::Testing.server_middleware.add Sidekiq::Batch::Server
Sidekiq::Testing.inline! do
  user = ...
  FacebookPostReport.perform_async(...)
  # this will break
  expect( user.report.posts_count ).to eq 100
  # this not
  expect( user.report.posts_count ).to eq 1
end

in test environment report will always include just 1 post which is really unfortunate.

I will contact with you through support email to not spam here.

[mperham]

@lefty313 I don't support automated testing batches in-process. You can test your worker code, you can test your callback code. You cannot test the entire workflow. People have hacked together solutions but nothing that I want to support long-term.

[lefty313]

People have hacked together solutions but nothing that I want to support long-term.

Could you post some references ? 👍

[mperham]

I don't have any links, just a hazy memory of someone mentioning it. Maybe @ryansch.

[victormours]

I've been in the same situation where I wanted to do some end to end testing on a batch and its callbacks, and I found that using Sidekiq::BatchSet.new.first.callbacks in my tests goes a long way.

[airhorns]

I would love to add another vote for making this kind of testing possible (from a sidekiq ent customer). A staging environment is a manual way to do integration testing and right now Sidekiq pushes me to never automate that integration test because of this. If we have a big team working with batches forcing them all to do this is really annoying. Any way to get an endorsed or supported way of testing?

[ryansch]

We do a lot of this sort of design pattern to work around this in our app:

class DoTheThing
  include Metaractor

  def call
    batch = Sidekiq::Batch.new
    batch.on(
      :success,
      DoTheThing,
      thing_args
    )
    batch.jobs do
      DoAPieceOfTheThing.perform_async
    end

    if Rails.env.test? && Sidekiq::Testing.inline?
      on_success('status', success_args)
    end
  end

  def on_success(status)
    # winning
  end
end

We're able to work through some fairly complex nested batches this way without issue.

[airhorns]

Yeah but every developer has to remember to get that right, be aware of why this is necessary and endure the cognitive overhead of blending in that test code to the implementation. If it's possible to do like this why isn't it possible to do in Sidekiq itself?

[lefty313]

@ryansch I would not deploy code like that to production just to workaround test env issue

ps: if you really want to write test with full integration you can spawn real sidekiq process

[ryansch]

I agree that it would be far better to have sidekiq do that for us but this approach has worked very well for us for a few years now in lieu of an official solution.

I simply haven't had time personally to add this to sidekiq.

[benoittgt]

I'm also interested in the ability to test full flow. In my case it's three steps in series (A->B->C). I dig few hours without success.

[mperham]

I continue to believe that a batch test is an integration test. It's not appropriate for unit testing. You can test all the pieces individually and you can create a super test which scripts all the pieces executing but batches really should execute within the server context to get full assurance that they work. Executing batches within the test environment would give a false sense of confidence.

[airhorns]

batches really should execute within the server context to get full assurance that they work.

Couldn't you say the same thing about any unit? What makes batches special such that they strictly need to be tested in staging, or that they're particularly false-confidence inducing?

[mperham]

It's my judgment around the amount of moving parts and amount of data (including serialization/deserialization) moving in and out of process. There's too many things which can fail which a test won't catch.

If anyone comes up with a solution they think is useful to all Sidekiq Pro users, I'm happy to review.

[sdhull]

The idea that batches (in their complete form) are untestable does not sit well with me.

If anyone comes up with a solution they think is useful to all Sidekiq Pro users, I'm happy to review

I'm probably missing something important here (it is after midnight after all 🥴), but I added this to the before block in the spec file I'm working on:

    Sidekiq::Testing.server_middleware do |chain|
      chain.add Sidekiq::Batch::Server
    end

And batch callbacks seem to be working in my spec now! I'm especially surprised since as far as I can tell, there is no concept of client middleware in sidekiq/testing, and that's what "registers" the jobs as belonging to the batch (I think?). And I'm not using the inline option in this case (I see special logic in Sidekiq::Batch for inline).

We're on sidekiq (6.5.8) and sidekiq-pro (5.5.8) if that helps any others who stumble on this. And fwiw I'm executing jobs with a poorly-named spec helper:

  def work_jobs_recursively
    # while not technically recursive, it's a similar idea
    while Sidekiq::Extensions::DelayedMailer.jobs.any? || Sidekiq::Worker.jobs.any?
      Sidekiq::Extensions::DelayedMailer.drain
      Sidekiq::Worker.drain_all
    end
  end

(I initially wrote it as a recursive method that called itself if Sidekiq::Worker.jobs.any? but then realized I could avoid the extra frames with a simple while loop)

[mperham]

Yeah, S::Testing.server_middleware was added a long time ago but forgot to document its usage with S::Batch. I've updated the wiki.

https://github.com/sidekiq/sidekiq/wiki/Batches#testing

[sobrinho]

@mperham the statement on Wiki doesn't seem accurate since the callbacks are fired after the first job, not after the second one.

# x.rb
$perform_count = 0
$on_success_count = 0
$on_complete_count = 0

Sidekiq::Testing.server_middleware do |chain|
  chain.add Sidekiq::Batch::Server
end

class HardWorker
  include Sidekiq::Worker

  def perform
    $perform_count += 1
    puts "perform##{$perform_count}"
  end
end

class SomeCallback
  def on_success(status, options)
    $on_success_count += 1
    puts "on_success##{$on_success_count}"
  end

  def on_complete(status, options)
    $on_complete_count += 1
    puts "on_complete##{$on_complete_count}"
  end
end

Sidekiq::Testing.inline!

b = Sidekiq::Batch.new
b.on(:success, SomeCallback)
b.on(:complete, SomeCallback)
b.jobs do
  HardWorker.perform_async
  HardWorker.perform_async
end

Outputs:

# RAILS_ENV=test bin/rails r x.rb
perform#1
on_complete#1
on_success#1
perform#2

[sobrinho]

How do you feel about having a testing mode like Sidekiq::Testing.integrated! which would actually call everything integrated on the specs?

Like an embedded Sidekiq.

[sobrinho]

Of course that would make the test a integrated spec instead of a unit test, but that's what we are looking for!

[sobrinho]

First call to integrated! (or another name) boots the embedded, we can keep it running but only enqueue and process jobs there while in the integrated/embedded testing mode.

[mperham]

perform#1
on_complete#1
on_success#1
perform#2

This is fixed with the latest versions.

[sobrinho]

Awesome, thanks! You are the best! ❤️