Specify the queue to be used with assert_enqueued_jobs #27624
Conversation
| Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
elfassy force-pushed the assert_enqueued_jobs_with_queue_level branch 3 times, most recently from b140f06 to 69921f3 Compare 
eileencodes left a comment
eileencodes left a comment There was a problem hiding this comment.
I like the idea of adding this, thanks for working on it. I added some comments about suggestions for changes.
There was a problem hiding this comment.
I think this sentence is confusing and can be worded better. How about something like "Asserts jobs were enqueued the specified number of times. If a queue name is passed, asserts jobs were enqueued for that specific queue."
There was a problem hiding this comment.
How about The number of times a job is enqueued to a specific queue can also be asserted
Comments would then read:
# Asserts that the number of enqueued jobs matches the given number. # ... # # If a block is passed, that block should cause the specified number of # jobs to be enqueued. # # ... # # The number of times a specific job is enqueued can be asserted. # # ... # # The number of times a job is enqueued to a specific queue can also be asserted. # # ... There was a problem hiding this comment.
Yea that's better. Only thing I'd change is where you say "that block should" - it sounds like "hey it should but who knows ¯_(ツ)_/¯" so I'd change it to "that block will enqueue the specified number of jobs"
There was a problem hiding this comment.
This is pretty picky on my part, but I don't like the example of the queue name being "low" because it implies in this example that queue name is the priority. While often queue name does have relation to priority there is a priority setting so I'd rather use "default" like you did below.
There was a problem hiding this comment.
I find this new code to be rather confusing to read. Can you think of a better way to write it? Even if there are more lines I'd like it to be more readable.
elfassy force-pushed the assert_enqueued_jobs_with_queue_level branch from 0eed72d to b16053c Compare 
rafaelfranca left a comment
rafaelfranca left a comment There was a problem hiding this comment.
Could you squash your commits?
elfassy force-pushed the assert_enqueued_jobs_with_queue_level branch from d781cfd to 8a4ce2e Compare 
elfassy force-pushed the assert_enqueued_jobs_with_queue_level branch from 8a4ce2e to 3738358 Compare 
6 participants
Summary
Change
assert_enqueued_jobsto allow thequeueto which jobs are enqueued to be verified. For example: