Add Process.clock_gettime support
#419
Conversation
|
|
||
| def clock_gettime_mock_time(clock_id, unit = :float_second) | ||
| mock_time = case clock_id | ||
| when Process::CLOCK_MONOTONIC |
There was a problem hiding this comment.
How about Process::CLOCK_THREAD_CPUTIME_ID?
There was a problem hiding this comment.
Yeah I could see adding that one too. I will focus on the bug fix first and then see if I can include this scope in the PR; if not, it should be a simpler follow-up after this lands.
test/timecop_test.rb
Outdated
| private | ||
|
|
||
| if RUBY_VERSION >= '2.1.0' |
There was a problem hiding this comment.
I know this is just following the convention of this codebase, so this isn't a comment on your PR per se:
Ruby 2.1.0 is 11 years old and has been EOL for 7 years.
There was a problem hiding this comment.
It would be nice to update the minimum Ruby version in the gemspec; right now it's at 1.9.2.
|
@lavoiesl more great feedback - thank you! |
|
Thanks @lavoiesl and @alexcwatt for making/workshopping this PR (and all the folks over the years that moved the original issues and PRs forward). I am going to have to play around with this a little bit, but don't see why this won't get merged in. One thought I had was that it would be nice if this new was mentioned in the README. Could you take a crack at that? |
|
@joshuacronemeyer I made a small change to the README. Let me know what you think. I'm happy to elaborate more but it seems the most important thing is to mention that Also let me know if you need me to change anything else, like squashing to a single commit; I kept the commit history originally so reviewers could easily see what I was changing between iterations. |
I wonder about a better name; this is a constant that represents enough time for Process.clock_gettime to have advanced.
|
@joshuacronemeyer Thank you for the detailed review! I obviously missed some things. I think this version addresses your comments. Please let me know what you think when you have another look. I meant to address this feedback sooner but couldn't get to it last weekend. I've preserved commit history so you can see what changed, but when this is ready to be merged, we can squash everything. |
| require 'timecop' | ||
|
|
||
| class TestTimecopWithProcessClock < Minitest::Test | ||
| TIME_EPSILON = 0.001 # seconds - represents enough time for Process.clock_gettime to have advanced if not frozen |
There was a problem hiding this comment.
Open to other ideas for this constant name to better represent the meaning that I'm currently explaining with the comment.
|
Can we make this stub opt-in? It seems to break various low-level use cases when it's enabled. e.g. #425 and the fluentd PR above... |
I am interested in being able to use Timecop with
Process.clock_gettime. I saw #220 (the request for this) and #258 (the PR that implemented it).This code is nearly the same as #258; I have only made minor changes.