leshill
expecting a Worker to have a queue size is broken.
Here is the pry session where I discovered the issue (worker names changed).
206: # TODO: I can't understand why there are 2 jobs.
207: # I've done everything I can think of to isolate the test to the single job I expect enqueued.
208: expect(OnlyOneLikeThis).to have_queue_size_of(2)
=> 209: binding.pry
210: expect(OnlyOneLikeThis).to have_queued(id: lead.id).in(:slow)
[1] pry(#<RSpec::ExampleGroups::...::OnlyOneLikeThis>)> ResqueSpec.queues
=> {"slow"=>[{:class=>"DoOtherStuff", :args=>[{:id=>1}]}, {:class=>"OnlyOneLikeThis", :args=>[{:id=>1}]}]}
There were 2 jobs total, only one of them was for the worker OnlyOneLikeThis, and my expectation was, I thought, also for that worker: expect(OnlyOneLikeThis).to have_queue_size_of(2), so I would have expected it to fail, since only one job was for that worker.
PR to come shortly.
Hi @pboling,
Thanks for the report and PR. Is the missing feature a matcher that matches only the jobs on a queue that have been enqueued by a given worker and not any other worker that may be using the same queue?
Well that's what I expected it to do, the test has little meaning if you count everything in the queue if your system backgrounds everything.
See the PR. I updated the code/specs for what I expected.
Could be made a separate feature.
Hi @pboling,
Yes, the confusion exists because the matcher allows referencing the queue by the worker’s class and by name. When referencing by the worker’s class, limiting the jobs to just that worker is certainly one way to view it.
However, this may be a significant breaking change for other users (or maybe not?)
One alternative is to just create a new matcher for jobs queued by this worker.
Any other opinions?
The only test that makes sense to me is when matching on a class you test how many jobs are enqueued for that class. Otherwise your test is semantically misleading, and you should be testing for how many jobs are in a queue.
If you want to not consider this a bug I would at least deprecate the old method of testing for the count of jobs in a queue by the class name, and separate that into a new matcher which only tests the number of jobs in a queue for a given class.
Either way I need to test two things:
- By class name to test for jobs enqueued of a given class
- By queue name to test for jobs enqueued in a given queue
Of note - is it possible for a single class to enqueue jobs into different queues, dynamically? If so then the classname => queue proxy is entirely incorrect.
Hi @pboling,
Deprecating the existing class based behavior and adding a new matcher is a reasonable way to migrate everyone forward, would you update the PR?
One alternative here would be to detect the type passed as an argument to the expect() method: if it's a symbol, look for the number of jobs on the queue. If it's a Class, count the instances of that class on the Class' queue.
Thoughts?
