Skip to content
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

Specify the queue to be used with assert_enqueued_jobs #27624

Merged

Conversation

elfassy
Copy link
Contributor

@elfassy elfassy commented Jan 9, 2017

Summary

Change assert_enqueued_jobs to allow the queue to which jobs are enqueued to be verified. For example:

assert_enqueued_jobs 10, queue: 'low', only: 'Fun' do
  ...
end
assert_enqueued_jobs 1, queue: 'high', only: 'Important' do
  ...
end

@rails-bot
Copy link

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 elfassy force-pushed the assert_enqueued_jobs_with_queue_level branch 3 times, most recently from b140f06 to 69921f3 Compare January 10, 2017 16:19
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of adding this, thanks for working on it. I added some comments about suggestions for changes.

@@ -77,14 +77,23 @@ def queue_adapter_for_test
# HelloJob.perform_later('jeremy')
# end
# end
def assert_enqueued_jobs(number, only: nil)
#
# If a queue name is passed, the number of times jobs enqueued to that specific queue can be asserted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
    #
    # ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

# If a queue name is passed, the number of times jobs enqueued to that specific queue can be asserted.
#
# def test_logging_job
# assert_enqueued_jobs 1, queue: 'low' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

enqueued_jobs.count do |job|
job_class = job.fetch(:job)
(only.nil? || Array(only).include?(job_class)) &&
(queue.nil? || job.fetch(:queue, job_class.queue_name) == queue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you squash your commits?

@elfassy elfassy force-pushed the assert_enqueued_jobs_with_queue_level branch from d781cfd to 8a4ce2e Compare January 18, 2017 13:47
@elfassy elfassy force-pushed the assert_enqueued_jobs_with_queue_level branch from 8a4ce2e to 3738358 Compare January 18, 2017 14:13
@guilleiguaran guilleiguaran merged commit 1a752b6 into rails:master Jan 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants