- Notifications
You must be signed in to change notification settings - Fork1k
chore(coderd/database/dbpurge): replace usage of time.* with quartz#14480
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
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
| agent:=mustCreateAgentWithLogs(ctx,t,db,user,org,tmpl,tv,now.Add(-8*24*time.Hour),t.Name()) | ||
| // After dbpurge completes, the ticker is reset. Trap this call. | ||
| trapReset:=clk.Trap().TickerReset() | ||
| defertrapReset.Close() |
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.
This seems shared between all tests, how about refactoring to a helper like below, or consider only testing it in one test and assuming it applies to the rest?
t.Helper()// After dbpurge completes, the ticker is reset. Trap this call.trapReset:=clk.Trap().TickerReset()defertrapReset.Close()closer:=do(clk)// Wait for the initial nanosecond tick.clk.Advance(time.Nanosecond).MustWait(ctx)trapReset.MustWait(ctx).Release()// Wait for ticker.Reset()returncloser
Perhaps this example can even be taken further, just here as food for thought.
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 modify this test in the follow-up PR#14460 so the sharing between tests won't be an issue.
But I do like the idea of extracting this to a test helper!
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
| funcTestDeleteOldWorkspaceAgentStats(t*testing.T) { | ||
| db,_:=dbtestutil.NewDB(t) | ||
| logger:=slogtest.Make(t,&slogtest.Options{IgnoreErrors:true}).Leveled(slog.LevelDebug) | ||
| ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitShort) |
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.
Nit:testutil.Context is a nice convenience for this:
funcContext(t*testing.T,dur time.Duration) context.Context {ctx,cancel:=context.WithTimeout(context.Background(),dur)t.Cleanup(cancel)returnctx}
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.
Will address in follow-up 👍
…teOldWorkspaceAgentLogs
…CreateAgentWithLogs
chore(dbpurge): use quartz.TickerFunc instead
a74273f intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Related to#10576
This PR introduces quartz to
coderd/database/dbpurgeand updates the following unit tests to make use of Quartz's functionality:TestPurgeTestDeleteOldWorkspaceAgentLogsAdditionally, updates
DeleteOldWorkspaceAgentLogsto replace the hard-coded interval with a parameter passed into the query. This aids in testing and brings us a step towards allowing operators to configure the cutoff interval for workspace agent logs.