wspurgin / rspec-sidekiq

RSpec for Sidekiq
https://github.com/wspurgin/rspec-sidekiq
Other
651 stars 133 forks source link

`be_delayed` without expected_arguments never matches #178

Closed mcrampon closed 11 months ago

mcrampon commented 2 years ago

From what I can see, this is due to how the jobs are found:

        def find_job(method, arguments, &block)
          job = (::Sidekiq::Extensions::DelayedClass.jobs + ::Sidekiq::Extensions::DelayedModel.jobs + ::Sidekiq::Extensions::DelayedMailer.jobs).find do |job|
            yaml = YAML.load(job['args'].first)
            @expected_method_receiver == yaml[0] && @expected_method.name == yaml[1] && (@expected_arguments <=> yaml[2]) == 0
          end

          yield job if block && job
        end

If no expected arguments are given, (@expected_arguments <=> yaml[2]) == 0 will always be false (for jobs that have been enqueued with arguments).

(Also, I'm not sure why there is a spaceship operator here instead of a simple ==, and why method and arguments are passed to find_job but never used, but that's another topic).

In short, this comparison should be performed only if expected_arguments are given.

That is, if you want this to be coherent with the rest of rspec. Otherwise, you can dismiss this issue :shrug:

ccyrille commented 2 years ago

To complete this issue, the most problematic / counter-intuitive seems :

# foo.rb
class Foo
  def call
    self.delay_for(1.minute).bar("hello")
  end

  def self.bar(arg)
     puts arg
  end
end

# foo_spec.rb
describe Foo do
  it "should not delay :bar" do
     expect(Foo.method :bar).to_not be_delayed
     Foo.new.call  # this will pass
  end
end

This is pretty misleading. I see two possible fix for that :

  1. Make be_delayed ignore the arguments check if no argument is passed to it (it will match the related job whatever the arguments). But I am not sure of the potential impacts.
  2. Allow at least to do something like be_delayed(any_args) using the capabilities of rspec-mock.

@packrat386 @coding-bunny what do you think ?

coding-bunny commented 2 years ago

Yeah, this feels misleading to me, as based on the code snippet, the test is supposed to fail since we are delaying for 1 minute.

The initial idea to only perform the argument check if arguments are actually passed feels good to me. Just not 100% sure if it is true in all cases, but I suppose testing/usage will reveal that.

teckwan commented 2 years ago

Hi @coding-bunny, I've submitted a PR to fix this issue. I've went with ignoring the argument check if non are passed as the solution.

coding-bunny commented 2 years ago

cc @philostler @packrat386