apiserver: Use WaitAdvance in listenerSuite.TestPause #7596
Conversation
babbageclunk
changed the title
apiserver/listener_test.go
Outdated
| @@ -157,14 +158,17 @@ func (s *listenerSuite) TestPause(c *gc.C) { | |||
| }() | |||
|
|
|||
| start <- true | |||
There was a problem hiding this comment.
start seems pointless? it's created just above and sent on here, and the time between those should be negligible
There was a problem hiding this comment.
Good call - removed.
apiserver/listener_test.go
Outdated
| } | ||
| c.Assert(s.listener.count, gc.Equals, 0) | ||
| s.clock.Advance(time.Millisecond) | ||
| err = s.clock.WaitAdvance(10*time.Millisecond, time.Millisecond, 1) | ||
| c.Assert(err, jc.ErrorIsNil) | ||
| select { | ||
| case <-done: | ||
| case <-time.After(10 * time.Second): |
There was a problem hiding this comment.
use coretesting.LongWait here? i.e. the amount of time to when something is expected to happen
apiserver/listener_test.go
Outdated
| @@ -157,14 +158,17 @@ func (s *listenerSuite) TestPause(c *gc.C) { | |||
| }() | |||
|
|
|||
| start <- true | |||
| s.clock.Advance((l.minPause/time.Millisecond - 1) * time.Millisecond) | |||
| minPauseTimeGone := time.After(50 * time.Millisecond) | |||
| err := s.clock.WaitAdvance(time.Millisecond, (l.minPause/time.Millisecond-1)*time.Millisecond, 1) | |||
There was a problem hiding this comment.
use coretesting.ShortWait for the 2nd arg here? i.e. the amount of time to when something is expected not to happen
apiserver/listener_test.go
Outdated
| select { | ||
| case <-done: | ||
| c.Fatal("pause returned too soon") | ||
| case <-time.After(50 * time.Millisecond): | ||
| case <-minPauseTimeGone: |
There was a problem hiding this comment.
I think this should be coretesting.ShortWait too
babbageclunk
force-pushed
the
listener-test-reliability
branch
from
fdbf299
to
83883a6
Compare
The test passes fine locally but fails often in CI, and when run under stress it fails after 1 or 2 runs. It looks like the clock advance is happening before the throttlingListener manages to wait for it. Change the Advance calls to WaitAdvance so we know the listener is waiting when we advance. Removed start channel - it complicated the test for no real benefit.
babbageclunk
force-pushed
the
listener-test-reliability
branch
from
83883a6
to
36f1846
Compare
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
Merge job died. :( |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
Build failed: Tests failed |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Description of change
The test passes fine locally but fails often in CI, and when run under
stress it fails after 1 or 2 runs. It looks like the clock advance is
happening before the
throttlingListenermanages to wait for it. Changethe
Advancecalls toWaitAdvanceso we know the listener is waiting whenwe advance. The
time.Afterthreshold needs to be created before theWaitAdvancecall so that the done channel doesn't appear to be readyearly in the case that we needed to wait for the accept goroutine to be
waiting. (Picking the max waits on the
WaitAdvancecalls was fiddlybecause
WaitAdvancedivides the max wait time into 10 chunks.)QA steps
The test passes normally and for more than 20 runs under stress.