Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Modify time first, round after (and strip additional precision). #568

Closed
wants to merge 1 commit into from

Conversation

dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 1, 2017

DO NOT MERGE. Let's discuss this change first.

I've noticed that TestDo_rateLimit_noNetworkCall test fails on Go master with the following error:

--- FAIL: TestDo_rateLimit_noNetworkCall (0.00s)
	github_test.go:549: rateLimitErr rate reset = 2017-02-25 01:49:01 +0000 UTC, want 2017-02-25 01:49:01 +0000 UTC m=+60.206993003

(Source: https://travis-ci.org/google/go-github/jobs/205180544.)

This is due to changes in Go 1.9 to perform monotonic elapsed time measurements. See golang/go#12914 and https://golang.org/design/12914-monotonic for full details.

Make the test pass by modifying time first, round after, and strip the monotonic clock reading by using AddDate(0, 0, 0). Doing that eliminates the unwanted additional monotonic precision from wanted time, making the expected time equality true.

This is also related to an open issue golang/go#18991. Depending on the outcome there, this PR could be simplified not to need AddDate(0, 0, 0).

I just wanted to get this PR created for visibility. We can discuss the potential fix, and wait on Go 1.9 to get closer to release before actually merging this, because there may be additional changes that would render this change overly complex or unnecessary.

I've noticed that TestDo_rateLimit_noNetworkCall test fails on Go
master with the following error:

	--- FAIL: TestDo_rateLimit_noNetworkCall (0.00s)
		github_test.go:549: rateLimitErr rate reset = 2017-02-25 01:49:01 +0000 UTC, want 2017-02-25 01:49:01 +0000 UTC m=+60.206993003

(Source: https://travis-ci.org/google/go-github/jobs/205180544.)

This is due to changes in Go 1.9 to perform monotonic elapsed time
measurements. See golang/go#12914 and
https://golang.org/design/12914-monotonic for full details.

Make the test pass by modifying time first, round after, and strip
the monotonic clock reading by using AddDate(0, 0, 0). Doing that
eliminates the unwanted additional monotonic precision from wanted
time, making the expected time equality true.
@dmitshur
Copy link
Member Author

dmitshur commented Mar 1, 2017

Worth noting, the reorder of Round(time.Second) and Add(time.Minute) actually has no effect at all, I did it just because the new order seemed more readable and logical to me. It was also more consistent with the comment.

The addition of AddDate(0, 0, 0) is the only thing that has an effect (in current Go master only, not in Go 1.8).

@dmitshur
Copy link
Member Author

dmitshur commented Mar 8, 2017

After golang/go@6c85fb0, this change to use AddDate(0, 0, 0) is no longer neccessary.

I still like the re-order of Round(time.Second) and Add(time.Minute), so maybe we can keep that. I'll update the PR.

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 8, 2017

I like the reorder too.

@dmitshur
Copy link
Member Author

dmitshur commented Mar 8, 2017

Actually, I think it's better to just close this PR, since the scope has changed too much. I'll just open a new PR for the small refactor, and reference this. We can close this PR since it's no longer needed.

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

Successfully merging this pull request may close these issues.

2 participants