Conversation
|
✅ Preview binaries are ready! To test with modules: |
There was a problem hiding this comment.
Pull request overview
This pull request integrates the coder/quartz library to provide a clock abstraction throughout the codebase, enabling better testability by allowing time-dependent code to use mock clocks in tests.
Changes:
- Added
github.com/coder/quartzdependency to replace directtimepackage usage - Replaced
time.Now(),time.Sleep(),time.After(), andtime.Since()calls with clock-based equivalents - Updated test files to use
quartz.NewMock(t)for deterministic time-based testing
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Added github.com/coder/quartz v0.1.2 dependency and removed unused go.uber.org/atomic |
| go.sum | Updated dependency checksums including quartz and transitive dependencies |
| lib/util/util.go | Added Clock field to WaitTimeout struct and replaced time operations with clock-based equivalents |
| lib/termexec/termexec.go | Added clock field to Process struct and replaced time operations with clock methods |
| lib/screentracker/conversation.go | Replaced GetTime function with Clock field and updated all time operations to use the clock |
| lib/screentracker/conversation_test.go | Updated tests to use mock clocks for deterministic testing |
| lib/httpapi/server.go | Added clock field to Server struct and updated ticker usage in snapshot loop |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/screentracker/conversation.go
Outdated
| time.Sleep(1 * time.Second) | ||
| timer := c.cfg.Clock.NewTimer(1 * time.Second) | ||
| defer timer.Stop() | ||
| <-timer.C |
There was a problem hiding this comment.
Suggestion: Maybe this could be a little helper? Say, sleep(clock, time.Second)? Seems re-usable (at least below) and clarifies the code.
Might be a useful addition to quartz. Having <-clock.After(time.Second) would also be nice.
| return nil | ||
| } | ||
| time.Sleep(interval) | ||
| sleepTimer := clock.NewTimer(interval) |
There was a problem hiding this comment.
Suggestion: Consider wrapping in if err := func() error { ... } so you can defer stop to avoid stopping on each case.
Alternatively, perhaps a helper function that contains the select since at least 2 are identical.
3 participants
Refactors existing usage of
time.Sleepetc. to usegithub.com/coder/quartz.🤖 Written by Claude Opus 4.5, reviewed by me.