chore: change mock clock to allow Advance() within timer/tick functions #13500
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
| select { | ||
| case <-t.c: | ||
| default: | ||
| } | ||
| if d == 0 { |
There was a problem hiding this comment.
before, when we had Advance() handle multiple sets of timers/ticks events, we just handled this case of a zero-duration timer reset in the advance loop. But now, since Advance() can only trigger one "flight" of events, we handle the zero-duration case inline.
One slight disadvantage is that there isn't a way to wait for such events to complete.
I think this is fine for relatively simple cases, where it's clear what the next event is. For example, if you're testing something with a single ticker, you can assert the duration of the ticker once and then use Where you can get yourself in trouble is when there are multiple timers and/or tickers, and you're not carefully controlling when they are set, or not paying attention, and so the test is written assuming the next event is one thing, but it's actually a different thing. |
I've modified the
Mock.Advance()function to be more simple and allow more flexibility in testing.tl;dr -
Advance()now only allows you to advance up to the next timer/ticker event and no further, and immediately advances the clock, rather than the clock advancing async as ticks and timers are processed.Why?
Prior to this change,
Advance()moves the clock forward, possibly triggering multiple timers/ticks. You had toWait()for the result before you could be sure the clock had moved as far as you asked. And, crucially, the internal implementation didn't allow you to make another call toAdvance()until the first one finished.But, a test case that I want to support is being able to have time progress during a timer/ticker functional callback. The added UT to
enterprise/tailnetis an example of this.heartbeatsmakes a call toNow()to record the time of a heartbeat, then later callsUntil()to calculate when next to check for expiry. In a real system these calls can happen with different "current" times, and there was a bug that I noticed that if a coordinator is just about to expire, but not quite, by the time we get around to calculated when next to check, it could already be in the past, and we would have ignored it. So, we need toTrap()the call toUntiland advance the clock.With the prior implementation, this fails. I initially went down a rabbit hole of having a central goroutine manage advancing the clock, and allowing
Advance()calls to come in while others have not fully resolved, but things get messy and complicated very quickly. In particular, how does the thing managing the clock know the order to process advances when one is happening during another?That way lies madness, probably, and completely destroys the goal of writing tests where the time is predictable after every mock call.
The proposed solution
So instead, we restrict
Advance()to be able to, in a single call, move the clock up to the next event and no further. Then, it can immediately set the new time, even if you asynchronously wait for timers/ticker functions to return.For cases where you don't know or want to compute how long to the next event, I've added
AdvanceNext()which returns the amount advanced for asserting or use in further calculations.This gets us back to a place where UTs are in the imperative style and the clock moves predictably, monotonically forward as the UT is executed.