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

Fix Flappy Stats Test #164

Merged
merged 1 commit into from
Aug 4, 2016
Merged

Fix Flappy Stats Test #164

merged 1 commit into from
Aug 4, 2016

Conversation

thanodnl
Copy link
Contributor

@thanodnl thanodnl commented Aug 4, 2016

This PR is in the hope to fix a flappy test that happens more often than not:

--- FAIL: TestProtocolStats (5.09s)
    Error Trace:    stats_test.go:89
    Error:      Should not be zero, but was 0

    Error Trace:    stats_test.go:90
    Error:      Should not be zero, but was 0

--- FAIL: TestStatsTestSuite (5.21s)
FAIL
FAIL    github.com/uber/ringpop-go/swim 9.984s

The reasoning behind the change is that metrics.Meter mentioned in the comments above is snapshotted every 5 seconds and made available to read from. By exactly waiting 5 seconds we might still read the values just before the snapshot is actually populated with information. By waiting an additional second the hope is that we read less 0's.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage remained the same at 94.668% when pulling a492b9a on test/flappy-stats-test into 19ff2e6 on dev.

@thanodnl thanodnl changed the title increase waiting time for snapshot of stats to be updated. Fix Flappy Stats Test Aug 4, 2016
@thanodnl
Copy link
Contributor Author

thanodnl commented Aug 4, 2016

Ran the unit tests on travis 6 times without any flap on the changed test.

@mennopruijssers
Copy link
Contributor

LGTM. it's sad though that we can't inject a different time in the meter library.

@@ -68,7 +68,7 @@ func (s *StatsTestSuite) TestProtocolStats() {

// We need to sleep for at least 5 seconds, as this is the tick period for
// the metrics.Meter and it cannot be easily changed.
time.Sleep(5 * time.Second)
time.Sleep(6 * time.Second)
Copy link
Contributor

@mennopruijssers mennopruijssers Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@motiejus
Copy link
Contributor

motiejus commented Aug 4, 2016

LGTM

The LGTM is in alt-text.

@thanodnl thanodnl merged commit b3995b6 into dev Aug 4, 2016
@thanodnl thanodnl deleted the test/flappy-stats-test branch August 4, 2016 14:25
@dansimau
Copy link
Contributor

dansimau commented Aug 4, 2016

I think we can optimise this

time.Sleep(5100*time.Millisecond)

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

Successfully merging this pull request may close these issues.

5 participants