Repository navigation
Use monotonic time when checking for timeout #105
Description
AbstractProcess#poll_for_exit(timeout) uses Time which can be stubbed in tests. Would be better to use monotonic time (ruby-concurrency/concurrent-ruby#256) to not have infinite loops when time changes.
It can be just some minimal implementation like cucumber/cucumber-ruby-core#103
Activity
enkessler commentedon Jul 5, 2016
I guess that I'm not seeing the problem.
The affected code doesn't look like it would get into an infinite loop unless you are specifically messing with time but, if you are intentionally messing with time for testing, wouldn't you expect different behavior out of time based functionality?
enkessler commentedon Jul 6, 2016
A followup question: if a tool did switch to something besides Time.now, wouldn't the various Time mocking libraries cease working for it unless they had their capability extended?
mikz commentedon Jul 8, 2016
The problem is that messing with time when testing is quite common. Also, technically you don't want to know the current time, but the amount of time that passed. Thats why
Process.clock_gettime(Process::CLOCK_MONOTONIC)is much better.I'd expect to timeout in given number of seconds, no matter if my internal clock of the system change. Either in Ruby or from the host OS (ntp, leap seconds, ...).
The whole purpose of monotonic time is that you don't want to clash with time mocking libraries. When measuring timeout, the clock itself is not important. Only what matters is the amount that passed.
If your system clock change, the timeout using
Timewould not be accurate.The patch would look like ruby-concurrency/concurrent-ruby#253
jarib commentedon Jul 8, 2016
FWIW, I think it makes sense for childprocess to use monotonic time for its timeouts.
enkessler commentedon Jul 8, 2016
Sure, and I do get why they are better than relying on system time from a production perspective. I'm just curious about how using
Time.nowis a negative testing impact.Say that I want to test a method that needs to timeout after 30 seconds. I don't want my test to take 30 seconds. I would rather mock time such that I can freeze it, kick off the method, advance time forward 30 seconds, and then confirm that the method timed out. If my method uses something besides
Time.nowand my mocking tool only works withTime.nowthen I can't mock time and my 30 second test will actually take 30 seconds to run. That's why when I hear something like 'using Time.now causes libraries that mock time to impact my tests' my first thought is 'yeah, but isn't that the point?'.In summary: I agree with your conclusion but I'm not understanding your premise.
sds commentedon Jan 12, 2019
Closing due to lack of activity. Happy to accept a pull request if someone would like to see this implemented!