Skip to content
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

Add Process.clock_gettime support #419

Merged

Conversation

alexcwatt
Copy link
Contributor

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.

test/timecop_test.rb Outdated Show resolved Hide resolved

def clock_gettime_mock_time(clock_id, unit = :float_second)
mock_time = case clock_id
when Process::CLOCK_MONOTONIC
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about Process::CLOCK_THREAD_CPUTIME_ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@alexcwatt alexcwatt requested a review from lavoiesl May 8, 2024 22:34
Copy link

@lavoiesl lavoiesl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested locally yet, but this LGTM

test/timecop_test.rb Outdated Show resolved Hide resolved
test/timecop_test.rb Outdated Show resolved Hide resolved
test/timecop_test.rb Outdated Show resolved Hide resolved
private

if RUBY_VERSION >= '2.1.0'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to update the minimum Ruby version in the gemspec; right now it's at 1.9.2.

@alexcwatt alexcwatt requested a review from lavoiesl May 9, 2024 15:54
@alexcwatt
Copy link
Contributor Author

@lavoiesl more great feedback - thank you!

@joshuacronemeyer
Copy link
Collaborator

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?

@alexcwatt
Copy link
Contributor Author

@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 Process.clock_gettime is now supported.

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.

Copy link
Collaborator

@joshuacronemeyer joshuacronemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the code works. I've flagged some problems with the tests and some additional tests i would like. Thanks again!

@alexcwatt
Copy link
Contributor Author

@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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to other ideas for this constant name to better represent the meaning that I'm currently explaining with the comment.

Copy link
Collaborator

@joshuacronemeyer joshuacronemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Thanks.

@joshuacronemeyer joshuacronemeyer merged commit 3a0b567 into travisjeffery:master Jun 1, 2024
11 checks passed
@alexcwatt alexcwatt mentioned this pull request Jun 1, 2024
@alexcwatt alexcwatt deleted the support-clock-gettime branch June 3, 2024 18:35
@nerdrew
Copy link
Contributor

nerdrew commented Jun 13, 2024

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants