- Notifications
You must be signed in to change notification settings - Fork1k
chore: change mock clock to allow Advance() within timer/tick functions#13500
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@spikecurtis and the rest of your teammates on |
| case<-t.c: | ||
| default: | ||
| } | ||
| ifd==0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
before, when we hadAdvance() handle multiple sets of timers/ticks events, we just handled this case of a zero-duration timer reset in the advance loop. But now, sinceAdvance() 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this is a great usability improvement.
However, I anticipate casual usage will simply ignore the returned duration ofAdvanceWait():
_, w := mClock.AdvanceNext(); w.MustWait(ctx)Do you foresee this being an anti-pattern?
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. |
Uh oh!
There was an error while loading.Please reload this page.
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, andimmediately 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 progressduring 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 isjust 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 happeningduring 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.