-
Notifications
You must be signed in to change notification settings - Fork 1
Rewrite timer logic #3
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
base: master
Are you sure you want to change the base?
Conversation
| // WARNING: Tickers will cause this function to continue forever until something cancels the ticker. | ||
| // It will sleep after each bit of progress to the tickers a chance to run, but using this with | ||
| // tickers is not recommended. | ||
| func (m *Mock) WaitForAllTimers() time.Time { |
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 can add a RunNextTimer later, that should be fairly trivial.
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.
RunNextTimer or a way to cancel WaitForAllTimers would be nice.
| t.stopped = true | ||
| t.mock.removeClockTimer((*internalTimer)(t)) | ||
| // Ticker is a mock implementation of [time.Ticker]. | ||
| type Ticker struct { |
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.
Restructured to be smaller (when not using mocktime) and to avoid having to abstract over Ticker versus Timer in the mock logic.
1. Don't advance time all at once, advance to each deadline. 2. Use a heap instead of constantly sorting/iterating. 3. Modernize the context logic to avoid extra goroutines. 4. Make calling Set/Add/etc. thread-safe. All callers will cooperatively advance the clock until the requested time.
Also improve reliability of immediately firing timers.
Kubuxu
left a comment
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.
LGTM assuming tests pass
This isn't really necessary for small-scale tests, but... I wanted to clean some things up a bit.