-
Notifications
You must be signed in to change notification settings - Fork 42
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
adjust_total_time before running on_complete callbacks #84
Conversation
a189927
to
fa25159
Compare
fa25159
to
691c367
Compare
Latest pushed changes:
|
def test_jobs_using_on_complete_have_accurate_total_time | ||
freeze_time do | ||
JobThatCompletesAfter3Seconds.perform_now(->(job) { assert_equal(3, job.total_time) }) | ||
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.
I would suggest simplifying the test:
def test_jobs_using_on_complete_have_accurate_total_time
freeze_time do
job = JobThatCompletesAfter3Seconds.new
job.perform_now
assert_equal(3, job.total_time)
end
end
That way you do not need to pass the assertion around, which is a little weird 😄
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 a different test though and it would already pass on master without the patch.
The reason for passing the assertion around is so it can be injected in the on_complete handler to show that at that moment, it will/won't have the accurate total time.
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.
Code LGTM. I would like it if you could modify the test file to remove the need to pass the assertions around. Apart from that this is ready to be merge 😄
@djmortonShopify 2 questions before I hit merge:
|
I think we usually bump the version in a separate PR. I'm ok with a patch version bump for this because I'm still of a mind that this is a bug fix. There are no changes to the API, so it'll only "break" for people relying on the previous, arguably incorrect, behaviour. @GustavoCaso Thoughts? |
Closes #83
What are you trying to accomplish?
We were using the
on_complete
callback to report the job total run time but we noticed thattotal_time
only gets updated after the callbacks are called.What approach did you choose and why?
There were 3 options:
adjust_total_time
so that it's called before the callbacks.adjust_total_time
so that it's called before the callbacks and also right before logging the output summary. This could be useful in case the "total_time after callbacks" is valuable.ActiveSupport::Notifications.instrument
instead. This would overlap with the callback functionality.I decided for 1 because I think it's the one that makes the most sense.
What reviewers should focus on?
total_time
insideon_complete
callbacks, this patch would be a breaking change.