chore: add clock pkg for testing time #13461
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
e6c30e4 to
8caaed8
Compare
There was a problem hiding this comment.
Nice work! I'm happy to see how the modified test cases in pubsub nicely demonstrates the utility of the trap concept. I didn't re-read the RFC but this implementation largely matches my recollection of the approach.
Considering this is likely going to be its own library in the future, I'd love to see some func Example...s that demonstrate the functionality and use-cases. Some in-package tests would be nice too.
clock/clock.go
Outdated
| // NewContextTicker is a convenience function that calls f on the interval d until either the | ||
| // given context expires or f returns an error. Callers may call Wait() on the returned Waiter | ||
| // to wait until this happens and obtain the errror. | ||
| NewContextTicker(ctx context.Context, d time.Duration, f func() error, tags ...string) Waiter |
There was a problem hiding this comment.
How would you feel about using functional options instead of ...string? An alternative to having this convenience function would be to accept the following signature:
NewTicker(d time.Duration, f func() error, opts ...Option) and you'd pass it clock.WithContext(ctx) or `clock.Tags("tag1", "tag2") as options.
I think this could result in a slightly cleaner and more flexible API, as well as the added verbosity/explicitness can help with code reads (i.e. "what are these strings?").
In this scenario, I imagine the return signature of NewTicker could include all three methods Stop, Reset and Wait.
There was a problem hiding this comment.
Firstly, maybe "ContextTicker" isn't the best name. It's not a ticker that optionally takes a context, it's a ticker calls the function f on the ticks, rather than just sending the ticks to a channel. Sort of like AfterFunc is a special timer that calls a function. "TickerFunc"?
Secondly, the idea is to include tags on every call in the clock interface, and all the timer/ticker calls as well. With the exception of this ContextTicker/TickerFunc, the idea is to verbatim replicate the functionality of the time package, not enhance it. So, I literally cannot think of another option we'd want other than tags, and even if I could, it's unlikely to apply to every call, so adding it to a generic Option interface would muddy the waters. I think variability is actually a downside in this case, and we should keep it simple.
Lastly, I'm not a fan of the verbosity of clock.Tags("tag1"). The signature says tags ...string, so I feel like that's clear enough once you jump to the definition.
|
I really like how this is looking so far! 👍 Just a few suggestions on my end. |
5da53ab to
e243711
Compare
e243711 to
63c9374
Compare
Adds a package for testing time/timer/ticker functions. Implementation is limited to
NewTimerandNewContextTicker, but will eventually be expanded to alltimefunctions from the standard library as well ascontext.WithTimeout(),context.WithDeadline().Replaces
benbjohnson/clockfor the pubsub watchdog, as a proof of concept.Eventually, as we expand functionality, we will replace most time-related functions with this library for testing.