-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
expecting a Worker to have a queue size is broken. #119
Comments
Fixed #120 |
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:
|
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? |
I'll try to get to it soon. |
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? |
Hi @sailing, That seems reasonable as well. |
Here is the pry session where I discovered the issue (worker names changed).
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.
The text was updated successfully, but these errors were encountered: