-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Fix ActiveJob assertions with a GlobalID object argument #18654
Conversation
if job_args = args.delete(:args) | ||
args[:args] = ActiveJob::Arguments.serialize(job_args) | ||
end | ||
original_args |
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.
This is weird. Why original_args
is returned and what is mutated is args
? Is not better to mutate the args.dup
and return it?
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.
Probably a little weird but I need both the original and serialized args. The serialized args are used in the assertion comparison and the original args are used in the failure message if the assertion fails. It probably makes immediate sense if you place that code in the assertion method as follows:
def assert_enqueued_with(args = {}, &_block)
original_enqueued_jobs = enqueued_jobs.dup
clear_enqueued_jobs
args.assert_valid_keys(:job, :args, :at, :queue)
original_args = args.dup
if job_args = args.delete(:args)
args[:args] = ActiveJob::Arguments.serialize(job_args)
end
yield
matching_job = enqueued_jobs.any? do |job|
args.all? { |key, value| value == job[key] }
end
assert matching_job, "No enqueued job found with #{original_args}"
ensure
queue_adapter.enqueued_jobs = original_enqueued_jobs + enqueued_jobs
end
However I would have to repeat it in assert_performed_with
so I decided to place it in a helper method. Would you prefer I remove the helper in favour of the above?
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.
Never mind the above, sorry! It's been a long day :) I'll push an update.
450ba00
to
9d3042d
Compare
I think that should be OK now. Sorry about that :) |
ping @rafaelfranca |
Fix ActiveJob assertions with a GlobalID object argument
Backported as 6d65df5 |
Fix ActiveJob assertions with a GlobalID object argument Conflicts: activejob/lib/active_job/test_helper.rb
Muito obrigado! |
Since 8a73f4b the following no longer works when
bob
is aGlobalId
object:This patch fixes that and includes tests.
cc @cristianbica