Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: add clock pkg for testing time#13461

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

Merged
spikecurtis merged 1 commit intomainfromspike/clock-testing-watchdog
Jun 5, 2024

Conversation

spikecurtis
Copy link
Contributor

@spikecurtisspikecurtis commentedJun 4, 2024
edited
Loading

Adds a package for testing time/timer/ticker functions. Implementation is limited toNewTimer andNewContextTicker, but will eventually be expanded to alltime functions from the standard library as well ascontext.WithTimeout(),context.WithDeadline().

Replacesbenbjohnson/clock for 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.

@spikecurtisGraphite App
Copy link
ContributorAuthor

spikecurtis commentedJun 4, 2024
edited
Loading

@spikecurtisspikecurtis marked this pull request as ready for reviewJune 4, 2024 07:38
@spikecurtisspikecurtisforce-pushed thespike/clock-testing-watchdog branch 2 times, most recently frome6c30e4 to8caaed8CompareJune 4, 2024 08:08
Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 somefunc Example...s that demonstrate the functionality and use-cases. Some in-package tests would be nice too.

johnstcn reacted with thumbs up emoji
// 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,ffunc()error,tags...string)Waiter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 itclock.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 ofNewTicker could include all three methodsStop,Reset andWait.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Firstly, maybe "ContextTicker" isn't the best name. It's not a ticker that optionally takes a context, it's a ticker calls the functionf on the ticks, rather than just sending the ticks to a channel. Sort of likeAfterFunc is a special timer that calls a function. "TickerFunc"?

Secondly, the idea is to include tags onevery call in theclock 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 thetime 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 toevery call, so adding it to a genericOption 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 ofclock.Tags("tag1"). The signature saystags ...string, so I feel like that's clear enough once you jump to the definition.

@johnstcn
Copy link
Member

I really like how this is looking so far! 👍 Just a few suggestions on my end.

@spikecurtisspikecurtisforce-pushed thespike/clock-testing-watchdog branch 2 times, most recently from5da53ab toe243711CompareJune 4, 2024 12:37
@spikecurtisspikecurtisforce-pushed thespike/clock-testing-watchdog branch frome243711 to63c9374CompareJune 5, 2024 06:14
@spikecurtisspikecurtis merged commit42324b3 intomainJun 5, 2024
@spikecurtisspikecurtis deleted the spike/clock-testing-watchdog branchJune 5, 2024 09:55
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 5, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@mafredrimafredriAwaiting requested review from mafredri

Assignees

@spikecurtisspikecurtis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@spikecurtis@johnstcn@mafredri

[8]ページ先頭

©2009-2025 Movatter.jp