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.

Sign up for GitHub

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

Jump to bottom

Keep a private reference to Process.clock_gettime #18

Merged
merged 1 commit into from
Jun 9, 2022
Merged

Keep a private reference to Process.clock_gettime #18

merged 1 commit into from
Jun 9, 2022

Conversation

casperisfine
Copy link

timeout 0.3.0 broke our test suite because we have some tests that stubs Process.clock_gettime making it return a value in the past, causing Timeout to trigger almost immediately.

I believe it wasn't a problem before because it was relying on Process.sleep.

cc @eregon what do you think? I agree this one is debatable, as if you are messing with time primitive you're on your own, but it's a common thing to do in tests suites with tools such as Timecop.

@byroot
Keep a private reference to Process.clock_gettime
171eef4 
`timeout 0.3.0` broke our test suite because we have some
tests that stubs `Process.clock_gettime` making it return
a value in the past, causing `Timeout` to trigger almost immediately.

I beleive it wasn't a problem before because it was relying on `Process.sleep`.
@eregon
Copy link
Member

eregon commented Jun 8, 2022

The fix seems reasonable.
I wonder if it's reasonable for Timecop to monkey patch clock_gettime though (especially with CLOCK_MONOTONIC), that seems likely to break far more things than just this. Shouldn't it only use a fixed instant for Time and Date/DateTime and leave the rest alone?

@casperisfine
Copy link
Author

casperisfine commented Jun 8, 2022 •
edited
Loading

Note that I said Timecop, but it actually doesn't do it (travisjeffery/timecop#220), it was a "manual" mocha stub.

Shouldn't it only use a fixed instant for Time and Date/DateTime and leave the rest alone?

Well, as long as some code Process.clock_gettime, you sometimes need to stub it to simulate some cases. timeout just happens to be a bit problematic.

that seems likely to break far more things than just this.

These tests are old, and AFAIK that's the first time they break. But yes, we could probably also cleanup our code and have a small indirection we could stub instead. If you're unconfortable with this change, I'm fine with closing. I mostly thought it was worthy of discussion, not necessarily of merging.

@eregon eregon merged commit e5911a3 into ruby:master Jun 9, 2022
@ChrisBr ChrisBr mentioned this pull request Jun 28, 2022
Closed
@jjb
Copy link
Contributor

jjb commented Feb 23, 2023

drawback of this is that if someone wants to stub Process.clock_gettime in order to test that something does time out, i think it won't work anymore 😄 i was not doing that but go excited when i saw this code because i might want to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eregon eregon eregon approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@casperisfine @eregon @jjb @byroot