From 306192ab7f2a856a42580bef9cbde5e72affd36c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 13:41:34 +0000 Subject: [PATCH 01/33] fix: stop registering a cleanup per timer fire Timer.fire registered a test cleanup each time a channel timer fired, from the goroutine that Advance starts. If the test stopped waiting on the Advance, for example because MustWait's context had expired, the fire could run after the test's cleanups, so its cleanup never ran and the goroutine writing to C leaked. Inside a testing/synctest bubble that panics with "main bubble goroutine has exited but blocked goroutines remain". Each fire also grew the test's cleanup list. The cleanup that NewMock registers now closes a channel that every pending write also selects on, and a timer that fires after it skips the write, so timer channels still cannot be read after the test ends. Pending writes now end when that cleanup runs, so cleanups registered after NewMock can still read a timer that has fired. --- mock.go | 6 ++ mock_test.go | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++ timer.go | 16 +++-- 3 files changed, 186 insertions(+), 6 deletions(-) diff --git a/mock.go b/mock.go index 7255aff..33bee22 100644 --- a/mock.go +++ b/mock.go @@ -30,6 +30,10 @@ type Mock struct { logger Logger mu sync.Mutex testOver bool + // done is closed by the cleanup registered in NewMock, which ends all pending timer channel + // writes. Timers can fire after the test's cleanups have run, so a cleanup registered by the + // fire itself would never run. + done chan struct{} // cur is the current time cur time.Time @@ -489,11 +493,13 @@ func NewMock(tb TestingT) *Mock { tb: tb, logger: tb, cur: cur, + done: make(chan struct{}), } tb.Cleanup(func() { m.mu.Lock() defer m.mu.Unlock() m.testOver = true + close(m.done) m.logger.Logf("Mock Clock - test cleanup; will no longer log clock events") }) return m diff --git a/mock_test.go b/mock_test.go index 7bc822d..47351c9 100644 --- a/mock_test.go +++ b/mock_test.go @@ -8,6 +8,7 @@ import ( "os" "runtime/pprof" "strings" + "sync" "testing" "time" @@ -496,6 +497,56 @@ func tRunFail(t testing.TB, f func(t testing.TB)) { } } +// cleanupTB is a quartz.TestingT that records Cleanup registrations so that a test can run them +// before it ends. Like testing.T, it never calls functions registered after its cleanups have run. +type cleanupTB struct { + testing.TB + + mu sync.Mutex + registered int + cleanups []func() + finished bool +} + +func newCleanupTB(t *testing.T) *cleanupTB { + tb := &cleanupTB{TB: t} + t.Cleanup(tb.runCleanups) + return tb +} + +func (c *cleanupTB) Cleanup(f func()) { + c.mu.Lock() + defer c.mu.Unlock() + c.registered++ + if !c.finished { + c.cleanups = append(c.cleanups, f) + } +} + +// registrations returns the number of Cleanup calls so far. +func (c *cleanupTB) registrations() int { + c.mu.Lock() + defer c.mu.Unlock() + return c.registered +} + +// runCleanups calls the registered functions in last added, first called order, including +// functions registered while it runs, as testing.T does when a test ends. +func (c *cleanupTB) runCleanups() { + for { + c.mu.Lock() + if len(c.cleanups) == 0 { + c.finished = true + c.mu.Unlock() + return + } + f := c.cleanups[len(c.cleanups)-1] + c.cleanups = c.cleanups[:len(c.cleanups)-1] + c.mu.Unlock() + f() + } +} + type testLogger struct { calls []string } @@ -603,6 +654,125 @@ func TestNoLeak_Go123(t *testing.T) { mClock.Advance(1 * time.Second).MustWait(ctx) } +// TestTimer_FireRegistersNoCleanup verifies that firing a channel timer does not register a test +// cleanup, so the cleanup list does not grow with the number of fires. +func TestTimer_FireRegistersNoCleanup(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + tb := newCleanupTB(t) + mClock := quartz.NewMock(tb).WithLogger(quartz.NoOpLogger) + tmr := mClock.NewTimer(time.Second) + before := tb.registrations() + const fires = 100 + for i := 0; i < fires; i++ { + mClock.Advance(time.Second).MustWait(ctx) + tmr.Reset(time.Second) + } + if got := tb.registrations() - before; got != 0 { + t.Fatalf("expected %d timer fires to register no cleanups, got %d", fires, got) + } +} + +// TestTimer_AbandonedAdvance verifies that the goroutine writing a fired timer's value to C exits +// when the test ends. A test can stop waiting on an Advance, for example when MustWait fails, so +// the timer can fire before or after the test's cleanups run. Calling Advance after running the +// cleanups makes the second ordering deterministic. +func TestTimer_AbandonedAdvance(t *testing.T) { + t.Parallel() + + t.Run("FiredBeforeCleanup", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + tb := newCleanupTB(t) + mClock := quartz.NewMock(tb) + _ = mClock.NewTimer(time.Second) + labeledAdvance(ctx, t.Name(), mClock, time.Second).MustWait(ctx) + // Only the goroutine blocked writing to the unread timer channel remains. + waitLabeledGoroutines(ctx, t, t.Name(), 1) + + tb.runCleanups() + waitLabeledGoroutines(ctx, t, t.Name(), 0) + }) + + t.Run("FiredAfterCleanup", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + tb := newCleanupTB(t) + mClock := quartz.NewMock(tb) + _ = mClock.NewTimer(time.Second) + + tb.runCleanups() + labeledAdvance(ctx, t.Name(), mClock, time.Second).MustWait(ctx) + waitLabeledGoroutines(ctx, t, t.Name(), 0) + }) +} + +// goroutineLabelKey is the pprof label key that labeledAdvance sets. +const goroutineLabelKey = "quartz_test" + +// labeledAdvance calls mClock.Advance with the pprof label goroutineLabelKey set to label. +// Goroutines inherit labels from the goroutine that starts them, so every goroutine that the +// Advance starts, directly or indirectly, carries the label. +func labeledAdvance( + ctx context.Context, label string, mClock *quartz.Mock, d time.Duration, +) quartz.AdvanceWaiter { + var w quartz.AdvanceWaiter + pprof.Do(ctx, pprof.Labels(goroutineLabelKey, label), func(context.Context) { + w = mClock.Advance(d) + }) + return w +} + +// waitLabeledGoroutines waits until exactly want goroutines carry the label set by +// labeledAdvance, and fails the test if ctx expires first. +func waitLabeledGoroutines(ctx context.Context, t testing.TB, label string, want int) { + t.Helper() + poll := time.NewTicker(time.Millisecond) + defer poll.Stop() + for { + got, stacks := labeledGoroutines(label) + if got == want { + return + } + select { + case <-ctx.Done(): + t.Fatalf("expected %d goroutines labeled %q, got %d:\n%s", want, label, got, stacks) + case <-poll.C: + } + } +} + +// labeledGoroutines returns the number of goroutines that carry the label set by labeledAdvance, +// and their stacks. +func labeledGoroutines(label string) (int, string) { + var buf bytes.Buffer + _ = pprof.Lookup("goroutine").WriteTo(&buf, 1) + // The first line is the profile header. Each record that follows starts with + // " @ " and lists its labels on a "# labels: " line. + _, body, _ := strings.Cut(buf.String(), "\n") + labelText := fmt.Sprintf("%q:%q", goroutineLabelKey, label) + var total int + var stacks []string + for _, record := range strings.Split(body, "\n\n") { + if !strings.Contains(record, labelText) { + continue + } + var n int + if _, err := fmt.Sscanf(record, "%d @", &n); err != nil { + panic(fmt.Sprintf("parse goroutine profile record %q: %v", record, err)) + } + total += n + stacks = append(stacks, record) + } + return total, strings.Join(stacks, "\n\n") +} + func TestTickerStop_Go123(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) diff --git a/timer.go b/timer.go index 0f5f733..8a62aa6 100644 --- a/timer.go +++ b/timer.go @@ -26,7 +26,8 @@ type Timer struct { // They also are not supposed to leak even if they are never read or stopped (Go runtime can garbage collect them). // We can't garbage-collect because we can't check if any other code besides the mock references, but we can ensure // that we don't leak goroutines so that the garbage collector can do its job when the mock is no longer - // referenced. The channels below allow us to interrupt the channel write goroutine. + // referenced. interrupt lets Stop and Reset interrupt the channel write goroutine, and + // mock.done interrupts it when the test ends. interrupt chan struct{} } @@ -38,12 +39,14 @@ func (t *Timer) fire(tt time.Time) { t.fn() return } else { + // Timer channel writes are abandoned when the test ends, so that the write goroutine does + // not leak beyond the test. Side effect is that timer channels cannot be read after the + // test exits. + if t.mock.testOver { + t.mock.mu.Unlock() + return + } interrupt := make(chan struct{}) - // Prevents the goroutine from leaking beyond the test. Side effect is that timer channels cannot be read - // after the test exits. - t.mock.tb.Cleanup(func() { - <-interrupt - }) t.interrupt = interrupt t.mock.mu.Unlock() go func() { @@ -51,6 +54,7 @@ func (t *Timer) fire(tt time.Time) { select { case t.c <- tt: case interrupt <- struct{}{}: + case <-t.mock.done: } }() } From 3484ca445f3e9e563d7413b3e0b02ada902cdfec Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 13:52:22 +0000 Subject: [PATCH 02/33] docs: state when timer channel writes end The comment in Timer.fire said pending writes end when the test ends. They end when the cleanup registered by NewMock runs, so cleanups registered before NewMock cannot read a fired timer, while cleanups registered after it can. The Mock.done comment also explained why there is no per-fire cleanup; it now only says what done does. --- mock.go | 4 +--- timer.go | 6 +++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/mock.go b/mock.go index 33bee22..03ed6dc 100644 --- a/mock.go +++ b/mock.go @@ -30,9 +30,7 @@ type Mock struct { logger Logger mu sync.Mutex testOver bool - // done is closed by the cleanup registered in NewMock, which ends all pending timer channel - // writes. Timers can fire after the test's cleanups have run, so a cleanup registered by the - // fire itself would never run. + // done is closed by the cleanup registered in NewMock to end all pending timer channel writes. done chan struct{} // cur is the current time diff --git a/timer.go b/timer.go index 8a62aa6..ad612e5 100644 --- a/timer.go +++ b/timer.go @@ -39,9 +39,9 @@ func (t *Timer) fire(tt time.Time) { t.fn() return } else { - // Timer channel writes are abandoned when the test ends, so that the write goroutine does - // not leak beyond the test. Side effect is that timer channels cannot be read after the - // test exits. + // Timer channel writes are abandoned when the cleanup registered by NewMock runs, so that + // the write goroutine does not leak beyond the test. Side effect is that timer channels + // cannot be read after that cleanup runs. if t.mock.testOver { t.mock.mu.Unlock() return From 5e51b02aa68995d09f4aced748caf48b4ce3504c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 13:52:22 +0000 Subject: [PATCH 03/33] test: make labeledAdvance a test helper pprof.Do adds a frame that testing cannot skip, so the mock's Advance log line pointed at the closure inside labeledAdvance, or at runtime/pprof when the closure was marked as a helper. Setting the labels with SetGoroutineLabels and marking labeledAdvance as a helper makes the log line point at the test. --- mock_test.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/mock_test.go b/mock_test.go index 47351c9..15d4bdf 100644 --- a/mock_test.go +++ b/mock_test.go @@ -690,7 +690,7 @@ func TestTimer_AbandonedAdvance(t *testing.T) { tb := newCleanupTB(t) mClock := quartz.NewMock(tb) _ = mClock.NewTimer(time.Second) - labeledAdvance(ctx, t.Name(), mClock, time.Second).MustWait(ctx) + labeledAdvance(ctx, t, t.Name(), mClock, time.Second).MustWait(ctx) // Only the goroutine blocked writing to the unread timer channel remains. waitLabeledGoroutines(ctx, t, t.Name(), 1) @@ -708,7 +708,7 @@ func TestTimer_AbandonedAdvance(t *testing.T) { _ = mClock.NewTimer(time.Second) tb.runCleanups() - labeledAdvance(ctx, t.Name(), mClock, time.Second).MustWait(ctx) + labeledAdvance(ctx, t, t.Name(), mClock, time.Second).MustWait(ctx) waitLabeledGoroutines(ctx, t, t.Name(), 0) }) } @@ -718,15 +718,16 @@ const goroutineLabelKey = "quartz_test" // labeledAdvance calls mClock.Advance with the pprof label goroutineLabelKey set to label. // Goroutines inherit labels from the goroutine that starts them, so every goroutine that the -// Advance starts, directly or indirectly, carries the label. +// Advance starts, directly or indirectly, carries the label. It does not use pprof.Do because +// the pprof.Do frame cannot be marked as a test helper, so the mock's log lines would not point +// at the caller. func labeledAdvance( - ctx context.Context, label string, mClock *quartz.Mock, d time.Duration, + ctx context.Context, t testing.TB, label string, mClock *quartz.Mock, d time.Duration, ) quartz.AdvanceWaiter { - var w quartz.AdvanceWaiter - pprof.Do(ctx, pprof.Labels(goroutineLabelKey, label), func(context.Context) { - w = mClock.Advance(d) - }) - return w + t.Helper() + pprof.SetGoroutineLabels(pprof.WithLabels(ctx, pprof.Labels(goroutineLabelKey, label))) + defer pprof.SetGoroutineLabels(ctx) + return mClock.Advance(d) } // waitLabeledGoroutines waits until exactly want goroutines carry the label set by From dd7c6bc012cd5b8c54d3a628f15cc35136e456e6 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 14:00:28 +0000 Subject: [PATCH 04/33] docs(timer.go): note pending writes can still be received The write goroutine may not have reached its select when NewMock's cleanup closes done, so a reader waiting at that moment can still get the value. The previous comment said channels cannot be read after the cleanup runs. --- timer.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/timer.go b/timer.go index ad612e5..3a81d36 100644 --- a/timer.go +++ b/timer.go @@ -39,9 +39,10 @@ func (t *Timer) fire(tt time.Time) { t.fn() return } else { - // Timer channel writes are abandoned when the cleanup registered by NewMock runs, so that - // the write goroutine does not leak beyond the test. Side effect is that timer channels - // cannot be read after that cleanup runs. + // The cleanup registered by NewMock ends pending timer channel writes, so the write + // goroutine does not leak beyond the test. A timer that fires after that cleanup has run + // is not written at all. A write already pending when it runs may still be received by a + // reader that is waiting at that moment. if t.mock.testOver { t.mock.mu.Unlock() return From 3c74741b5250f1e73f49b026e7da7089de843a88 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 14:37:17 +0000 Subject: [PATCH 05/33] test(mock_test.go): check late timer fires with the leak checker The pprof label helpers, cleanupTB, and the cleanup registration count tested how the fix works. verifyNoLeakTestMain already reports a write goroutine that outlives the tests, so TestNoLeak_Go123 now also fires a timer after the test that owns the Mock has ended. On 9591439 that subtest leaks the write goroutine and the leak check fails. Without the done case in the write select, FiredDuringTest fails the same way. --- mock_test.go | 182 +++++---------------------------------------------- 1 file changed, 16 insertions(+), 166 deletions(-) diff --git a/mock_test.go b/mock_test.go index 15d4bdf..6fb8def 100644 --- a/mock_test.go +++ b/mock_test.go @@ -8,7 +8,6 @@ import ( "os" "runtime/pprof" "strings" - "sync" "testing" "time" @@ -497,56 +496,6 @@ func tRunFail(t testing.TB, f func(t testing.TB)) { } } -// cleanupTB is a quartz.TestingT that records Cleanup registrations so that a test can run them -// before it ends. Like testing.T, it never calls functions registered after its cleanups have run. -type cleanupTB struct { - testing.TB - - mu sync.Mutex - registered int - cleanups []func() - finished bool -} - -func newCleanupTB(t *testing.T) *cleanupTB { - tb := &cleanupTB{TB: t} - t.Cleanup(tb.runCleanups) - return tb -} - -func (c *cleanupTB) Cleanup(f func()) { - c.mu.Lock() - defer c.mu.Unlock() - c.registered++ - if !c.finished { - c.cleanups = append(c.cleanups, f) - } -} - -// registrations returns the number of Cleanup calls so far. -func (c *cleanupTB) registrations() int { - c.mu.Lock() - defer c.mu.Unlock() - return c.registered -} - -// runCleanups calls the registered functions in last added, first called order, including -// functions registered while it runs, as testing.T does when a test ends. -func (c *cleanupTB) runCleanups() { - for { - c.mu.Lock() - if len(c.cleanups) == 0 { - c.finished = true - c.mu.Unlock() - return - } - f := c.cleanups[len(c.cleanups)-1] - c.cleanups = c.cleanups[:len(c.cleanups)-1] - c.mu.Unlock() - f() - } -} - type testLogger struct { calls []string } @@ -642,136 +591,37 @@ func TestTimerReset_NegativeDuration(t *testing.T) { } } +// TestNoLeak_Go123 relies on verifyNoLeakTestMain to report goroutines that outlive the tests. func TestNoLeak_Go123(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - mClock := quartz.NewMock(t) - - _ = mClock.NewTimer(1 * time.Second) - _ = mClock.NewTicker(1 * time.Second) - mClock.Advance(1 * time.Second).MustWait(ctx) -} - -// TestTimer_FireRegistersNoCleanup verifies that firing a channel timer does not register a test -// cleanup, so the cleanup list does not grow with the number of fires. -func TestTimer_FireRegistersNoCleanup(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - tb := newCleanupTB(t) - mClock := quartz.NewMock(tb).WithLogger(quartz.NoOpLogger) - tmr := mClock.NewTimer(time.Second) - before := tb.registrations() - const fires = 100 - for i := 0; i < fires; i++ { - mClock.Advance(time.Second).MustWait(ctx) - tmr.Reset(time.Second) - } - if got := tb.registrations() - before; got != 0 { - t.Fatalf("expected %d timer fires to register no cleanups, got %d", fires, got) - } -} - -// TestTimer_AbandonedAdvance verifies that the goroutine writing a fired timer's value to C exits -// when the test ends. A test can stop waiting on an Advance, for example when MustWait fails, so -// the timer can fire before or after the test's cleanups run. Calling Advance after running the -// cleanups makes the second ordering deterministic. -func TestTimer_AbandonedAdvance(t *testing.T) { - t.Parallel() - t.Run("FiredBeforeCleanup", func(t *testing.T) { + t.Run("FiredDuringTest", func(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - tb := newCleanupTB(t) - mClock := quartz.NewMock(tb) - _ = mClock.NewTimer(time.Second) - labeledAdvance(ctx, t, t.Name(), mClock, time.Second).MustWait(ctx) - // Only the goroutine blocked writing to the unread timer channel remains. - waitLabeledGoroutines(ctx, t, t.Name(), 1) + mClock := quartz.NewMock(t) - tb.runCleanups() - waitLabeledGoroutines(ctx, t, t.Name(), 0) + _ = mClock.NewTimer(1 * time.Second) + _ = mClock.NewTicker(1 * time.Second) + mClock.Advance(1 * time.Second).MustWait(ctx) }) - t.Run("FiredAfterCleanup", func(t *testing.T) { + // A test that stops waiting on an Advance can end before its timers fire. + t.Run("TimerFiredAfterTestEnds", func(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - tb := newCleanupTB(t) - mClock := quartz.NewMock(tb) - _ = mClock.NewTimer(time.Second) - - tb.runCleanups() - labeledAdvance(ctx, t, t.Name(), mClock, time.Second).MustWait(ctx) - waitLabeledGoroutines(ctx, t, t.Name(), 0) - }) -} - -// goroutineLabelKey is the pprof label key that labeledAdvance sets. -const goroutineLabelKey = "quartz_test" - -// labeledAdvance calls mClock.Advance with the pprof label goroutineLabelKey set to label. -// Goroutines inherit labels from the goroutine that starts them, so every goroutine that the -// Advance starts, directly or indirectly, carries the label. It does not use pprof.Do because -// the pprof.Do frame cannot be marked as a test helper, so the mock's log lines would not point -// at the caller. -func labeledAdvance( - ctx context.Context, t testing.TB, label string, mClock *quartz.Mock, d time.Duration, -) quartz.AdvanceWaiter { - t.Helper() - pprof.SetGoroutineLabels(pprof.WithLabels(ctx, pprof.Labels(goroutineLabelKey, label))) - defer pprof.SetGoroutineLabels(ctx) - return mClock.Advance(d) -} - -// waitLabeledGoroutines waits until exactly want goroutines carry the label set by -// labeledAdvance, and fails the test if ctx expires first. -func waitLabeledGoroutines(ctx context.Context, t testing.TB, label string, want int) { - t.Helper() - poll := time.NewTicker(time.Millisecond) - defer poll.Stop() - for { - got, stacks := labeledGoroutines(label) - if got == want { - return - } - select { - case <-ctx.Done(): - t.Fatalf("expected %d goroutines labeled %q, got %d:\n%s", want, label, got, stacks) - case <-poll.C: - } - } -} - -// labeledGoroutines returns the number of goroutines that carry the label set by labeledAdvance, -// and their stacks. -func labeledGoroutines(label string) (int, string) { - var buf bytes.Buffer - _ = pprof.Lookup("goroutine").WriteTo(&buf, 1) - // The first line is the profile header. Each record that follows starts with - // " @ " and lists its labels on a "# labels: " line. - _, body, _ := strings.Cut(buf.String(), "\n") - labelText := fmt.Sprintf("%q:%q", goroutineLabelKey, label) - var total int - var stacks []string - for _, record := range strings.Split(body, "\n\n") { - if !strings.Contains(record, labelText) { - continue - } - var n int - if _, err := fmt.Sscanf(record, "%d @", &n); err != nil { - panic(fmt.Sprintf("parse goroutine profile record %q: %v", record, err)) + var mClock *quartz.Mock + t.Run("Inner", func(t *testing.T) { + mClock = quartz.NewMock(t) + _ = mClock.NewTimer(time.Second) + }) + if err := mClock.Advance(time.Second).Wait(ctx); err != nil { + t.Fatal(err) } - total += n - stacks = append(stacks, record) - } - return total, strings.Join(stacks, "\n\n") + }) } func TestTickerStop_Go123(t *testing.T) { From 3615c486b924d43f2959cfc0d29d510a9bc33aeb Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 14:38:52 +0000 Subject: [PATCH 06/33] refactor(timer.go): drop the testOver skip in fire done already ends a write that starts after NewMock's cleanup has run, so the skip only decided whether a reader already waiting on C at that moment gets the value. No test covered it, and the leak tests pass, and fail when the done case is removed, without it. --- timer.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/timer.go b/timer.go index 3a81d36..fa8ef46 100644 --- a/timer.go +++ b/timer.go @@ -40,13 +40,8 @@ func (t *Timer) fire(tt time.Time) { return } else { // The cleanup registered by NewMock ends pending timer channel writes, so the write - // goroutine does not leak beyond the test. A timer that fires after that cleanup has run - // is not written at all. A write already pending when it runs may still be received by a - // reader that is waiting at that moment. - if t.mock.testOver { - t.mock.mu.Unlock() - return - } + // goroutine does not leak beyond the test. A write already pending when it runs may still + // be received by a reader that is waiting at that moment. interrupt := make(chan struct{}) t.interrupt = interrupt t.mock.mu.Unlock() From 6d95cb2b976c5ab962c81b35100b82e744c3585e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 14:39:23 +0000 Subject: [PATCH 07/33] docs: move the late write note to Mock.done The note that a write can still reach a reader already waiting on C describes done, so it now sits on the done field instead of in Timer.fire. The interrupt paragraph is rewrapped to 100 columns and names what ends the write goroutine. --- mock.go | 3 ++- timer.go | 13 +++++-------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/mock.go b/mock.go index 03ed6dc..1fe884c 100644 --- a/mock.go +++ b/mock.go @@ -30,7 +30,8 @@ type Mock struct { logger Logger mu sync.Mutex testOver bool - // done is closed by the cleanup registered in NewMock to end all pending timer channel writes. + // done is closed by the cleanup registered in NewMock to end timer channel writes. After that, + // a write can still reach a reader that is already waiting on C. done chan struct{} // cur is the current time diff --git a/timer.go b/timer.go index fa8ef46..282cb71 100644 --- a/timer.go +++ b/timer.go @@ -23,11 +23,11 @@ type Timer struct { // not to be reading. That means we need to start a new goroutine to do the channel write if we are a channel-based // timer. // - // They also are not supposed to leak even if they are never read or stopped (Go runtime can garbage collect them). - // We can't garbage-collect because we can't check if any other code besides the mock references, but we can ensure - // that we don't leak goroutines so that the garbage collector can do its job when the mock is no longer - // referenced. interrupt lets Stop and Reset interrupt the channel write goroutine, and - // mock.done interrupts it when the test ends. + // They also are not supposed to leak even if they are never read or stopped (Go runtime can + // garbage collect them). We can't garbage-collect because we can't check if any other code + // besides the mock references, but we can ensure that we don't leak goroutines so that the + // garbage collector can do its job when the mock is no longer referenced. interrupt lets Stop + // and Reset end the write goroutine; Mock.done ends it when NewMock's cleanup runs. interrupt chan struct{} } @@ -39,9 +39,6 @@ func (t *Timer) fire(tt time.Time) { t.fn() return } else { - // The cleanup registered by NewMock ends pending timer channel writes, so the write - // goroutine does not leak beyond the test. A write already pending when it runs may still - // be received by a reader that is waiting at that moment. interrupt := make(chan struct{}) t.interrupt = interrupt t.mock.mu.Unlock() From a459fae3bdacd04587aa6a00ae4c5e7a5732e15e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 20:40:21 +0000 Subject: [PATCH 08/33] test(mock_test.go): read a fired timer from a later cleanup Tests often stop a server in a cleanup registered after NewMock, and the server's workers may read a timer that fired while they were busy. On main, the cleanup that each fire registered ran before that cleanup and ended the write, so the worker blocked and the shutdown hung. TestTimer_ReadableDuringCleanup fails on main and passes here. --- mock_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/mock_test.go b/mock_test.go index 6fb8def..918f3d6 100644 --- a/mock_test.go +++ b/mock_test.go @@ -591,6 +591,35 @@ func TestTimerReset_NegativeDuration(t *testing.T) { } } +// TestTimer_ReadableDuringCleanup checks that a cleanup registered after NewMock, which runs before +// NewMock's cleanup, can still receive from a timer that fired during the test. Tests that stop a +// server in such a cleanup rely on this when a worker reads the timer on its way out. +func TestTimer_ReadableDuringCleanup(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + mClock := quartz.NewMock(t) + busy := make(chan struct{}) + exited := make(chan struct{}) + // Stands in for a server's Close: it unblocks the worker and waits for it to exit. + t.Cleanup(func() { + close(busy) + select { + case <-exited: + case <-time.After(10 * time.Second): + t.Error("worker did not receive the timer value during cleanup") + } + }) + tmr := mClock.NewTimer(time.Second) + go func() { + defer close(exited) + <-busy + <-tmr.C + }() + mClock.Advance(time.Second).MustWait(ctx) +} + // TestNoLeak_Go123 relies on verifyNoLeakTestMain to report goroutines that outlive the tests. func TestNoLeak_Go123(t *testing.T) { t.Parallel() From 87363a3ba14020f290adaf85b5b13ea68f348792 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 20:42:17 +0000 Subject: [PATCH 09/33] fix: send a fired timer's value to a waiting reader first A timer write that reaches its select after NewMock's cleanup has closed done, while a reader is already waiting on C, has two ready cases, and select picks one at random. In a probe, the waiting reader got the value in 93 of 200 runs. The write now tries a non-blocking send first, so a waiting reader always gets the value (200 of 200). A write with no waiting reader still ends on done, Stop or Reset, so it does not outlive the test, and no value can be received after Stop or Reset returns. --- mock.go | 5 +++-- timer.go | 8 ++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/mock.go b/mock.go index 1fe884c..a94dd1d 100644 --- a/mock.go +++ b/mock.go @@ -30,8 +30,9 @@ type Mock struct { logger Logger mu sync.Mutex testOver bool - // done is closed by the cleanup registered in NewMock to end timer channel writes. After that, - // a write can still reach a reader that is already waiting on C. + // done is closed by the cleanup registered in NewMock to end timer channel writes that are + // waiting for a reader. A reader that is already waiting on C when a write starts still gets + // the value, even after done is closed. done chan struct{} // cur is the current time diff --git a/timer.go b/timer.go index 282cb71..d8f7d94 100644 --- a/timer.go +++ b/timer.go @@ -44,6 +44,14 @@ func (t *Timer) fire(tt time.Time) { t.mock.mu.Unlock() go func() { defer close(interrupt) + // select picks at random among ready cases. Send first to a reader that is already + // waiting, so it gets the value even when done is closed or Stop or Reset is waiting on + // interrupt. + select { + case t.c <- tt: + return + default: + } select { case t.c <- tt: case interrupt <- struct{}{}: From cfea58d4e5437b93ccc2616108f8784fd17531b8 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 20:42:17 +0000 Subject: [PATCH 10/33] test(mock_go125_test.go): deliver to readers waiting at Mock cleanup Without synctest, a test cannot know that a reader is waiting on C before the write starts, so the check would be flaky. synctest.Test is stable since Go 1.25 and go.mod says 1.23, hence the build tag. --- mock_go125_test.go | 50 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 mock_go125_test.go diff --git a/mock_go125_test.go b/mock_go125_test.go new file mode 100644 index 0000000..22e7a58 --- /dev/null +++ b/mock_go125_test.go @@ -0,0 +1,50 @@ +//go:build go1.25 + +package quartz_test + +import ( + "testing" + "testing/synctest" + "time" + + "github.com/coder/quartz" +) + +// TestTimer_ReaderWaitingAtMockCleanup checks that a reader waiting on C when NewMock's cleanup +// runs still receives a timer value written after that cleanup, as happens when the write's +// goroutine is scheduled late. The timers fire from a later cleanup to get that order on every +// run. synctest.Wait makes sure the readers are waiting before NewMock's cleanup runs and that +// every write has ended before the check. +func TestTimer_ReaderWaitingAtMockCleanup(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + // select picks at random among ready cases, so a write that does not try the waiting reader + // first still reaches it half the time. With 10 timers, the test then fails in all but 1 of + // 1024 runs. + const n = 10 + var mClock *quartz.Mock + received := make(chan struct{}, n) + quit := make(chan struct{}) + // Registered before NewMock, so it runs after NewMock's cleanup. + t.Cleanup(func() { + mClock.Advance(time.Second) + synctest.Wait() + close(quit) + if got := len(received); got != n { + t.Errorf("%d of %d waiting readers received the timer value", got, n) + } + }) + mClock = quartz.NewMock(t) + for range n { + tmr := mClock.NewTimer(time.Second) + go func() { + select { + case <-tmr.C: + received <- struct{}{} + case <-quit: + } + }() + } + synctest.Wait() + }) +} From eab5edf579308d2f2ee313aad58a44853bad4856 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 21:12:26 +0000 Subject: [PATCH 11/33] fix: end pending timer writes through their interrupt channels Mock.done was created by NewMock, outside any synctest bubble. When a Mock created with the outer test's t had a timer created and fired inside a bubble, the write goroutine also waited on done, which synctest does not count as durably blocked, so the test hung until go test's timeout instead of panicking at once as on main. NewMock's cleanup now ends each pending write by receiving on that timer's interrupt channel, as Stop does. That channel is created where the timer fires, so it belongs to the bubble. The Mock tracks timers with a write in flight, and a write that delivers its value removes its timer. A timer that fires after the cleanup starts no goroutine and only reaches a reader that is already waiting. --- mock.go | 13 +++++++------ timer.go | 55 ++++++++++++++++++++++++++++++++++++++----------------- 2 files changed, 45 insertions(+), 23 deletions(-) diff --git a/mock.go b/mock.go index a94dd1d..d38e50f 100644 --- a/mock.go +++ b/mock.go @@ -30,10 +30,9 @@ type Mock struct { logger Logger mu sync.Mutex testOver bool - // done is closed by the cleanup registered in NewMock to end timer channel writes that are - // waiting for a reader. A reader that is already waiting on C when a write starts still gets - // the value, even after done is closed. - done chan struct{} + // writes holds the timers that have a goroutine sending their fired value on C. The cleanup + // registered in NewMock ends those goroutines. + writes map[*Timer]struct{} // cur is the current time cur time.Time @@ -493,13 +492,15 @@ func NewMock(tb TestingT) *Mock { tb: tb, logger: tb, cur: cur, - done: make(chan struct{}), + writes: make(map[*Timer]struct{}), } tb.Cleanup(func() { m.mu.Lock() defer m.mu.Unlock() m.testOver = true - close(m.done) + for t := range m.writes { + t.endWriteLocked() + } m.logger.Logf("Mock Clock - test cleanup; will no longer log clock events") }) return m diff --git a/timer.go b/timer.go index d8f7d94..1315a76 100644 --- a/timer.go +++ b/timer.go @@ -26,8 +26,8 @@ type Timer struct { // They also are not supposed to leak even if they are never read or stopped (Go runtime can // garbage collect them). We can't garbage-collect because we can't check if any other code // besides the mock references, but we can ensure that we don't leak goroutines so that the - // garbage collector can do its job when the mock is no longer referenced. interrupt lets Stop - // and Reset end the write goroutine; Mock.done ends it when NewMock's cleanup runs. + // garbage collector can do its job when the mock is no longer referenced. interrupt lets Stop, + // Reset and the cleanup registered by NewMock end the write goroutine. interrupt chan struct{} } @@ -38,29 +38,56 @@ func (t *Timer) fire(tt time.Time) { t.mock.mu.Unlock() t.fn() return + } else if t.mock.testOver { + t.mock.mu.Unlock() + // NewMock's cleanup has run and would not end a write goroutine, so only a reader that is + // already waiting gets the value. + select { + case t.c <- tt: + default: + } } else { interrupt := make(chan struct{}) t.interrupt = interrupt + t.mock.writes[t] = struct{}{} t.mock.mu.Unlock() go func() { - defer close(interrupt) // select picks at random among ready cases. Send first to a reader that is already - // waiting, so it gets the value even when done is closed or Stop or Reset is waiting on + // waiting, so it gets the value even when Stop, Reset or NewMock's cleanup is waiting on // interrupt. select { case t.c <- tt: - return default: + select { + case t.c <- tt: + case interrupt <- struct{}{}: + // endWriteLocked received this and removed the write. + return + } } - select { - case t.c <- tt: - case interrupt <- struct{}{}: - case <-t.mock.done: + // Close before locking: endWriteLocked waits on interrupt with the lock held. + close(interrupt) + t.mock.mu.Lock() + defer t.mock.mu.Unlock() + // Otherwise endWriteLocked has removed this write, and Reset may have started another. + if t.interrupt == interrupt { + t.interrupt = nil + delete(t.mock.writes, t) } }() } } +// endWriteLocked ends the goroutine that fire started to send on C, if it has not ended yet. +func (t *Timer) endWriteLocked() { + if t.interrupt == nil { + return + } + <-t.interrupt + t.interrupt = nil + delete(t.mock.writes, t) +} + func (t *Timer) next() time.Time { return t.nxt } @@ -82,10 +109,7 @@ func (t *Timer) Stop(tags ...string) bool { result := !t.stopped t.mock.removeTimerLocked(t) // check if we've already fired, and if so, interrupt it. - if t.interrupt != nil { - <-t.interrupt - t.interrupt = nil - } + t.endWriteLocked() return result } @@ -104,10 +128,7 @@ func (t *Timer) Reset(d time.Duration, tags ...string) bool { defer close(c.complete) result := !t.stopped // check if we've already fired, and if so, interrupt it. - if t.interrupt != nil { - <-t.interrupt - t.interrupt = nil - } + t.endWriteLocked() // Remove the timer from the mock's event list synchronously; otherwise the // stale old schedule can cause e.g. Advance() to reject advancing past the // old nxt, even though the timer has been reset. From 76cc0ae67570060f9faae4778b72b544f48323a3 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 21:12:43 +0000 Subject: [PATCH 12/33] docs(mock.go): say when NewMock's cleanup drops timer values Cleanups run in reverse order, so a cleanup registered before NewMock runs after the pending timer writes have ended. If it waits for code that reads a fired timer, that code never gets the value and the wait hangs. The NewMock doc now says what is dropped and which order works. --- mock.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mock.go b/mock.go index d38e50f..6755410 100644 --- a/mock.go +++ b/mock.go @@ -483,6 +483,9 @@ func (m *Mock) WithLogger(l Logger) *Mock { // NewMock creates a new Mock with the time set to midnight UTC on Jan 1, 2024. // You may re-set the time earlier than this, but only before timers or tickers // are created. +// +// A timer value that has not been received when the cleanup registered by NewMock runs is dropped. +// Call NewMock before registering cleanups that wait for code reading the Mock's timers. func NewMock(tb TestingT) *Mock { cur, err := time.Parse(time.RFC3339, "2024-01-01T00:00:00Z") if err != nil { From 75d90414b6dcddf0b00035b76f4493cd5f067921 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 21:12:59 +0000 Subject: [PATCH 13/33] docs(timer.go): wrap the Timer channel comment to 100 columns --- timer.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/timer.go b/timer.go index 1315a76..0d05295 100644 --- a/timer.go +++ b/timer.go @@ -17,11 +17,12 @@ type Timer struct { fn func() // AfterFunc function, if set stopped bool // True if stopped, false if running - // As of Go 1.23, timer channels are unbuffered and guaranteed to block forever after a call to stop. + // As of Go 1.23, timer channels are unbuffered and guaranteed to block forever after a call to + // stop. // - // When a mocked timer fires, we don't want to block on a channel write, because it's fine for the code under test - // not to be reading. That means we need to start a new goroutine to do the channel write if we are a channel-based - // timer. + // When a mocked timer fires, we don't want to block on a channel write, because it's fine for + // the code under test not to be reading. That means we need to start a new goroutine to do the + // channel write if we are a channel-based timer. // // They also are not supposed to leak even if they are never read or stopped (Go runtime can // garbage collect them). We can't garbage-collect because we can't check if any other code From 03a421a77c46011d558f0b50539c347d91436288 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 21:20:25 +0000 Subject: [PATCH 14/33] test(mock_go125_test.go): fire before and after NewMock's cleanup A timer that fires after NewMock's cleanup no longer starts a write goroutine, so this test stopped reaching the send that tries a waiting reader first, and removing that send passed. The FiredBeforeCleanup row fires the timer as the test's last step: in a bubble the write goroutine often starts after the cleanup waits on interrupt. Without the first send it fails 200 of 200 runs, and about half of them with -race. FiredAfterCleanup fails every run when fire sends nothing after the cleanup. Each round is a new bubble with one timer. Ten timers in one bubble failed only 78 of 200 runs without the first send, because only the write the cleanup ends first meets a waiting interrupt. --- mock_go125_test.go | 82 ++++++++++++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 32 deletions(-) diff --git a/mock_go125_test.go b/mock_go125_test.go index 22e7a58..83ea02f 100644 --- a/mock_go125_test.go +++ b/mock_go125_test.go @@ -3,6 +3,7 @@ package quartz_test import ( + "context" "testing" "testing/synctest" "time" @@ -11,40 +12,57 @@ import ( ) // TestTimer_ReaderWaitingAtMockCleanup checks that a reader waiting on C when NewMock's cleanup -// runs still receives a timer value written after that cleanup, as happens when the write's -// goroutine is scheduled late. The timers fire from a later cleanup to get that order on every -// run. synctest.Wait makes sure the readers are waiting before NewMock's cleanup runs and that -// every write has ended before the check. +// runs receives the value of a timer that fires just before or after that cleanup. synctest.Wait +// makes sure the reader is waiting before the timer fires and that the send has ended before the +// check. func TestTimer_ReaderWaitingAtMockCleanup(t *testing.T) { t.Parallel() - synctest.Test(t, func(t *testing.T) { - // select picks at random among ready cases, so a write that does not try the waiting reader - // first still reaches it half the time. With 10 timers, the test then fails in all but 1 of - // 1024 runs. - const n = 10 - var mClock *quartz.Mock - received := make(chan struct{}, n) - quit := make(chan struct{}) - // Registered before NewMock, so it runs after NewMock's cleanup. - t.Cleanup(func() { - mClock.Advance(time.Second) - synctest.Wait() - close(quit) - if got := len(received); got != n { - t.Errorf("%d of %d waiting readers received the timer value", got, n) + for _, tc := range []struct { + name string + fireAfterCleanup bool + }{ + // The test fires the timer as its last step. In a bubble, the goroutine that sends the + // value often starts only after NewMock's cleanup is waiting to end it. + {name: "FiredBeforeCleanup"}, + {name: "FiredAfterCleanup", fireAfterCleanup: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + // When the sending goroutine starts late, select picks at random between the reader + // and the cleanup, so a send that does not try the reader first loses the value about + // half the time. Each round is a new bubble, so without the race detector, which + // changes scheduling, the test then fails in nearly every run. + for range 10 { + synctest.Test(t, func(t *testing.T) { + var mClock *quartz.Mock + received := make(chan struct{}, 1) + quit := make(chan struct{}) + // Registered before NewMock, so it runs after NewMock's cleanup. + t.Cleanup(func() { + if tc.fireAfterCleanup { + mClock.Advance(time.Second) + } + synctest.Wait() + close(quit) + if len(received) == 0 { + t.Error("the waiting reader did not receive the timer value") + } + }) + mClock = quartz.NewMock(t) + tmr := mClock.NewTimer(time.Second) + go func() { + select { + case <-tmr.C: + received <- struct{}{} + case <-quit: + } + }() + synctest.Wait() + if !tc.fireAfterCleanup { + mClock.Advance(time.Second).MustWait(context.Background()) + } + }) } }) - mClock = quartz.NewMock(t) - for range n { - tmr := mClock.NewTimer(time.Second) - go func() { - select { - case <-tmr.C: - received <- struct{}{} - case <-quit: - } - }() - } - synctest.Wait() - }) + } } From d74f980dfa064f23266b8a4635bc6f492fc77102 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 21:21:39 +0000 Subject: [PATCH 15/33] docs(timer.go): wrap the write goroutine comments to 100 columns One line was 101 columns. The comment above the interrupt check started with "Otherwise", which reads as a note on the check's else branch; it now names the condition it describes. --- timer.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/timer.go b/timer.go index 0d05295..587d249 100644 --- a/timer.go +++ b/timer.go @@ -54,8 +54,8 @@ func (t *Timer) fire(tt time.Time) { t.mock.mu.Unlock() go func() { // select picks at random among ready cases. Send first to a reader that is already - // waiting, so it gets the value even when Stop, Reset or NewMock's cleanup is waiting on - // interrupt. + // waiting, so it gets the value even when Stop, Reset or NewMock's cleanup is waiting + // on interrupt. select { case t.c <- tt: default: @@ -70,7 +70,8 @@ func (t *Timer) fire(tt time.Time) { close(interrupt) t.mock.mu.Lock() defer t.mock.mu.Unlock() - // Otherwise endWriteLocked has removed this write, and Reset may have started another. + // If interrupt has changed, endWriteLocked has removed this write, and Reset may have + // started another. if t.interrupt == interrupt { t.interrupt = nil delete(t.mock.writes, t) From f4ab3dbe4c15ab93381f1cdb1e87ff335eb91a4b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 21:40:24 +0000 Subject: [PATCH 16/33] fix: end a timer's earlier write when it fires again NewTimer and Reset fire a timer with a non-positive duration from a new goroutine, so NewTimer(0) followed by Reset(0) often fires the timer twice before the first write ends. The second fire replaced interrupt, and the Mock tracks one write per timer, so nothing ended the first write and it leaked: 93 to 99 of 100 rounds in the new TimerFiredTwice subtest. fire now ends the earlier write first, as Reset does. --- mock_test.go | 10 ++++++++++ timer.go | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/mock_test.go b/mock_test.go index 918f3d6..173a913 100644 --- a/mock_test.go +++ b/mock_test.go @@ -636,6 +636,16 @@ func TestNoLeak_Go123(t *testing.T) { mClock.Advance(1 * time.Second).MustWait(ctx) }) + // NewTimer and Reset fire a timer with a non-positive duration from a new goroutine, so both + // fires can start a write before either ends. Over 100 rounds, that happens in most of them. + t.Run("TimerFiredTwice", func(t *testing.T) { + t.Parallel() + for range 100 { + tmr := quartz.NewMock(t).WithLogger(quartz.NoOpLogger).NewTimer(0) + tmr.Reset(0) + } + }) + // A test that stops waiting on an Advance can end before its timers fire. t.Run("TimerFiredAfterTestEnds", func(t *testing.T) { t.Parallel() diff --git a/timer.go b/timer.go index 587d249..ad7aa72 100644 --- a/timer.go +++ b/timer.go @@ -48,6 +48,10 @@ func (t *Timer) fire(tt time.Time) { default: } } else { + // Reset and NewTimer fire a timer with a non-positive duration from a new goroutine, so the + // timer can fire again before the write from an earlier fire has ended. writes tracks one + // write per timer, so end the earlier one, as Reset would. + t.endWriteLocked() interrupt := make(chan struct{}) t.interrupt = interrupt t.mock.writes[t] = struct{}{} From 4b8ecff1ca56f78ead6239ecd33e0acdee1d31b1 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 12:46:43 +0000 Subject: [PATCH 17/33] test: fold mock_go125_test.go into mock_test.go go.mod now requires Go 1.26, so testing/synctest is always available and the go1.25 build tag no longer selects anything. --- mock_go125_test.go | 68 ---------------------------------------------- mock_test.go | 57 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 68 deletions(-) delete mode 100644 mock_go125_test.go diff --git a/mock_go125_test.go b/mock_go125_test.go deleted file mode 100644 index 83ea02f..0000000 --- a/mock_go125_test.go +++ /dev/null @@ -1,68 +0,0 @@ -//go:build go1.25 - -package quartz_test - -import ( - "context" - "testing" - "testing/synctest" - "time" - - "github.com/coder/quartz" -) - -// TestTimer_ReaderWaitingAtMockCleanup checks that a reader waiting on C when NewMock's cleanup -// runs receives the value of a timer that fires just before or after that cleanup. synctest.Wait -// makes sure the reader is waiting before the timer fires and that the send has ended before the -// check. -func TestTimer_ReaderWaitingAtMockCleanup(t *testing.T) { - t.Parallel() - for _, tc := range []struct { - name string - fireAfterCleanup bool - }{ - // The test fires the timer as its last step. In a bubble, the goroutine that sends the - // value often starts only after NewMock's cleanup is waiting to end it. - {name: "FiredBeforeCleanup"}, - {name: "FiredAfterCleanup", fireAfterCleanup: true}, - } { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - // When the sending goroutine starts late, select picks at random between the reader - // and the cleanup, so a send that does not try the reader first loses the value about - // half the time. Each round is a new bubble, so without the race detector, which - // changes scheduling, the test then fails in nearly every run. - for range 10 { - synctest.Test(t, func(t *testing.T) { - var mClock *quartz.Mock - received := make(chan struct{}, 1) - quit := make(chan struct{}) - // Registered before NewMock, so it runs after NewMock's cleanup. - t.Cleanup(func() { - if tc.fireAfterCleanup { - mClock.Advance(time.Second) - } - synctest.Wait() - close(quit) - if len(received) == 0 { - t.Error("the waiting reader did not receive the timer value") - } - }) - mClock = quartz.NewMock(t) - tmr := mClock.NewTimer(time.Second) - go func() { - select { - case <-tmr.C: - received <- struct{}{} - case <-quit: - } - }() - synctest.Wait() - if !tc.fireAfterCleanup { - mClock.Advance(time.Second).MustWait(context.Background()) - } - }) - } - }) - } -} diff --git a/mock_test.go b/mock_test.go index 173a913..96b386e 100644 --- a/mock_test.go +++ b/mock_test.go @@ -9,6 +9,7 @@ import ( "runtime/pprof" "strings" "testing" + "testing/synctest" "time" "github.com/coder/quartz" @@ -620,6 +621,62 @@ func TestTimer_ReadableDuringCleanup(t *testing.T) { mClock.Advance(time.Second).MustWait(ctx) } +// TestTimer_ReaderWaitingAtMockCleanup checks that a reader waiting on C when NewMock's cleanup +// runs receives the value of a timer that fires just before or after that cleanup. synctest.Wait +// makes sure the reader is waiting before the timer fires and that the send has ended before the +// check. +func TestTimer_ReaderWaitingAtMockCleanup(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + fireAfterCleanup bool + }{ + // The test fires the timer as its last step. In a bubble, the goroutine that sends the + // value often starts only after NewMock's cleanup is waiting to end it. + {name: "FiredBeforeCleanup"}, + {name: "FiredAfterCleanup", fireAfterCleanup: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + // When the sending goroutine starts late, select picks at random between the reader + // and the cleanup, so a send that does not try the reader first loses the value about + // half the time. Each round is a new bubble, so without the race detector, which + // changes scheduling, the test then fails in nearly every run. + for range 10 { + synctest.Test(t, func(t *testing.T) { + var mClock *quartz.Mock + received := make(chan struct{}, 1) + quit := make(chan struct{}) + // Registered before NewMock, so it runs after NewMock's cleanup. + t.Cleanup(func() { + if tc.fireAfterCleanup { + mClock.Advance(time.Second) + } + synctest.Wait() + close(quit) + if len(received) == 0 { + t.Error("the waiting reader did not receive the timer value") + } + }) + mClock = quartz.NewMock(t) + tmr := mClock.NewTimer(time.Second) + go func() { + select { + case <-tmr.C: + received <- struct{}{} + case <-quit: + } + }() + synctest.Wait() + if !tc.fireAfterCleanup { + mClock.Advance(time.Second).MustWait(context.Background()) + } + }) + } + }) + } +} + // TestNoLeak_Go123 relies on verifyNoLeakTestMain to report goroutines that outlive the tests. func TestNoLeak_Go123(t *testing.T) { t.Parallel() From e6cbe10788fc962f23262ac6b99397a663a2f55c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 12:47:58 +0000 Subject: [PATCH 18/33] test(mock_test.go): wait on bubble time in two cleanup tests TestTickerFunc_ContextDoneDuringTick checked that Wait had not returned by waiting 100ms of wall time, which passes when Wait is merely slow. In a synctest bubble, synctest.Wait returns once every goroutine is blocked, so the check runs when Wait has either returned or is waiting for the callback. TestTimer_ReadableDuringCleanup waited 10 seconds before reporting a worker that never received. Bubble time moves as soon as every goroutine is blocked, so the failure is reported at once, and a quit channel lets the stuck worker exit so the bubble can end. --- mock_test.go | 134 ++++++++++++++++++++++++++++----------------------- 1 file changed, 74 insertions(+), 60 deletions(-) diff --git a/mock_test.go b/mock_test.go index 96b386e..dfdd30e 100644 --- a/mock_test.go +++ b/mock_test.go @@ -222,51 +222,56 @@ func TestPeek(t *testing.T) { // function callback is in progress. func TestTickerFunc_ContextDoneDuringTick(t *testing.T) { t.Parallel() - testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second) - defer testCancel() - mClock := quartz.NewMock(t) + synctest.Test(t, func(t *testing.T) { + testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer testCancel() + mClock := quartz.NewMock(t) - tickStart := make(chan struct{}) - tickDone := make(chan struct{}) - ctx, cancel := context.WithCancel(testCtx) - defer cancel() - tkr := mClock.TickerFunc(ctx, time.Second, func() error { - close(tickStart) + tickStart := make(chan struct{}) + tickDone := make(chan struct{}) + ctx, cancel := context.WithCancel(testCtx) + defer cancel() + tkr := mClock.TickerFunc(ctx, time.Second, func() error { + close(tickStart) + select { + case <-tickDone: + case <-testCtx.Done(): + t.Error("timeout waiting for tickDone") + } + return nil + }) + w := mClock.Advance(time.Second) select { - case <-tickDone: + case <-tickStart: + // OK case <-testCtx.Done(): - t.Error("timeout waiting for tickDone") + t.Fatal("timeout waiting for tickStart") } - return nil - }) - w := mClock.Advance(time.Second) - select { - case <-tickStart: - // OK - case <-testCtx.Done(): - t.Fatal("timeout waiting for tickStart") - } - waitErr := make(chan error, 1) - go func() { - waitErr <- tkr.Wait() - }() - cancel() - select { - case <-waitErr: - t.Fatal("wait should not return while tick callback in progress") - case <-time.After(time.Millisecond * 100): - // OK - } - close(tickDone) - select { - case err := <-waitErr: - if !errors.Is(err, context.Canceled) { - t.Fatal("expected context.Canceled error") + waitErr := make(chan error, 1) + go func() { + waitErr <- tkr.Wait() + }() + cancel() + // Once every goroutine in the bubble is blocked, Wait has either returned or is waiting + // for the callback. + synctest.Wait() + select { + case <-waitErr: + t.Fatal("wait should not return while tick callback in progress") + default: + // OK } - case <-testCtx.Done(): - t.Fatal("timed out waiting for wait to finish") - } - w.MustWait(testCtx) + close(tickDone) + select { + case err := <-waitErr: + if !errors.Is(err, context.Canceled) { + t.Fatal("expected context.Canceled error") + } + case <-testCtx.Done(): + t.Fatal("timed out waiting for wait to finish") + } + w.MustWait(testCtx) + }) } // TestTickerFunc_LongCallback tests that we don't call the ticker func a second time while the @@ -597,28 +602,37 @@ func TestTimerReset_NegativeDuration(t *testing.T) { // server in such a cleanup rely on this when a worker reads the timer on its way out. func TestTimer_ReadableDuringCleanup(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() + synctest.Test(t, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() - mClock := quartz.NewMock(t) - busy := make(chan struct{}) - exited := make(chan struct{}) - // Stands in for a server's Close: it unblocks the worker and waits for it to exit. - t.Cleanup(func() { - close(busy) - select { - case <-exited: - case <-time.After(10 * time.Second): - t.Error("worker did not receive the timer value during cleanup") - } + mClock := quartz.NewMock(t) + busy := make(chan struct{}) + quit := make(chan struct{}) + exited := make(chan struct{}) + // Stands in for a server's Close: it unblocks the worker and waits for it to exit. Bubble + // time moves once every goroutine is blocked, so a worker that never receives fails the + // test at once. quit then lets the worker exit, so the bubble can end. + t.Cleanup(func() { + close(busy) + select { + case <-exited: + case <-time.After(10 * time.Second): + t.Error("worker did not receive the timer value during cleanup") + } + close(quit) + }) + tmr := mClock.NewTimer(time.Second) + go func() { + defer close(exited) + <-busy + select { + case <-tmr.C: + case <-quit: + } + }() + mClock.Advance(time.Second).MustWait(ctx) }) - tmr := mClock.NewTimer(time.Second) - go func() { - defer close(exited) - <-busy - <-tmr.C - }() - mClock.Advance(time.Second).MustWait(ctx) } // TestTimer_ReaderWaitingAtMockCleanup checks that a reader waiting on C when NewMock's cleanup From cc83997a47ea679431032ce20a2224d4ece88688 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 12:49:22 +0000 Subject: [PATCH 19/33] fix: report a fired, unread timer value from Stop and Reset A time.Timer whose value has not been received counts as active since Go 1.23: Stop and Reset discard the value and return true. The mock returned false, so the idiom `if !t.Stop() { <-t.C }` blocked forever after a fire, because Stop had already ended the write. coder/coder uses that idiom in coderd/database/pubsub/watchdog.go and enterprise/coderd/usage/generator.go. Stop and Reset now return true when they end a write that had not sent the value. TestTimerStop_Go123 and TestTimerReset_Go123 asserted the old results; TestTimer_StopAndReset replaces them with the results of a time.Timer on Go 1.26 and adds a row for the idiom. --- mock_test.go | 93 +++++++++++++++++++++++++++++++--------------------- timer.go | 30 ++++++++++------- 2 files changed, 75 insertions(+), 48 deletions(-) diff --git a/mock_test.go b/mock_test.go index dfdd30e..507dd42 100644 --- a/mock_test.go +++ b/mock_test.go @@ -514,45 +514,64 @@ func (l *testLogger) Logf(format string, args ...any) { l.calls = append(l.calls, fmt.Sprintf(format, args...)) } -func TestTimerStop_Go123(t *testing.T) { +// TestTimer_StopAndReset checks what Stop and Reset return for a timer that has fired, and that +// no value is received after they return, as with a time.Timer. +func TestTimer_StopAndReset(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - mClock := quartz.NewMock(t) - - tmr := mClock.NewTimer(1 * time.Second) - mClock.Advance(1 * time.Second).MustWait(ctx) - if tmr.Stop() { - t.Fatal("timer hadn't already been active") - } - - select { - case tme := <-tmr.C: - t.Fatalf("got channel read after stop: %s", tme) - default: - // OK! - } -} - -func TestTimerReset_Go123(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - mClock := quartz.NewMock(t) - - tmr := mClock.NewTimer(1 * time.Second) - mClock.Advance(1 * time.Second).MustWait(ctx) - if tmr.Reset(1 * time.Second) { - t.Fatal("timer hadn't already been active") + fired := func(ctx context.Context, m *quartz.Mock) *quartz.Timer { + tmr := m.NewTimer(time.Second) + m.Advance(time.Second).MustWait(ctx) + return tmr + } + firedAndReceived := func(ctx context.Context, m *quartz.Mock) *quartz.Timer { + tmr := fired(ctx, m) + <-tmr.C + return tmr + } + stop := func(tmr *quartz.Timer) bool { return tmr.Stop() } + reset := func(tmr *quartz.Timer) bool { return tmr.Reset(time.Hour) } + // stopAndDrain is the way to stop a timer before Go 1.23. It returns false if the receive + // blocks, which bubble time reports at once. + stopAndDrain := func(tmr *quartz.Timer) bool { + if !tmr.Stop() { + select { + case <-tmr.C: + case <-time.After(time.Second): + return false + } + } + return true } - - select { - case tme := <-tmr.C: - t.Fatalf("got channel read after stop: %s", tme) - default: - // OK! + for _, tc := range []struct { + name string + start func(context.Context, *quartz.Mock) *quartz.Timer + call func(*quartz.Timer) bool + want bool + }{ + {name: "StopFiredUnread", start: fired, call: stop, want: true}, + {name: "StopFiredReceived", start: firedAndReceived, call: stop, want: false}, + {name: "ResetFiredUnread", start: fired, call: reset, want: true}, + {name: "StopAndDrainFiredUnread", start: fired, call: stopAndDrain, want: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + tmr := tc.start(ctx, quartz.NewMock(t)) + if got := tc.call(tmr); got != tc.want { + t.Errorf("got %v, want %v", got, tc.want) + } + // Lets any goroutine that could still send on C run before the check. + synctest.Wait() + select { + case tme := <-tmr.C: + t.Errorf("received %s after the call returned", tme) + default: + } + }) + }) } } diff --git a/timer.go b/timer.go index ad7aa72..7887ac5 100644 --- a/timer.go +++ b/timer.go @@ -84,14 +84,18 @@ func (t *Timer) fire(tt time.Time) { } } -// endWriteLocked ends the goroutine that fire started to send on C, if it has not ended yet. -func (t *Timer) endWriteLocked() { +// endWriteLocked ends the goroutine that fire started to send on C, if it has not ended yet. It +// reports whether that goroutine ended without sending the value. +func (t *Timer) endWriteLocked() bool { if t.interrupt == nil { - return + return false } - <-t.interrupt + // The goroutine sends on interrupt when it ends without sending the value, and closes + // interrupt once it has sent the value. + _, unsent := <-t.interrupt t.interrupt = nil delete(t.mock.writes, t) + return unsent } func (t *Timer) next() time.Time { @@ -99,8 +103,9 @@ func (t *Timer) next() time.Time { } // Stop prevents the Timer from firing. It returns true if the call stops the timer, false if the -// timer has already expired or been stopped. Stop does not close the channel, to prevent a read -// from the channel succeeding incorrectly. +// timer has already expired or been stopped. As with a time.Timer, a timer whose value has not been +// received has not expired: Stop discards the value and returns true. Stop does not close the +// channel, to prevent a read from the channel succeeding incorrectly. // // See https://pkg.go.dev/time#Timer.Stop for more information. func (t *Timer) Stop(tags ...string) bool { @@ -114,13 +119,15 @@ func (t *Timer) Stop(tags ...string) bool { defer close(c.complete) result := !t.stopped t.mock.removeTimerLocked(t) - // check if we've already fired, and if so, interrupt it. - t.endWriteLocked() + if t.endWriteLocked() { + result = true + } return result } // Reset changes the timer to expire after duration d. It returns true if the timer had been active, -// false if the timer had expired or been stopped. +// false if the timer had expired or been stopped. As with a time.Timer, a timer whose value has not +// been received is active: Reset discards the value and returns true. // // See https://pkg.go.dev/time#Timer.Reset for more information. func (t *Timer) Reset(d time.Duration, tags ...string) bool { @@ -133,8 +140,9 @@ func (t *Timer) Reset(d time.Duration, tags ...string) bool { t.mock.matchCallLocked(c) defer close(c.complete) result := !t.stopped - // check if we've already fired, and if so, interrupt it. - t.endWriteLocked() + if t.endWriteLocked() { + result = true + } // Remove the timer from the mock's event list synchronously; otherwise the // stale old schedule can cause e.g. Advance() to reject advancing past the // old nxt, even though the timer has been reset. From 9a2647ff21fc7d689dbf140d2be498d2771311b4 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 12:52:12 +0000 Subject: [PATCH 20/33] fix: drop timer fires that Stop or Reset replaced NewTimer and Reset fire a timer with a non-positive duration from a new goroutine. When Stop or Reset ran before that goroutine, the fire still sent its value after they returned, and a Reset's new schedule was removed: NewTimer(0) then Reset(time.Hour) delivered a value and left no event in 200 of 200 synctest runs. A fire that Advance started has the same race with a Stop or Reset that gets the lock first. fire now returns without effect when the timer was stopped or nxt no longer matches the fire's time, and NewTimer, AfterFunc and Reset set nxt to the current time for a non-positive duration so their own fire still matches. Two fires can no longer both start a write, so fire no longer ends an earlier one. The new StopZeroDuration and ResetZeroDuration rows fail without the check. --- mock.go | 2 ++ mock_test.go | 26 ++++++++++++++++++++------ timer.go | 13 +++++++++---- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/mock.go b/mock.go index 6755410..6254deb 100644 --- a/mock.go +++ b/mock.go @@ -102,6 +102,7 @@ func (m *Mock) NewTimer(d time.Duration, tags ...string) *Timer { if d <= 0 { // zero or negative duration timer means we should immediately fire // it, rather than add it. + t.nxt = m.cur go t.fire(t.mock.cur) return t } @@ -123,6 +124,7 @@ func (m *Mock) AfterFunc(d time.Duration, f func(), tags ...string) *Timer { if d <= 0 { // zero or negative duration timer means we should immediately fire // it, rather than add it. + t.nxt = m.cur go t.fire(t.mock.cur) return t } diff --git a/mock_test.go b/mock_test.go index 507dd42..3594ee6 100644 --- a/mock_test.go +++ b/mock_test.go @@ -515,7 +515,7 @@ func (l *testLogger) Logf(format string, args ...any) { } // TestTimer_StopAndReset checks what Stop and Reset return for a timer that has fired, and that -// no value is received after they return, as with a time.Timer. +// no value is received after they return and Reset's schedule stays, as with a time.Timer. func TestTimer_StopAndReset(t *testing.T) { t.Parallel() fired := func(ctx context.Context, m *quartz.Mock) *quartz.Timer { @@ -528,6 +528,8 @@ func TestTimer_StopAndReset(t *testing.T) { <-tmr.C return tmr } + // NewTimer(0) fires from a new goroutine, which can run before or after the call. + zeroDuration := func(_ context.Context, m *quartz.Mock) *quartz.Timer { return m.NewTimer(0) } stop := func(tmr *quartz.Timer) bool { return tmr.Stop() } reset := func(tmr *quartz.Timer) bool { return tmr.Reset(time.Hour) } // stopAndDrain is the way to stop a timer before Go 1.23. It returns false if the receive @@ -547,11 +549,18 @@ func TestTimer_StopAndReset(t *testing.T) { start func(context.Context, *quartz.Mock) *quartz.Timer call func(*quartz.Timer) bool want bool + // wantNext is how long after the call the timer fires, or 0 if it does not. + wantNext time.Duration }{ {name: "StopFiredUnread", start: fired, call: stop, want: true}, {name: "StopFiredReceived", start: firedAndReceived, call: stop, want: false}, - {name: "ResetFiredUnread", start: fired, call: reset, want: true}, + {name: "ResetFiredUnread", start: fired, call: reset, want: true, wantNext: time.Hour}, {name: "StopAndDrainFiredUnread", start: fired, call: stopAndDrain, want: true}, + {name: "StopZeroDuration", start: zeroDuration, call: stop, want: true}, + { + name: "ResetZeroDuration", start: zeroDuration, call: reset, + want: true, wantNext: time.Hour, + }, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -559,17 +568,21 @@ func TestTimer_StopAndReset(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - tmr := tc.start(ctx, quartz.NewMock(t)) + mClock := quartz.NewMock(t) + tmr := tc.start(ctx, mClock) if got := tc.call(tmr); got != tc.want { t.Errorf("got %v, want %v", got, tc.want) } - // Lets any goroutine that could still send on C run before the check. + // Lets any goroutine that could still send on C run before the checks. synctest.Wait() select { case tme := <-tmr.C: t.Errorf("received %s after the call returned", tme) default: } + if next, _ := mClock.Peek(); next != tc.wantNext { + t.Errorf("timer fires in %s, want %s", next, tc.wantNext) + } }) }) } @@ -726,8 +739,9 @@ func TestNoLeak_Go123(t *testing.T) { mClock.Advance(1 * time.Second).MustWait(ctx) }) - // NewTimer and Reset fire a timer with a non-positive duration from a new goroutine, so both - // fires can start a write before either ends. Over 100 rounds, that happens in most of them. + // NewTimer(0) and Reset(0) each fire the timer from a new goroutine, and in most of 100 rounds + // both run after Reset. Only one of them may start a write: the Mock tracks one write per + // timer, so its cleanup would not end a second one. t.Run("TimerFiredTwice", func(t *testing.T) { t.Parallel() for range 100 { diff --git a/timer.go b/timer.go index 7887ac5..496bd74 100644 --- a/timer.go +++ b/timer.go @@ -34,6 +34,14 @@ type Timer struct { func (t *Timer) fire(tt time.Time) { t.mock.mu.Lock() + // A fire that gets the lock after Stop or Reset belongs to the schedule they replaced: Stop + // stops the timer, and Reset moves nxt. Both happen when NewTimer or Reset fires a + // non-positive duration from a new goroutine, and when Stop or Reset gets the lock before a + // fire that Advance started. + if t.stopped || !t.nxt.Equal(tt) { + t.mock.mu.Unlock() + return + } t.mock.removeTimerLocked(t) if t.fn != nil { t.mock.mu.Unlock() @@ -48,10 +56,6 @@ func (t *Timer) fire(tt time.Time) { default: } } else { - // Reset and NewTimer fire a timer with a non-positive duration from a new goroutine, so the - // timer can fire again before the write from an earlier fire has ended. writes tracks one - // write per timer, so end the earlier one, as Reset would. - t.endWriteLocked() interrupt := make(chan struct{}) t.interrupt = interrupt t.mock.writes[t] = struct{}{} @@ -151,6 +155,7 @@ func (t *Timer) Reset(d time.Duration, tags ...string) bool { // zero or negative duration timer means we should immediately re-fire // it, rather than re-add it. t.stopped = false + t.nxt = t.mock.cur go t.fire(t.mock.cur) return result } From c7b89603155aa5320f669404b6a854fe49dd2813 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 12:53:22 +0000 Subject: [PATCH 21/33] test(mock_test.go): stop and reset a timer while Advance fires it A Stop or Reset right after Advance returns usually gets the lock before the fire that Advance started, and before the previous commit that fire still sent its value afterward: 1998 of 2000 runs after Stop and 1996 of 2000 after Reset. Without the check in fire, the new rows fail 100 of 100 runs, and about 63 of 100 with -race. --- mock_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/mock_test.go b/mock_test.go index 3594ee6..6aa15e4 100644 --- a/mock_test.go +++ b/mock_test.go @@ -530,6 +530,12 @@ func TestTimer_StopAndReset(t *testing.T) { } // NewTimer(0) fires from a new goroutine, which can run before or after the call. zeroDuration := func(_ context.Context, m *quartz.Mock) *quartz.Timer { return m.NewTimer(0) } + // Advance fires the timer from a new goroutine, which can run before or after the call. + advancing := func(_ context.Context, m *quartz.Mock) *quartz.Timer { + tmr := m.NewTimer(time.Second) + _ = m.Advance(time.Second) + return tmr + } stop := func(tmr *quartz.Timer) bool { return tmr.Stop() } reset := func(tmr *quartz.Timer) bool { return tmr.Reset(time.Hour) } // stopAndDrain is the way to stop a timer before Go 1.23. It returns false if the receive @@ -561,6 +567,11 @@ func TestTimer_StopAndReset(t *testing.T) { name: "ResetZeroDuration", start: zeroDuration, call: reset, want: true, wantNext: time.Hour, }, + {name: "StopDuringAdvance", start: advancing, call: stop, want: true}, + { + name: "ResetDuringAdvance", start: advancing, call: reset, + want: true, wantNext: time.Hour, + }, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() From d67f36ad1ef5ef360f532a36b1d45d5a45190874 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 15:30:01 +0000 Subject: [PATCH 22/33] test(mock_test.go): stop an AfterFunc timer before f's goroutine runs The check that drops replaced fires also covers AfterFunc: with AfterFunc(0, f) and an immediate Stop, Stop returned true and f still ran in 200 of 200 synctest runs before that check, and in 0 of 200 after it. A time.AfterFunc timer never runs f after Stop returns true. The new StopAfterFuncZeroDuration row checks that Stop's result agrees with whether f ran, because f's goroutine can run before or after Stop. Without the check in fire it fails 100 of 100 runs. The start functions now also return the channel the fire sends on: C, or one that f sends on. --- mock_test.go | 66 ++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 20 deletions(-) diff --git a/mock_test.go b/mock_test.go index 6aa15e4..3cc1a0c 100644 --- a/mock_test.go +++ b/mock_test.go @@ -518,48 +518,70 @@ func (l *testLogger) Logf(format string, args ...any) { // no value is received after they return and Reset's schedule stays, as with a time.Timer. func TestTimer_StopAndReset(t *testing.T) { t.Parallel() - fired := func(ctx context.Context, m *quartz.Mock) *quartz.Timer { + // Each start function returns the timer and the channel its fire sends on. + fired := func(ctx context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { tmr := m.NewTimer(time.Second) m.Advance(time.Second).MustWait(ctx) - return tmr + return tmr, tmr.C } - firedAndReceived := func(ctx context.Context, m *quartz.Mock) *quartz.Timer { - tmr := fired(ctx, m) - <-tmr.C - return tmr + received := func(ctx context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { + tmr, c := fired(ctx, m) + <-c + return tmr, c } // NewTimer(0) fires from a new goroutine, which can run before or after the call. - zeroDuration := func(_ context.Context, m *quartz.Mock) *quartz.Timer { return m.NewTimer(0) } + zeroDuration := func(_ context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { + tmr := m.NewTimer(0) + return tmr, tmr.C + } // Advance fires the timer from a new goroutine, which can run before or after the call. - advancing := func(_ context.Context, m *quartz.Mock) *quartz.Timer { + advancing := func(_ context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { tmr := m.NewTimer(time.Second) _ = m.Advance(time.Second) - return tmr + return tmr, tmr.C + } + // AfterFunc(0) runs f from a new goroutine, which can run before or after the call. + zeroAfterFunc := func(_ context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { + ran := make(chan time.Time, 1) + return m.AfterFunc(0, func() { ran <- time.Time{} }), ran } - stop := func(tmr *quartz.Timer) bool { return tmr.Stop() } - reset := func(tmr *quartz.Timer) bool { return tmr.Reset(time.Hour) } + stop := func(tmr *quartz.Timer, _ <-chan time.Time) bool { return tmr.Stop() } + reset := func(tmr *quartz.Timer, _ <-chan time.Time) bool { return tmr.Reset(time.Hour) } // stopAndDrain is the way to stop a timer before Go 1.23. It returns false if the receive // blocks, which bubble time reports at once. - stopAndDrain := func(tmr *quartz.Timer) bool { + stopAndDrain := func(tmr *quartz.Timer, c <-chan time.Time) bool { if !tmr.Stop() { select { - case <-tmr.C: + case <-c: case <-time.After(time.Second): return false } } return true } + // stopAfterFunc reports whether Stop's result agrees with whether f ran, as for a + // time.AfterFunc timer: true if f never runs, false if f has started. Either can happen, + // depending on whether f's goroutine runs before Stop. + stopAfterFunc := func(tmr *quartz.Timer, ran <-chan time.Time) bool { + stopped := tmr.Stop() + synctest.Wait() + select { + case <-ran: + return !stopped + default: + return stopped + } + } for _, tc := range []struct { name string - start func(context.Context, *quartz.Mock) *quartz.Timer - call func(*quartz.Timer) bool + start func(context.Context, *quartz.Mock) (*quartz.Timer, <-chan time.Time) + call func(*quartz.Timer, <-chan time.Time) bool want bool // wantNext is how long after the call the timer fires, or 0 if it does not. wantNext time.Duration }{ {name: "StopFiredUnread", start: fired, call: stop, want: true}, - {name: "StopFiredReceived", start: firedAndReceived, call: stop, want: false}, + {name: "StopFiredReceived", start: received, call: stop, want: false}, {name: "ResetFiredUnread", start: fired, call: reset, want: true, wantNext: time.Hour}, {name: "StopAndDrainFiredUnread", start: fired, call: stopAndDrain, want: true}, {name: "StopZeroDuration", start: zeroDuration, call: stop, want: true}, @@ -572,6 +594,10 @@ func TestTimer_StopAndReset(t *testing.T) { name: "ResetDuringAdvance", start: advancing, call: reset, want: true, wantNext: time.Hour, }, + { + name: "StopAfterFuncZeroDuration", start: zeroAfterFunc, call: stopAfterFunc, + want: true, + }, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -580,14 +606,14 @@ func TestTimer_StopAndReset(t *testing.T) { defer cancel() mClock := quartz.NewMock(t) - tmr := tc.start(ctx, mClock) - if got := tc.call(tmr); got != tc.want { + tmr, c := tc.start(ctx, mClock) + if got := tc.call(tmr, c); got != tc.want { t.Errorf("got %v, want %v", got, tc.want) } - // Lets any goroutine that could still send on C run before the checks. + // Lets any goroutine that could still send on c run before the checks. synctest.Wait() select { - case tme := <-tmr.C: + case tme := <-c: t.Errorf("received %s after the call returned", tme) default: } From 6ad6c962fa8c7d251f0b0297efa37c41d4718866 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 15:30:17 +0000 Subject: [PATCH 23/33] docs(mock.go): say NewMock's cleanup advice covers timers only "The Mock's timers" can be read to include tickers, but a ticker still stops when the cleanup that NewTicker registers runs, so calling NewMock first does not keep its ticks readable. The NewMock doc now says so and points to NewTicker. --- mock.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mock.go b/mock.go index 6254deb..1670104 100644 --- a/mock.go +++ b/mock.go @@ -487,7 +487,8 @@ func (m *Mock) WithLogger(l Logger) *Mock { // are created. // // A timer value that has not been received when the cleanup registered by NewMock runs is dropped. -// Call NewMock before registering cleanups that wait for code reading the Mock's timers. +// Call NewMock before registering cleanups that wait for code reading the Mock's timers. This does +// not cover tickers; see NewTicker. func NewMock(tb TestingT) *Mock { cur, err := time.Parse(time.RFC3339, "2024-01-01T00:00:00Z") if err != nil { From af20b2b8fb2c9de74e0e6230d7c0920ec3b7ae94 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:28:45 +0000 Subject: [PATCH 24/33] test(mock_test.go): say why the AfterFunc row checks agreement The start function comment said each returns the channel its fire sends on, but for the AfterFunc row f sends on it and fire sends nothing. The AfterFunc row checks that Stop's result agrees with whether f ran instead of expecting true, and the comment did not say why. Stop gets the Mock's lock before f's goroutine in practice, but that goroutine got it first in 19 of 32000 runs with -race, and no Mock call can hold it back, so a row that expected true would fail in those runs. --- mock_test.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/mock_test.go b/mock_test.go index 3cc1a0c..ba279f9 100644 --- a/mock_test.go +++ b/mock_test.go @@ -518,7 +518,8 @@ func (l *testLogger) Logf(format string, args ...any) { // no value is received after they return and Reset's schedule stays, as with a time.Timer. func TestTimer_StopAndReset(t *testing.T) { t.Parallel() - // Each start function returns the timer and the channel its fire sends on. + // Each start function returns the timer and the channel its value arrives on: C, or for an + // AfterFunc timer a channel that f sends on. fired := func(ctx context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { tmr := m.NewTimer(time.Second) m.Advance(time.Second).MustWait(ctx) @@ -540,7 +541,8 @@ func TestTimer_StopAndReset(t *testing.T) { _ = m.Advance(time.Second) return tmr, tmr.C } - // AfterFunc(0) runs f from a new goroutine, which can run before or after the call. + // AfterFunc(0) runs f from a new goroutine. Stop usually gets the Mock's lock before it: the + // goroutine got it first in 19 of 32000 runs with -race and in none without. zeroAfterFunc := func(_ context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { ran := make(chan time.Time, 1) return m.AfterFunc(0, func() { ran <- time.Time{} }), ran @@ -559,9 +561,10 @@ func TestTimer_StopAndReset(t *testing.T) { } return true } - // stopAfterFunc reports whether Stop's result agrees with whether f ran, as for a - // time.AfterFunc timer: true if f never runs, false if f has started. Either can happen, - // depending on whether f's goroutine runs before Stop. + // stopAfterFunc reports whether Stop's result agrees with whether f ran, which is what a + // time.AfterFunc timer promises: true if f never runs, false if f has started. Expecting true + // would fail in the runs where f's goroutine gets the lock first, and no Mock call can hold + // that goroutine back. stopAfterFunc := func(tmr *quartz.Timer, ran <-chan time.Time) bool { stopped := tmr.Stop() synctest.Wait() From 703d0b439aad5ee1fccc078f9c1f4e0778483c50 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:28:54 +0000 Subject: [PATCH 25/33] docs(mock.go): say when a ticker's ticks stop "This does not cover tickers; see NewTicker" did not say what happens to a ticker. Its ticks stop when the cleanup that NewTicker registers runs, which is before any cleanup registered earlier in the test, so the NewMock doc now says that. --- mock.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mock.go b/mock.go index 1670104..e9901b4 100644 --- a/mock.go +++ b/mock.go @@ -487,8 +487,9 @@ func (m *Mock) WithLogger(l Logger) *Mock { // are created. // // A timer value that has not been received when the cleanup registered by NewMock runs is dropped. -// Call NewMock before registering cleanups that wait for code reading the Mock's timers. This does -// not cover tickers; see NewTicker. +// Call NewMock before registering cleanups that wait for code reading the Mock's timers. Tickers +// work differently: a ticker stops sending ticks when the cleanup registered by its NewTicker call +// runs. func NewMock(tb TestingT) *Mock { cur, err := time.Parse(time.RFC3339, "2024-01-01T00:00:00Z") if err != nil { From 9de08b8729197b78bf5ab87b59041ccfb1ada192 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:58:38 +0000 Subject: [PATCH 26/33] docs(mock.go): say what Reset does to a ticker after its cleanup The NewTicker doc said a ticker sends no ticks after the test ends, and the NewMock doc said a ticker stops when its NewTicker cleanup runs. Both are false once Reset is called after that cleanup: Reset starts a new runLoop, ticks are sent again, and only Stop ends it. Both docs now say so, and the NewMock doc names NewTicker tickers, because a TickerFunc keeps calling its function until its context ends. --- mock.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/mock.go b/mock.go index e9901b4..7f3a140 100644 --- a/mock.go +++ b/mock.go @@ -71,9 +71,10 @@ func (m *Mock) TickerFunc(ctx context.Context, d time.Duration, f func() error, return t } -// NewTicker creates a mocked ticker attached to this Mock. Note that it will cease sending ticks on its channel at the -// end of the test, to avoid leaking any goroutines. Ticks are suppressed even if the mock clock is advanced after the -// test completes. Best practice is to only manipulate the mock time in the main goroutine of the test. +// NewTicker creates a mocked ticker attached to this Mock. The ticker stops sending ticks when the +// cleanup that NewTicker registers runs, and advancing the clock after that sends none. A Reset +// after that cleanup starts the ticker again, and only Stop ends it then. Best practice is to only +// manipulate the mock time in the main goroutine of the test. func (m *Mock) NewTicker(d time.Duration, tags ...string) *Ticker { if d <= 0 { panic("NewTicker called with negative or zero duration") @@ -488,8 +489,8 @@ func (m *Mock) WithLogger(l Logger) *Mock { // // A timer value that has not been received when the cleanup registered by NewMock runs is dropped. // Call NewMock before registering cleanups that wait for code reading the Mock's timers. Tickers -// work differently: a ticker stops sending ticks when the cleanup registered by its NewTicker call -// runs. +// from NewTicker work differently: a ticker stops sending ticks when the cleanup registered by +// NewTicker runs, unless Reset starts it again after that cleanup. func NewMock(tb TestingT) *Mock { cur, err := time.Parse(time.RFC3339, "2024-01-01T00:00:00Z") if err != nil { From c4ba3c2f9633af83ac320c1be8d9fdf6d856b43c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:58:38 +0000 Subject: [PATCH 27/33] test(mock_test.go): give the AfterFunc race rate without a count The comment gave one measured count, but repeated measurements vary: f's goroutine got the lock first in 19 of 32000 runs with -race here and in 28 to 45 of 32000 in a review. "A few in ten thousand" holds for all of them. --- mock_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mock_test.go b/mock_test.go index ba279f9..bac2182 100644 --- a/mock_test.go +++ b/mock_test.go @@ -542,7 +542,7 @@ func TestTimer_StopAndReset(t *testing.T) { return tmr, tmr.C } // AfterFunc(0) runs f from a new goroutine. Stop usually gets the Mock's lock before it: the - // goroutine got it first in 19 of 32000 runs with -race and in none without. + // goroutine gets it first in a few in ten thousand runs with -race. zeroAfterFunc := func(_ context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { ran := make(chan time.Time, 1) return m.AfterFunc(0, func() { ran <- time.Time{} }), ran From 09613b7a7fc20c1b7cca4e9728928b7b0794ae0c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 17:28:04 +0000 Subject: [PATCH 28/33] docs(mock.go): lead the NewMock doc with what cleanups can receive The NewMock doc now says first what a cleanup registered after NewMock can rely on, then what NewMock's cleanup drops and what a later fire sends, and points to NewTicker for ticks. NewTicker no longer describes a Reset after its cleanup: #38 removes that cleanup, so the sentence would only be churn. --- mock.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/mock.go b/mock.go index 7f3a140..9225dfe 100644 --- a/mock.go +++ b/mock.go @@ -72,9 +72,8 @@ func (m *Mock) TickerFunc(ctx context.Context, d time.Duration, f func() error, } // NewTicker creates a mocked ticker attached to this Mock. The ticker stops sending ticks when the -// cleanup that NewTicker registers runs, and advancing the clock after that sends none. A Reset -// after that cleanup starts the ticker again, and only Stop ends it then. Best practice is to only -// manipulate the mock time in the main goroutine of the test. +// cleanup that NewTicker registers runs, and advancing the clock after that sends none. Best +// practice is to only manipulate the mock time in the main goroutine of the test. func (m *Mock) NewTicker(d time.Duration, tags ...string) *Ticker { if d <= 0 { panic("NewTicker called with negative or zero duration") @@ -487,10 +486,11 @@ func (m *Mock) WithLogger(l Logger) *Mock { // You may re-set the time earlier than this, but only before timers or tickers // are created. // -// A timer value that has not been received when the cleanup registered by NewMock runs is dropped. -// Call NewMock before registering cleanups that wait for code reading the Mock's timers. Tickers -// from NewTicker work differently: a ticker stops sending ticks when the cleanup registered by -// NewTicker runs, unless Reset starts it again after that cleanup. +// Cleanups registered after NewMock run before NewMock's own cleanup, so they can still receive +// the value of a timer that fired during the test. NewMock's cleanup drops timer values that have +// not been received, and a timer from NewTimer that fires after it sends only to a reader already +// waiting on C. Call NewMock before registering cleanups that wait for code reading the Mock's +// timers. Ticks from NewTicker stop earlier; see [Mock.NewTicker]. func NewMock(tb TestingT) *Mock { cur, err := time.Parse(time.RFC3339, "2024-01-01T00:00:00Z") if err != nil { From 7f21d1d85544a06ca0e31b4ebcb9d13c7c9b7540 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 17:28:04 +0000 Subject: [PATCH 29/33] docs(timer.go): cover an earlier fire in the stale-fire comment The comment named Stop and Reset as the reasons a fire is dropped, but the check also drops the second fire after NewTimer(0) and Reset(0), where the first fire has already removed the timer. --- timer.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/timer.go b/timer.go index 496bd74..498463d 100644 --- a/timer.go +++ b/timer.go @@ -34,10 +34,9 @@ type Timer struct { func (t *Timer) fire(tt time.Time) { t.mock.mu.Lock() - // A fire that gets the lock after Stop or Reset belongs to the schedule they replaced: Stop - // stops the timer, and Reset moves nxt. Both happen when NewTimer or Reset fires a - // non-positive duration from a new goroutine, and when Stop or Reset gets the lock before a - // fire that Advance started. + // Drop a fire whose schedule is gone: the timer was stopped, has already fired, or Reset + // moved nxt. Fires for a non-positive duration and fires from Advance run on new goroutines, + // so Stop, Reset or an earlier fire can take the lock first. if t.stopped || !t.nxt.Equal(tt) { t.mock.mu.Unlock() return From 3eb43f867830ba24ea7e3568a646a64c34e06291 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 17:28:19 +0000 Subject: [PATCH 30/33] test(mock_test.go): restore TestTickerFunc_ContextDoneDuringTick e6cbe10 moved this test into a synctest bubble so that it no longer waits 100ms of wall time. That change is unrelated to the timer fixes here and will be done separately. --- mock_test.go | 85 +++++++++++++++++++++++++--------------------------- 1 file changed, 40 insertions(+), 45 deletions(-) diff --git a/mock_test.go b/mock_test.go index bac2182..afc614f 100644 --- a/mock_test.go +++ b/mock_test.go @@ -222,56 +222,51 @@ func TestPeek(t *testing.T) { // function callback is in progress. func TestTickerFunc_ContextDoneDuringTick(t *testing.T) { t.Parallel() - synctest.Test(t, func(t *testing.T) { - testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second) - defer testCancel() - mClock := quartz.NewMock(t) + testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer testCancel() + mClock := quartz.NewMock(t) - tickStart := make(chan struct{}) - tickDone := make(chan struct{}) - ctx, cancel := context.WithCancel(testCtx) - defer cancel() - tkr := mClock.TickerFunc(ctx, time.Second, func() error { - close(tickStart) - select { - case <-tickDone: - case <-testCtx.Done(): - t.Error("timeout waiting for tickDone") - } - return nil - }) - w := mClock.Advance(time.Second) - select { - case <-tickStart: - // OK - case <-testCtx.Done(): - t.Fatal("timeout waiting for tickStart") - } - waitErr := make(chan error, 1) - go func() { - waitErr <- tkr.Wait() - }() - cancel() - // Once every goroutine in the bubble is blocked, Wait has either returned or is waiting - // for the callback. - synctest.Wait() - select { - case <-waitErr: - t.Fatal("wait should not return while tick callback in progress") - default: - // OK - } - close(tickDone) + tickStart := make(chan struct{}) + tickDone := make(chan struct{}) + ctx, cancel := context.WithCancel(testCtx) + defer cancel() + tkr := mClock.TickerFunc(ctx, time.Second, func() error { + close(tickStart) select { - case err := <-waitErr: - if !errors.Is(err, context.Canceled) { - t.Fatal("expected context.Canceled error") - } + case <-tickDone: case <-testCtx.Done(): - t.Fatal("timed out waiting for wait to finish") + t.Error("timeout waiting for tickDone") } - w.MustWait(testCtx) + return nil }) + w := mClock.Advance(time.Second) + select { + case <-tickStart: + // OK + case <-testCtx.Done(): + t.Fatal("timeout waiting for tickStart") + } + waitErr := make(chan error, 1) + go func() { + waitErr <- tkr.Wait() + }() + cancel() + select { + case <-waitErr: + t.Fatal("wait should not return while tick callback in progress") + case <-time.After(time.Millisecond * 100): + // OK + } + close(tickDone) + select { + case err := <-waitErr: + if !errors.Is(err, context.Canceled) { + t.Fatal("expected context.Canceled error") + } + case <-testCtx.Done(): + t.Fatal("timed out waiting for wait to finish") + } + w.MustWait(testCtx) } // TestTickerFunc_LongCallback tests that we don't call the ticker func a second time while the From ed9d9fd8a3f05550f139c4d15dc59e36f1ae207d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 17:29:32 +0000 Subject: [PATCH 31/33] test(mock_test.go): name what disagrees when AfterFunc Stop fails StopAfterFuncZeroDuration failed with "got false, want true", which did not say whether f ran after Stop returned true or Stop returned false before f ran. It now runs after the table as its own subtest that names the disagreement, and the AfterFunc race is explained once there. With the AfterFunc row gone, the start functions return only the timer. StopAndDrainFiredUnread is gone: it passes whenever StopFiredUnread does, and a mutation that breaks Stop's result for a fired timer fails StopFiredUnread alone. StopFiredUnread now says that its result keeps if !t.Stop() { <-t.C } from blocking. --- mock_test.go | 103 ++++++++++++++++++++++----------------------------- 1 file changed, 45 insertions(+), 58 deletions(-) diff --git a/mock_test.go b/mock_test.go index afc614f..1bf9222 100644 --- a/mock_test.go +++ b/mock_test.go @@ -513,75 +513,40 @@ func (l *testLogger) Logf(format string, args ...any) { // no value is received after they return and Reset's schedule stays, as with a time.Timer. func TestTimer_StopAndReset(t *testing.T) { t.Parallel() - // Each start function returns the timer and the channel its value arrives on: C, or for an - // AfterFunc timer a channel that f sends on. - fired := func(ctx context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { + fired := func(ctx context.Context, m *quartz.Mock) *quartz.Timer { tmr := m.NewTimer(time.Second) m.Advance(time.Second).MustWait(ctx) - return tmr, tmr.C + return tmr } - received := func(ctx context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { - tmr, c := fired(ctx, m) - <-c - return tmr, c + received := func(ctx context.Context, m *quartz.Mock) *quartz.Timer { + tmr := fired(ctx, m) + <-tmr.C + return tmr } // NewTimer(0) fires from a new goroutine, which can run before or after the call. - zeroDuration := func(_ context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { - tmr := m.NewTimer(0) - return tmr, tmr.C + zeroDuration := func(_ context.Context, m *quartz.Mock) *quartz.Timer { + return m.NewTimer(0) } // Advance fires the timer from a new goroutine, which can run before or after the call. - advancing := func(_ context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { + advancing := func(_ context.Context, m *quartz.Mock) *quartz.Timer { tmr := m.NewTimer(time.Second) _ = m.Advance(time.Second) - return tmr, tmr.C - } - // AfterFunc(0) runs f from a new goroutine. Stop usually gets the Mock's lock before it: the - // goroutine gets it first in a few in ten thousand runs with -race. - zeroAfterFunc := func(_ context.Context, m *quartz.Mock) (*quartz.Timer, <-chan time.Time) { - ran := make(chan time.Time, 1) - return m.AfterFunc(0, func() { ran <- time.Time{} }), ran - } - stop := func(tmr *quartz.Timer, _ <-chan time.Time) bool { return tmr.Stop() } - reset := func(tmr *quartz.Timer, _ <-chan time.Time) bool { return tmr.Reset(time.Hour) } - // stopAndDrain is the way to stop a timer before Go 1.23. It returns false if the receive - // blocks, which bubble time reports at once. - stopAndDrain := func(tmr *quartz.Timer, c <-chan time.Time) bool { - if !tmr.Stop() { - select { - case <-c: - case <-time.After(time.Second): - return false - } - } - return true - } - // stopAfterFunc reports whether Stop's result agrees with whether f ran, which is what a - // time.AfterFunc timer promises: true if f never runs, false if f has started. Expecting true - // would fail in the runs where f's goroutine gets the lock first, and no Mock call can hold - // that goroutine back. - stopAfterFunc := func(tmr *quartz.Timer, ran <-chan time.Time) bool { - stopped := tmr.Stop() - synctest.Wait() - select { - case <-ran: - return !stopped - default: - return stopped - } + return tmr } + stop := func(tmr *quartz.Timer) bool { return tmr.Stop() } + reset := func(tmr *quartz.Timer) bool { return tmr.Reset(time.Hour) } for _, tc := range []struct { name string - start func(context.Context, *quartz.Mock) (*quartz.Timer, <-chan time.Time) - call func(*quartz.Timer, <-chan time.Time) bool + start func(context.Context, *quartz.Mock) *quartz.Timer + call func(*quartz.Timer) bool want bool // wantNext is how long after the call the timer fires, or 0 if it does not. wantNext time.Duration }{ + // Stop returning true here keeps if !t.Stop() { <-t.C } from blocking. {name: "StopFiredUnread", start: fired, call: stop, want: true}, {name: "StopFiredReceived", start: received, call: stop, want: false}, {name: "ResetFiredUnread", start: fired, call: reset, want: true, wantNext: time.Hour}, - {name: "StopAndDrainFiredUnread", start: fired, call: stopAndDrain, want: true}, {name: "StopZeroDuration", start: zeroDuration, call: stop, want: true}, { name: "ResetZeroDuration", start: zeroDuration, call: reset, @@ -592,10 +557,6 @@ func TestTimer_StopAndReset(t *testing.T) { name: "ResetDuringAdvance", start: advancing, call: reset, want: true, wantNext: time.Hour, }, - { - name: "StopAfterFuncZeroDuration", start: zeroAfterFunc, call: stopAfterFunc, - want: true, - }, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -604,14 +565,14 @@ func TestTimer_StopAndReset(t *testing.T) { defer cancel() mClock := quartz.NewMock(t) - tmr, c := tc.start(ctx, mClock) - if got := tc.call(tmr, c); got != tc.want { + tmr := tc.start(ctx, mClock) + if got := tc.call(tmr); got != tc.want { t.Errorf("got %v, want %v", got, tc.want) } - // Lets any goroutine that could still send on c run before the checks. + // Lets any goroutine that could still send on C run before the checks. synctest.Wait() select { - case tme := <-c: + case tme := <-tmr.C: t.Errorf("received %s after the call returned", tme) default: } @@ -621,6 +582,32 @@ func TestTimer_StopAndReset(t *testing.T) { }) }) } + + // A time.AfterFunc timer's Stop returns true if f never runs, and false if f has started. + // AfterFunc(0) runs f from a new goroutine, and Stop usually gets the Mock's lock before it: + // the goroutine gets it first in a few in ten thousand runs with -race. No Mock call can hold + // that goroutine back, so this accepts either order and checks that Stop's result agrees with + // whether f ran. + t.Run("StopAfterFuncZeroDuration", func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + ran := make(chan struct{}) + tmr := quartz.NewMock(t).AfterFunc(0, func() { close(ran) }) + stopped := tmr.Stop() + // Lets f's goroutine run before the check. + synctest.Wait() + select { + case <-ran: + if stopped { + t.Error("f ran, but Stop returned true") + } + default: + if !stopped { + t.Error("f has not run, but Stop returned false") + } + } + }) + }) } // TestTimerReset_NegativeDuration verifies that resetting a still-active timer From 1ac69512802eaad646ce1142242dc4fc54ce1546 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 17:29:32 +0000 Subject: [PATCH 32/33] test(mock_test.go): name the NewTimer(0) and Reset(0) leak case TimerFiredTwice named a result that the fix prevents. The new name says what the subtest does. --- mock_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mock_test.go b/mock_test.go index 1bf9222..5c7ac26 100644 --- a/mock_test.go +++ b/mock_test.go @@ -764,7 +764,7 @@ func TestNoLeak_Go123(t *testing.T) { // NewTimer(0) and Reset(0) each fire the timer from a new goroutine, and in most of 100 rounds // both run after Reset. Only one of them may start a write: the Mock tracks one write per // timer, so its cleanup would not end a second one. - t.Run("TimerFiredTwice", func(t *testing.T) { + t.Run("ZeroDurationTimerResetToZero", func(t *testing.T) { t.Parallel() for range 100 { tmr := quartz.NewMock(t).WithLogger(quartz.NoOpLogger).NewTimer(0) From 65aed390128750de696fa78c08ac304ca6700d1c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 18:10:30 +0000 Subject: [PATCH 33/33] docs(mock.go): say again what Reset does to a ticker after its cleanup On this branch a Reset after the ticker's own cleanup starts its runLoop again, and ticks then arrive until Stop, also after NewMock's cleanup. 09613b7 dropped that from the NewTicker doc because #38 removes the per-ticker cleanup, but this branch can merge first, so its docs must hold without #38. NewMock's pointer to NewTicker now names the exception too. --- mock.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/mock.go b/mock.go index 9225dfe..6325cd8 100644 --- a/mock.go +++ b/mock.go @@ -72,8 +72,9 @@ func (m *Mock) TickerFunc(ctx context.Context, d time.Duration, f func() error, } // NewTicker creates a mocked ticker attached to this Mock. The ticker stops sending ticks when the -// cleanup that NewTicker registers runs, and advancing the clock after that sends none. Best -// practice is to only manipulate the mock time in the main goroutine of the test. +// cleanup that NewTicker registers runs, and advancing the clock after that sends none. A Reset +// after that cleanup starts the ticker again, and only Stop ends it then. Best practice is to only +// manipulate the mock time in the main goroutine of the test. func (m *Mock) NewTicker(d time.Duration, tags ...string) *Ticker { if d <= 0 { panic("NewTicker called with negative or zero duration") @@ -490,7 +491,8 @@ func (m *Mock) WithLogger(l Logger) *Mock { // the value of a timer that fired during the test. NewMock's cleanup drops timer values that have // not been received, and a timer from NewTimer that fires after it sends only to a reader already // waiting on C. Call NewMock before registering cleanups that wait for code reading the Mock's -// timers. Ticks from NewTicker stop earlier; see [Mock.NewTicker]. +// timers. Ticks from NewTicker stop earlier, unless Reset starts the ticker again; see +// [Mock.NewTicker]. func NewMock(tb TestingT) *Mock { cur, err := time.Parse(time.RFC3339, "2024-01-01T00:00:00Z") if err != nil {