-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
have_enqueued_job supports time object #2157
Conversation
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.
Thank a lot for your PR @alpaca-tc
Just a small comment but this is not a blocker.
@@ -172,6 +173,13 @@ def set_expected_number(relativity, count) | |||
end | |||
end | |||
|
|||
def serialize_and_deserialize_arguments(args) | |||
serialized = ::ActiveJob::Arguments.serialize(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.
I am surprise we have to serialize then deserialize. I am wondering if it can be simpler?
Also maybe we can avoid using exception to validate the correct behavior?
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 changed to use simple time rounder instead of serializer/deserializer.
It is copied from ActiveJob::TestHelper.
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 prefer the original implementation, it's simpler as it replicates what Rails does without special casing, the new implementation may not catch other edgecases and we're just reinventing what Rails is doing.
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'd also like to see a green build, which is why I started #2158
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 also like the original implementation, so I reverted. And, I deleted ActiveJob::DeserializationError
because it never occurred.
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.
Thanks for the change and sorry for the back and forth.
493fbb9
to
8a7317a
Compare
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.
LGTM but waiting for a green CI.
Can you cherry-pick 4eeb9ac into this to see if you get a green build? |
I'm happy to merge this if you are @benoittgt I think the failures are the same as 4-0-dev normally right? |
Yes and yes Jon. I will try tonight to fix the build on 4-0-dev. |
b293259
to
7ab98dd
Compare
Sorry last call. Can you rebase from 4-0-dev? Thanks a lot |
7ab98dd
to
e8bc2d9
Compare
@benoittgt All green now 💚 🎉 |
Thanks! 💚 |
Rails6 can pass Time as arguments to ActiveJob but
have_enqueued_job#with
doesn't support time object.Deserialized time has 0 usec value, it will fail when compared with original time.
This PR fixes it by to serialize/deserialize arguments before comparing.