-
Notifications
You must be signed in to change notification settings - Fork 248
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
Issue #510: use monotonic tick count #512
Issue #510: use monotonic tick count #512
Conversation
class << self | ||
def now | ||
if defined?(Process::CLOCK_MONOTONIC) | ||
Process.clock_gettime(Process::CLOCK_MONOTONIC) |
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 can tell you that on every Ruby implementation that I'm aware of, Process::CLOCK_MONOTONIC
will be defined.
If you insist on this, you should at least put the if statement outside the method definition rather than checking it every 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.
Since I can never see every Ruby implementation, I feel best going with the documentation.
I moved the if
outside. That was tricky to get the tests to still pass, but I think I got it working, by using load
to keep reloading the file, and especially one final time in a before(:all)
block, to put it back like it started. 0ea7595
@@ -132,15 +132,15 @@ def status_for_time(time) | |||
it 'still does not process events because it is paused' do | |||
# pretend we were woken up at 0.6 seconds since start | |||
allow(config).to receive(:sleep). | |||
with(anything) { |*_args| state[:time] += 2.0 } | |||
with(anything) { |*_args| state[:monotonic_time] += 2.0 } |
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.
Is it necessary to rename this field?
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 found it helpful to keep clear that that was not the Time.now
stub value but rather the MonotonicTime.now
stub value.
It looked to me like it was just used in this test, so I didn't see a downside to clarifying the name.
Did I get that wrong?
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'm not sure I was just concerned it was part of a public interface.
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.
My read is that it's strictly internal to this test. The only reason it's in a hash is so it can be mutated as the test runs, to mimic time advancing. (An @
variable in the test would work that way too.)
0ea7595
to
20f0585
Compare
Addressed issue #510:
Listen::MonotonicTime
module withnow
method. including tests.mtime
) as it was, usingTime.now
.timestamp
and_timestamp
methods.