This repository has been archived by the owner on Aug 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 105
fix those damn sporadically false positive usage tests #342
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
after lots of experimentation I figured out the mock clock sometimes simply doesn't properly trigger, so that in Usage.Report() sometimes nothing is received on the tick channel, despite advancing the fake clock by more than strictly nessecary (i tried with an extra ms), despite calling runtime.Goshed() ourselves, and despite sleeping 20 ms with the real clock. The author of the clock package confirms that due to the way the runtime schedules goroutines, there's no way around the fake clock sometimes not working. See https://gophers.slack.com/archives/general/p1462238960008162 Furthermore, in discussion with the golang developers at golang/go#8869 it becomes clear that it's unlikely that we'll have a fakeable clock anytime soon. Ben Johnson (clock author) suggests in the above mentioned gophers thread that we could mock out the tick function and pass in a different function in tests. However, that changes so much of the time logic that it becomes pointless to do any time-based testing in this design. We could also switch to simply test the basics, not time based. Since the timing code is pretty easy. However before we go that route, I wanted to try working with the real clock. Basically run the usage reporting in real time, but scaled down to millisecond level instead of second level, to make it finish fairly quickly. So now some semantics are changing a bit: * we allow up to <period> ms for the usage report to be in the state we need it * so we now works with steps, which don't happen at exact predictable timestamps, rather they have to happen within a timeframe * checking timestamp would have gotten more complicated, so I just removed it. It's easy to reason that if the updates come within the alotted times, then the timestamps should also be set correctly. * there's no serious need to explicitly pass around interval settings anymore, we just use 1 everywhere. If it turns out that this approach also triggers false positives (for example due to circleCI machines being maxed out of CPU and the reporting unable to happen within the needed time) then we can address as needed and still switch to the simpler approach. But that seems very unlikely. This should work.
7f66846
to
f035541
Compare
for the record I just ran |
I think you are going about testing this the wrong way. I would
|
Dieterbe
added a commit
that referenced
this pull request
Jan 2, 2017
Dieterbe
added a commit
that referenced
this pull request
Jan 2, 2017
Dieterbe
added a commit
that referenced
this pull request
Jan 3, 2017
Dieterbe
added a commit
that referenced
this pull request
Jan 3, 2017
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
after lots of experimentation I figured out the mock clock sometimes
simply doesn't properly trigger, so that in Usage.Report() sometimes
nothing is received on the tick channel, despite advancing the fake
clock by more than strictly nessecary (i tried with an extra ms),
despite calling runtime.Goshed() ourselves, and despite sleeping
20 ms with the real clock.
The author of the clock package confirms that due to the way the
runtime schedules goroutines, there's no way around the fake clock
sometimes not working. See
https://gophers.slack.com/archives/general/p1462238960008162
Furthermore, in discussion with the golang developers at
golang/go#8869 it becomes clear that it's
unlikely that we'll have a fakeable clock anytime soon.
Ben Johnson (clock author) suggests in the above mentioned gophers
thread that we could mock out the tick function and pass in a different
function in tests. However, that changes so much of the time logic
that it becomes pointless to do any time-based testing in this design.
We could also switch to simply test the basics, not time based.
Since the timing code is pretty easy.
However before we go that route, I wanted to try working with the real
clock. Basically run the usage reporting in real time, but scaled down
to millisecond level instead of second level, to make it finish fairly
quickly.
So now some semantics are changing a bit:
need it
timestamps, rather they have to happen within a timeframe
removed it. It's easy to reason that if the updates come within the
alotted times, then the timestamps should also be set correctly.
anymore, we just use 1 everywhere.
If it turns out that this approach also triggers false positives
(for example due to circleCI machines being maxed out of CPU and the
reporting unable to happen within the needed time) then we can address
as needed and still switch to the simpler approach.
But that seems very unlikely. This should work.