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 usage information to clock library README#13594

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-readme-usage
Jun 25, 2024

Conversation

spikecurtis
Copy link
Contributor

@spikecurtisspikecurtis commentedJun 18, 2024
edited
Loading

Adds a Usage section to the README of the clock testing library.

bpmct reacted with hooray emoji
@spikecurtisGraphite App
Copy link
ContributorAuthor

spikecurtis commentedJun 18, 2024
edited
Loading

@spikecurtisspikecurtis marked this pull request as ready for reviewJune 18, 2024 11:40
Comment on lines 5 to 6
_Note: Quartz is the name I'm targeting for the standalone open source project when we spin this
out._
Copy link
Member

Choose a reason for hiding this comment

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

Do you know when this would be? Is it worth calling thisquartz here instead now?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I think it will be soon. We could rename toquartz in-repo before spinning out, or at the time we spin out. I don't think one is particularly easier or harder.

}
```

The`*Mock` clock starts at Jan 1, 2024, 00:00 UTC by default, but you can set any start time you'd like prior to your test.
Copy link
Member

Choose a reason for hiding this comment

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

2024 will quickly be arbitrary and difficult to remember. How about the year 2000?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

More or less still arbitrary. If the start time matters at all to the test best practice is toSet() it at the start.

distinguish them in your traps.

```go
trap:= mClock.Trap.Now("foo")// traps any calls that contain "foo"
Copy link
Member

Choose a reason for hiding this comment

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

I find tag systems have a persistent cognitive overhead as I have to remember the subtle differences between implementations. E.g.:

  • do tags match exactly or does a subset suffice
  • if I trap "foo" and call w/ "foo" "bar" does it match?
  • if I trap "foo" "bar" and call w/ "bar" does it match?
  • is matching case sensitive?

And then, there's a long-term burden of keeping the tag names consistent.

Instead, I wonder if we should promote accepting multiple independent clock instances.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should promote accepting multiple independent clock instances.

I think that would significantly increase the friction of adopting this library. In simple cases, you should be able to get by without using tags at all. In more complex cases, having to manage multiple clocks could quickly become burdensome. I could imagine folks ending up just keeping amap[string]clock to keep track of them all, which amounts to much the same thing?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking using different variables, so leveraging the language instead of a prescribed data structure.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The cognitive overhead of thinking about multiple clocks sounds way harder than tags. I'm strongly against promoting it in our documentation, but there is nothing actually stopping people from doing it if tags feels that burdensome.

We could make the tag matching more explicit in our naming, at the expense of a more verbose API. I.e.

trap := mClock.Trap().Now().Contains("foo")

rather than our present

trap := mClock.Trap().Now("foo")

It would be more extensible for additional matching strategies in future. I personally prefer to type less in UTs and handle the "cognitive burden", but I'm open to changing it if that's an overall consensus.

Keep in mind that this is a library forunit testing, and as such, the number of unique calls is typically a handful, perhaps a few dozen. I don't think it's particularly good practice to mock time for integration tests involving more than a few components.

Tagging (labeling) systems like K8s are designed to handle clusters with tens of thousands of containers and so have a complex matching language. Honestly for this library, just having a single string with exact matching would be sufficient for 95% of cases, but

  1. the...string syntax affords multiple entries, and it felt annoying to like,panic if you put more than one
  2. there are some few use cases for multiple tags, such as components that programmatically start multiple tickers/timers, such asapphealth, which then allows us to verify tickers for each app get started.

@spikecurtisspikecurtisforce-pushed thespike/clock-testing-new-ticker branch fromc7079dc to39aa737CompareJune 20, 2024 05:20
@spikecurtisspikecurtisforce-pushed thespike/clock-testing-readme-usage branch fromedbd4ed tofcecc22CompareJune 20, 2024 05:20
@spikecurtisspikecurtisforce-pushed thespike/clock-testing-new-ticker branch from39aa737 tod314742CompareJune 20, 2024 06:03
@spikecurtisspikecurtisforce-pushed thespike/clock-testing-readme-usage branch fromfcecc22 to38ec726CompareJune 20, 2024 06:03
Base automatically changed fromspike/clock-testing-new-ticker tomainJune 20, 2024 06:16
@spikecurtisspikecurtisforce-pushed thespike/clock-testing-readme-usage branch from38ec726 to82e85b4CompareJune 20, 2024 06:21
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 on the documentation!


Whenever you would call into`time` to start a timer or ticker, call`Component.clock` instead.

In production, set this clock to`quartz.NewReal()` to create a clock that just transparently passes
Copy link
Member

Choose a reason for hiding this comment

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

I wonder ifNewStdlib or something like that would be clearer. I feel likeNewReal refers to a real-time clock (hardware). Maybe just me. 😅

Copy link
Member

Choose a reason for hiding this comment

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

NewReal makes sense to me in the context of a mock clock.

I find the reference toStd orStdlib in API names to be a code smell.

d,w:= mClock.AdvanceNext()
w.MustWait(ctx)
// d contains the duration we advanced
```
Copy link
Member

Choose a reason for hiding this comment

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

It might be good to document when and why you would useAdvance instead ofAdvanceNext.Advance has basically the same limitation, except it can increment to times before any tick/timers.

To makeAdvance more useful, I think it would be good to have amClock.NextTick(), this can allow you to write code like:

fori:=0;i<10;i++ {d:=time.Secondifnt:=mClock.NextTick();nt<d {d=nt}mClock.Advance(d).MustWait(ctx)}

Thoughts? A use-case that is very hard to cover undermClock.Advance is:

t:=mClock.NewTicker(10*time.Second)forranget.C {// do job that takes nondeterministic amount of time.t.Reset(10*time.Second)}

PS. The argument could probably be made that this library is not meant for this kind of use-case, but then I question the utility of theAdvance function.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I do think theNextTick() call is useful, although I would probably call itNext() or perhapsNextEvent(), since it's not just tickers that are affected.

The use case I had in mind was being able to blow through timers and ticks to advance a certain duration.

desired := time.Minutefor desired > 0 {    n := mClock.Next()    if n > desired {        mClock.Advance(desired).MustWait(ctx)        break    }    mClock.Advance(n).MustWait(ctx)    desired -= n}

I'm not sure I understand the point of your first example. Is it that you don't know the size of the ticks and you want to advance 10 seconds or 10 ticks, whichever comes first?

In terms of the second example, I'm pretty confident it works fine withAdvance(): you can trap thet.Reset() call and control the amount of mocked time the job takes (the amount of real time should be irrelevant).

mafredri reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Oh, and, I thinkAdvance() isusually preferable toAdvanceNext(), since it allows you to keep a mental tally of time passing during the test case. But, not always. There was even one test I refactored that I thought was clearer usingSet().

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the point of your first example. Is it that you don't know the size of the ticks and you want to advance 10 seconds or 10 ticks, whichever comes first?

I based it off the example in readme, so I didn't really pay attention to the i < 10. Essentially the idea was to advance 10 seconds, one second at a time, or if an event is incoming, advance to it and then continue (since you can't advance past it). I believe yourdesired example is closer to what I was going for.

In terms of the second example, I'm pretty confident it works fine with Advance(): you can trap the t.Reset() call and control the amount of mocked time the job takes (the amount of real time should be irrelevant).

I suppose you're right, since we can assume the clock is stopped unless we control it, we know when thatReset will land. I wanted to demonstrate a case where we couldn't know theReset value but my example didn't quite reflect this. 😅

Oh, and, I think Advance() is usually preferable to AdvanceNext(), since it allows you to keep a mental tally of time passing during the test case. But, not always. There was even one test I refactored that I thought was clearer using Set().

I can totally get behind that reasoning, but in complex systems I think it can become hard to account for all the timers and ticks, esp. if they're of different lengths, or worse, not fully predictable (then again, that could be a sign of bad design).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I addedNext() up stack from this, btw.

@spikecurtisspikecurtisforce-pushed thespike/clock-testing-readme-usage branch from82e85b4 to2ed3ff0CompareJune 20, 2024 11:22
@ammarioammario removed their request for reviewJune 24, 2024 18:36
@ammario
Copy link
Member

Removed my review as it seems like you're getting great FB from others and I don't want to hold up the release.

@spikecurtisspikecurtis merged commit0d2f146 intomainJun 25, 2024
@spikecurtisspikecurtis deleted the spike/clock-testing-readme-usage branchJune 25, 2024 12:38
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@ammarioammarioammario left review comments

@johnstcnjohnstcnjohnstcn approved these changes

@mafredrimafredriAwaiting requested review from mafredri

@kylecarbskylecarbsAwaiting requested review from kylecarbs

Assignees

@spikecurtisspikecurtis

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@spikecurtis@ammario@mafredri@johnstcn@kylecarbs

[8]ページ先頭

©2009-2025 Movatter.jp