-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Refs #17684: Enhances rdoc for ActiveJob assert_performed assertions #17687
Conversation
# | ||
# def test_jobs | ||
# queue_adapter.perform_enqueued_jobs = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could just tell about the condition but not show how to enable. This code for example will change the global state of the tests and will make test order dependent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right about the global state change, but not showing how to enable is little help.
My feeling is that something like this is missing, maybe in test_helper:
def perform_enqueued_jobs
@old_perform_enqueued_jobs = queue_adapter.perform_enqueued_jobs
queue_adapter.perform_enqueued_jobs = true
yield
ensure
queue_adapter.perform_enqueued_jobs = @old_perform_enqueued_jobs
end
Maybe this may be placed in test_adapter.
So one can code this test:
perform_enqueued_jobs do
assert_performed_jobs 1 do
# ...
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but I'd make it the default value of assert_performed_jobs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a good approach, too...
How do we proceed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add a new commit to this PR adding support to enable perform_enqueued_jobs
by default in the block version of assert_performed_jobs
@rafaelfranca Do you agree the following lines
and
are potentially dangerous, as they change queue_adapter state? |
…ate required by assert_performed_jobs assertions
Yeah, they should have a after block too. |
Squashed and merged at 0bc04ce...d886a27 |
Refs #17684: Enhances rdoc for ActiveJob assert_performed assertions
No description provided.