Potentially unintentional breaking change in 4.0.2 with Symbol in argument matchers? (Part 2)

[n-rodriguez]

Hi there! It's broken when using ActiveJob:

      context "when expected arguments include symbols" do
        let(:active_job) { create_active_job }
        let(:worker_args) { [:foo, {bar: :baz}] }
        it "returns true" do
          active_job.perform_later(*worker_args) # because ActiveJob allows it and manage the Symbol serialization/deserialization
          expect(Sidekiq::Worker).to have_enqueued_sidekiq_job(*worker_args)
        end
      end

  1) RSpec::Sidekiq::Matchers::HaveEnqueuedSidekiqJob expected usage ActiveJob when expected arguments include symbols returns true
     Failure/Error: expect(Sidekiq::Worker).to have_enqueued_sidekiq_job(*worker_args)

       expected to have an enqueued Sidekiq::Job job
         with arguments:
           -["foo", {"bar"=>"baz"}]
       but have enqueued only jobs
         -JID:ca5afd696d61239a5dd2eb95 with arguments:
           -[:foo, {:bar=>:baz}]

It was working in 4.0.0 and no longer works in 4.0.2

See:

nicolas@MBP-de-Nicolas:~/PROJECTS/GITHUB/FORKS/foo/rspec-sidekiq$ git status
HEAD détachée sur v4.0.0
Modifications qui ne seront pas validées :
  (utilisez "git add/rm <fichier>..." pour mettre à jour ce qui sera validé)
  (utilisez "git restore <fichier>..." pour annuler les modifications dans le répertoire de travail)
    supprimé :        .ruby-version
    modifié :         spec/rspec/sidekiq/matchers/have_enqueued_sidekiq_job_spec.rb

aucune modification n'a été ajoutée à la validation (utilisez "git add" ou "git commit -a")
nicolas@MBP-de-Nicolas:~/PROJECTS/GITHUB/FORKS/foo/rspec-sidekiq$ bundle exec rspec
[rspec-sidekiq] WARNING! Sidekiq will *NOT* process jobs in this environment. See https://github.com/wspurgin/rspec-sidekiq/wiki/FAQ-&-Troubleshooting
..2023-08-25T23:11:00.594Z pid=78113 tid=1qf1 INFO: Sidekiq 7.1.2 connecting to Redis with options {:size=>10, :pool_name=>"internal", :url=>nil}
....................................................................................................Here!
OK!
........................................

Finished in 0.13985 seconds (files took 0.43229 seconds to load)
142 examples, 0 failures

Originally posted by @n-rodriguez in https://github.com/wspurgin/rspec-sidekiq/issues/203#issuecomment-1694017026

[n-rodriguez]

After some digging I think we should revert normalize_arguments to jsonified_expected_arguments.

Actually there are 2 uses cases : one with ActiveJob and one with Sidekiq::Job.

As you already know Sidekiq::Job use pure JSON serialization (https://github.com/sidekiq/sidekiq/wiki/Best-Practices#1-make-your-job-parameters-small-and-simple) whereas ActiveJob allows Symbol serialization (and others) with a set of custom Rails serializers (https://github.com/rails/rails/blob/main/activejob/lib/active_job/serializers.rb).

IMHO the error encountered by @jarthod in the first place (https://github.com/wspurgin/rspec-sidekiq/issues/203#issue-1857882260) was actually right (if my supposition is right, @jarthod use Sidekiq::Job)

 expected to have an enqueued WebhookWorker job
   with arguments:
     -["http://host.fr", [{:event=>"check.down", :time=>"2023-03-23T00:00:00Z"}]]
 but have enqueued only jobs
  -JID:75961eff3be0808eeeaf26f9 with arguments:
     -["http://host.fr", [{"event"=>"check.down", "time"=>"2023-03-23T00:00:00Z"}]]

Sidekiq::Job stores JSON data and deserialize them without using any specific deserializer, so you won't find any symbols in this payload. That's why it fails and it's actually a good thing 👍 Because the first assumption was false :

expect(WebhookWorker).to have_enqueued_sidekiq_job("http://host.fr", [{
  event: 'check.down',
  time: "2023-03-23T00:00:00Z"
}])

On the contrary, ActiveJob can deserialize symbols. You can see it in the payload :

  1) RSpec::Sidekiq::Matchers::HaveEnqueuedSidekiqJob expected usage ActiveJob when expected arguments include symbols returns true
     Failure/Error: expect(Sidekiq::Worker).to have_enqueued_sidekiq_job(*worker_args)

       expected to have an enqueued Sidekiq::Job job
         with arguments:
           -["foo", {"bar"=>"baz"}]
       but have enqueued only jobs
         -JID:ca5afd696d61239a5dd2eb95 with arguments:
           -[:foo, {:bar=>:baz}] # <-- HERE

Here the test fails because it's using normalize_arguments.

But the good new is : now you really test what you're giving to Sidekiq 😄

I've made a PR.

[jarthod]

IMHO the error encountered by @jarthod in the first place (https://github.com/wspurgin/rspec-sidekiq/issues/203#issue-1857882260) was actually right (if my supposition is right, @jarthod use Sidekiq::Job)

Yes, I never said it wasn't right by the way, just that it was an unintentional breaking change

After some digging I think we should revert normalize_arguments to jsonified_expected_arguments.

As discussed in #203 this jsonified_expected_arguments method is simply broken so it should not be reverted like this as in your PR. If the goal is to change the gem to support only raw arguments types, this method should probably be removed entirely to avoid keeping misleading complexity in the code.

[wspurgin]

Thanks for raising the behavior with ActiveJob @n-rodriguez

As @jarthod noted, the previous method simply didn't work and provided the expected args as they were.

The current behavior, which is just to support symbols in expected args isn't really something I'm willing to support past this major version.

I'm in favor of adding an option to RSpec's configuration, that will disable this recursive stringifcation altogether and treat the expected arguments as-is. If you want to open a PR that does that, I'd gladly review it

[n-rodriguez]

Actually I've changed my tests to only use have_enqueued_job and get rid of this gem. Thank you!

Edit:

to be more precise : use rspec-sidekiq if you use Sidekiq::Job, else use have_enqueued_job

[n-rodriguez]

BTW maybe we should mention in the README that rspec-sidekiq is for people who use Sidekiq::Job and redirect ActiveJob users to the Rails documentation. wdyt?

[wspurgin]

@n-rodriguez I might consider doing that when/if this gem drops all the custom stuff for Active Job for the next major version. I think at the time of this gem's inception, there wasn't as strong of support for Active Job matchers when the underlying thing was Sidekiq, but now that's not the case and the matchers from Rails work fine.