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

use monotonic time #160

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@mikz
Copy link
Contributor

commented May 10, 2016

see ruby-concurrency/concurrent-ruby#256 for more information

Main reason is that your test can stub Time. So measuring duration of tests by that can get affected.
Similar thing was resolved in cucumber 2. See cucumber/cucumber-ruby-core#101

@shepmaster

This comment has been minimized.

Copy link
Member

commented May 10, 2016

Thanks! I don't suppose there is a gem that would provide this logic already, is there? It seems pretty out-of-scope for CI::Reporter to know about the underlying Ruby interpreter!

Out of curiosity, did you experience any concrete problems from the use of a time source that could be monkeyed with?

@shepmaster

This comment has been minimized.

Copy link
Member

commented May 10, 2016

a gem that would provide this logic

Especially if the same thing has already been copied into two other projects...

@mikz

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2016

@shepmaster there is concurrent-ruby implementation, but that looks like an overkill. I have not seen any other reasonable cross platform implementation.

Yes, my test suite runs -30 days and other random numbers.

@mikz

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2016

Just btw, I as a developer would pick copied code like this in my dependencies over having possibility of a version clash when trying to upgrade. Or installing another gem.

@mikz mikz force-pushed the mikz:monotonic-time branch from 7d16ad7 to cb0d704 May 10, 2016

@mikz mikz force-pushed the mikz:monotonic-time branch from cb0d704 to 627239d Jun 2, 2016

@mikz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

@shepmaster this is rebased and green. I really don't think it is worth to abstract this somewhere.
You generally needs this in concurrent environments for timeouts or when measuring duration of tests (which can stub time).

@shepmaster

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

I really don't think it is worth to abstract this somewhere.

I understand your feelings, but I don't agree with them 😸 As you can tell by my long response times, this is a low-touch project, and I'm not excited to maintain ~30 lines of code that from all appearances appear to be highly specific to a given Ruby interpreter.

I do agree that this kind of thing would be highly valuable. I could see accepting a PR that refactors CI::Reporter to allow injecting some kind of TimeSource. That would allow you to maintain those 30 lines of code in each of your projects, while not having to maintain a hacked-up version of CI::Reporter. How's that sound?

@mikz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2016

How's that sound?

Better than having a gem dependency.

But still I don't think it is acceptable default experience. Have you considered adding more maintainers to the project, so it is not just yours responsibility?

@Bertg

This comment has been minimized.

Copy link

commented Apr 25, 2018

Process.clock_gettime(Process::CLOCK_MONOTONIC) is supported by jRuby >9.0.0. So we could remove the check for jRuby. Older jRuby version would still use the Time.now fallback.

As a compromise between the argument It seems pretty out-of-scope for CI::Reporter to know about the underlying Ruby interpreter and the desire to add monotonic time.

Would that be acceptable?

@mikz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

@Bertg great! Will update the patch. Looks like there is no Process::CLOCK_MONOTONIC in JRuby 1.7.

@mikz mikz force-pushed the mikz:monotonic-time branch from 627239d to 499e5be Apr 25, 2018

@mikz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

@shepmaster is it small enough now? It is really just getting time.

@Bertg

This comment has been minimized.

Copy link

commented Apr 25, 2018

Looks great to me 👍

@Bertg

This comment has been minimized.

Copy link

commented Apr 25, 2018

@shepmaster I was actually planning to make this PR as well. I got inspired by this post: https://blog.dnsimple.com/2018/03/elapsed-time-with-ruby-the-right-way/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.