fix: fix mock TickerFunc to act like real #7
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
| for m.inProgress { | ||
| m.cond.Wait() | ||
| } |
There was a problem hiding this comment.
note: this is the canonical usage from https://pkg.go.dev/sync#Cond.Wait
| mClock.Advance(time.Second).MustWait(testCtx) | ||
|
|
||
| waitErr := make(chan error, 1) | ||
| go func() { |
There was a problem hiding this comment.
suggestion: Do we need a waitgroup to ensure this goroutine is completely finished by the time the text exits? Ditto above on L244.
There was a problem hiding this comment.
We read from waitErr on L307, so a waitgroup is redundant. On success, we know this goroutine has terminated.
Generally speaking, we mainly care about whether the code under test leaks goroutines. Leaking a test goroutine almost never matters, since the test is finite (the exception is if the leaked goroutine interferes with other tests, which is rare).
Turns out #7 is bugged. When we refused to call the TickerFunc callback, we didn't advance the next event time, so the Mock kept re-scheduling `fire()` and we could never finish processing on the tick time.
fixes #5
fixes #6
Fixes the mock TickerFunc to behave like the real one in that it won't run more than one callback at a time and will wait until the callback completes to return from a
Wait()call.