From a9e9b997a8ca636bd346dd0a09968c62e8fe9103 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 13:35:06 +0000 Subject: [PATCH 01/32] fix: report completed advances when context expired AdvanceWaiter.Wait and MustWait selected on the advance channel and ctx.Done() together. When both were ready, select picked one at random, so a completed advance failed about half the time with an expired context. The context only bounds how long to wait, so check for completion again before returning the context error. MustWait now calls Wait so both share that logic. --- mock.go | 18 +++++++++++------- mock_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/mock.go b/mock.go index 7255aff..ebc60f1 100644 --- a/mock.go +++ b/mock.go @@ -232,13 +232,20 @@ type AdvanceWaiter struct { ch chan struct{} } -// Wait for all timers and ticks to complete, or until context expires. +// Wait for all timers and ticks to complete, or until context expires. If the timers and ticks +// have completed, Wait returns nil even if the context has also expired. func (w AdvanceWaiter) Wait(ctx context.Context) error { select { case <-w.ch: return nil case <-ctx.Done(): - return ctx.Err() + // select picks randomly among ready cases; report completion if it also happened. + select { + case <-w.ch: + return nil + default: + return ctx.Err() + } } } @@ -247,11 +254,8 @@ func (w AdvanceWaiter) Wait(ctx context.Context) error { // benchmark, similar to `t.FailNow()`. func (w AdvanceWaiter) MustWait(ctx context.Context) { w.tb.Helper() - select { - case <-w.ch: - return - case <-ctx.Done(): - w.tb.Fatalf("context expired while waiting for clock to advance: %s", ctx.Err()) + if err := w.Wait(ctx); err != nil { + w.tb.Fatalf("context expired while waiting for clock to advance: %s", err) } } diff --git a/mock_test.go b/mock_test.go index 7bc822d..d8c0887 100644 --- a/mock_test.go +++ b/mock_test.go @@ -591,6 +591,33 @@ func TestTimerReset_NegativeDuration(t *testing.T) { } } +// TestAdvanceWaiter_CompletedWithExpiredContext verifies that a completed advance is reported as +// complete when the context has also expired, rather than at random. +func TestAdvanceWaiter_CompletedWithExpiredContext(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + t.Run("Wait", func(t *testing.T) { + t.Parallel() + mClock := quartz.NewMock(t) + // With no timers or tickers, the advance completes before Advance returns. + if err := mClock.Advance(time.Second).Wait(ctx); err != nil { + t.Fatalf("Wait() = %v, want nil", err) + } + }) + + t.Run("MustWait", func(t *testing.T) { + t.Parallel() + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) + mClock.Advance(time.Second).MustWait(ctx) + if tb.failed { + t.Fatal("MustWait failed the test after the advance completed") + } + }) +} + func TestNoLeak_Go123(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) From 50b22d0c12e5e86814ea0ad3408a1a575f104d7c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 13:36:28 +0000 Subject: [PATCH 02/32] fix: prefer ready calls over expired context in traps Trap.Wait and Call.Release had the same random select as AdvanceWaiter: with a trapped call ready, or a released call complete, an expired context could still win. Check the call channel again before returning the context error. The regression tests are internal because the public API cannot order these states before the context check: catch delivers each call from its own goroutine, and a call completes only after its last release. --- mock.go | 45 ++++++++++++++++++++++++-------------- mock_internal_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 16 deletions(-) create mode 100644 mock_internal_test.go diff --git a/mock.go b/mock.go index ebc60f1..c2e109f 100644 --- a/mock.go +++ b/mock.go @@ -691,7 +691,7 @@ type Call struct { } // Release the call and wait for it to complete. If the provided context expires before the call completes, it returns -// an error. +// an error. If the call has completed, Release returns nil even if the context has also expired. // // IMPORTANT: If a call is trapped by more than one trap, they all must release the call before it can complete, and // they must do so from different goroutines. @@ -699,7 +699,12 @@ func (c *Call) Release(ctx context.Context) error { c.apiCall.releases.Done() select { case <-ctx.Done(): - return fmt.Errorf("timed out waiting for release; did more than one trap capture the call?: %w", ctx.Err()) + // select picks randomly among ready cases; report completion if it also happened. + select { + case <-c.apiCall.complete: + default: + return fmt.Errorf("timed out waiting for release; did more than one trap capture the call?: %w", ctx.Err()) + } case <-c.apiCall.complete: // OK } @@ -808,26 +813,34 @@ func (t *Trap) callReleased() { var ErrTrapClosed = errors.New("trap closed") +// Wait for a call to be trapped, or until the context expires or the trap is closed. If a call is +// ready, Wait returns it even if the context has also expired. func (t *Trap) Wait(ctx context.Context) (*Call, error) { + var a *apiCall select { case <-ctx.Done(): - return nil, ctx.Err() + // select picks randomly among ready cases; report a ready call if there is one. + select { + case a = <-t.calls: + default: + return nil, ctx.Err() + } case <-t.done: return nil, ErrTrapClosed - case a := <-t.calls: - c := &Call{ - Time: a.Time, - Duration: a.Duration, - Tags: a.Tags, - apiCall: a, - trap: t, - tb: t.mock.tb, - } - t.mu.Lock() - defer t.mu.Unlock() - t.unreleasedCalls++ - return c, nil + case a = <-t.calls: + } + c := &Call{ + Time: a.Time, + Duration: a.Duration, + Tags: a.Tags, + apiCall: a, + trap: t, + tb: t.mock.tb, } + t.mu.Lock() + defer t.mu.Unlock() + t.unreleasedCalls++ + return c, nil } // MustWait calls Wait() and then if there is an error, immediately fails the diff --git a/mock_internal_test.go b/mock_internal_test.go new file mode 100644 index 0000000..101ef30 --- /dev/null +++ b/mock_internal_test.go @@ -0,0 +1,51 @@ +package quartz + +import ( + "context" + "testing" +) + +// The public API cannot make a trapped call ready, or complete, before Trap.Wait or Call.Release +// observes the context: catch delivers each call from its own goroutine, and a call completes only +// after its last release. These tests build that state directly. + +func TestTrapWait_CallReadyWithExpiredContext(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + // A buffered channel holding the call stands in for a catch goroutine blocked sending it. + trap := &Trap{ + fn: clockFunctionNow, + mock: NewMock(t), + calls: make(chan *apiCall, 1), + done: make(chan struct{}), + } + want := newCall(clockFunctionNow, nil) + trap.calls <- want + + c, err := trap.Wait(ctx) + if err != nil { + t.Fatalf("Wait() error = %v, want nil", err) + } + if c.apiCall != want { + t.Fatal("Wait() returned a different call than the ready one") + } +} + +func TestCallRelease_CompletedWithExpiredContext(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + a := newCall(clockFunctionNow, nil) + a.releases.Add(1) + // Closing complete first stands in for the trapped call returning after Release calls + // releases.Done and before Release checks the context. + close(a.complete) + c := &Call{tb: t, apiCall: a, trap: &Trap{unreleasedCalls: 1}} + + if err := c.Release(ctx); err != nil { + t.Fatalf("Release() = %v, want nil", err) + } +} From 35bd73f8b372aff1d9cf89fdbab1c906228d416e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 13:38:19 +0000 Subject: [PATCH 03/32] test(mock_test.go): cover expired context with pending work The completion recheck added to AdvanceWaiter.Wait and Trap.Wait must not block: when the awaited work has not happened, an expired context still returns its error. Call.Release already has this coverage in Test_MultipleTrapsDeadlock. --- mock_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/mock_test.go b/mock_test.go index d8c0887..44a819d 100644 --- a/mock_test.go +++ b/mock_test.go @@ -618,6 +618,42 @@ func TestAdvanceWaiter_CompletedWithExpiredContext(t *testing.T) { }) } +// TestExpiredContext_WorkPending verifies that waits still return the context error when the +// context has expired and the awaited work has not happened. +func TestExpiredContext_WorkPending(t *testing.T) { + t.Parallel() + expired, cancel := context.WithCancel(context.Background()) + cancel() + + t.Run("AdvanceWaiter", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + mClock := quartz.NewMock(t) + release := make(chan struct{}) + mClock.AfterFunc(time.Second, func() { <-release }) + w := mClock.Advance(time.Second) + // The advance cannot complete until the AfterFunc returns. + err := w.Wait(expired) + close(release) + w.MustWait(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + } + }) + + t.Run("Trap", func(t *testing.T) { + t.Parallel() + mClock := quartz.NewMock(t) + trap := mClock.Trap().Now() + defer trap.Close() + if _, err := trap.Wait(expired); !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + } + }) +} + func TestNoLeak_Go123(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) From 6922fa20088e7696f70e9a27ba259028e70c1118 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 13:52:50 +0000 Subject: [PATCH 04/32] docs(mock.go): stop implying expired contexts let waits succeed The Release and AdvanceWaiter.Wait docs said they return nil when the work has completed even if the context has also expired. Read as "an already-expired context works", that is wrong for the common cases: call.Release(expired) right after trap.MustWait returned an error 1000/1000, because the call completes only after Release's own releases.Done(). Advance with a pending AfterFunc followed by Wait(expired) also failed 1000/1000, because triggered events run on other goroutines. Describe the guarantee as completion by the time the context expires. --- mock.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/mock.go b/mock.go index c2e109f..7cbf8f7 100644 --- a/mock.go +++ b/mock.go @@ -232,8 +232,9 @@ type AdvanceWaiter struct { ch chan struct{} } -// Wait for all timers and ticks to complete, or until context expires. If the timers and ticks -// have completed, Wait returns nil even if the context has also expired. +// Wait for all timers and ticks to complete, or until context expires. If they have completed by +// the time the context expires, Wait returns nil. Triggered timers and ticks run on other +// goroutines, so a context that has already expired gives them no time to complete. func (w AdvanceWaiter) Wait(ctx context.Context) error { select { case <-w.ch: @@ -691,7 +692,9 @@ type Call struct { } // Release the call and wait for it to complete. If the provided context expires before the call completes, it returns -// an error. If the call has completed, Release returns nil even if the context has also expired. +// an error. If the call has completed by the time the context expires, Release returns nil. +// The call completes only after it is released, so a context that has already expired gives it +// no time to complete. // // IMPORTANT: If a call is trapped by more than one trap, they all must release the call before it can complete, and // they must do so from different goroutines. From b4bfb51cb70e0066b97259442f3a95b9973abdc2 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 13:53:01 +0000 Subject: [PATCH 05/32] docs(mock.go): describe Trap.Wait expired context like the others "If a call is ready" reads as "if the code under test has made the call", but each trapped call reaches Wait from a separate catch goroutine. With a call trapped by two traps, trap0.MustWait proved the call was made, yet trap1.Wait(expired) still failed 997/1000 because trap1's catch goroutine had not sent yet. Use the same wording as Release and AdvanceWaiter.Wait. --- mock.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mock.go b/mock.go index 7cbf8f7..b31d01c 100644 --- a/mock.go +++ b/mock.go @@ -816,8 +816,9 @@ func (t *Trap) callReleased() { var ErrTrapClosed = errors.New("trap closed") -// Wait for a call to be trapped, or until the context expires or the trap is closed. If a call is -// ready, Wait returns it even if the context has also expired. +// Wait for a call to be trapped, or until the context expires or the trap is closed. If a call +// has arrived by the time the context expires, Wait returns it. Trapped calls arrive from other +// goroutines, so a context that has already expired gives them no time to arrive. func (t *Trap) Wait(ctx context.Context) (*Call, error) { var a *apiCall select { From 9554a3a2ac4a2e9d714b16951a81789befacd586 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 13:53:54 +0000 Subject: [PATCH 06/32] test(mock_test.go): fail instead of hang when waits block If AdvanceWaiter.Wait or Trap.Wait ever blocked on an expired context, TestExpiredContext_WorkPending hung until the go test timeout. Run each Wait on a goroutine and bound it with a 10s context so a regression fails with a message. With both inner checks changed to block, each subtest now fails after 10s with "Wait() did not return after the context expired". --- mock_test.go | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/mock_test.go b/mock_test.go index 44a819d..82e9952 100644 --- a/mock_test.go +++ b/mock_test.go @@ -635,21 +635,41 @@ func TestExpiredContext_WorkPending(t *testing.T) { mClock.AfterFunc(time.Second, func() { <-release }) w := mClock.Advance(time.Second) // The advance cannot complete until the AfterFunc returns. - err := w.Wait(expired) - close(release) - w.MustWait(ctx) - if !errors.Is(err, context.Canceled) { - t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + errs := make(chan error, 1) + go func() { errs <- w.Wait(expired) }() + select { + case err := <-errs: + close(release) + w.MustWait(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + } + case <-ctx.Done(): + close(release) + t.Fatal("Wait() did not return after the context expired") } }) t.Run("Trap", func(t *testing.T) { t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + mClock := quartz.NewMock(t) trap := mClock.Trap().Now() defer trap.Close() - if _, err := trap.Wait(expired); !errors.Is(err, context.Canceled) { - t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + errs := make(chan error, 1) + go func() { + _, err := trap.Wait(expired) + errs <- err + }() + select { + case err := <-errs: + if !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + } + case <-ctx.Done(): + t.Fatal("Wait() did not return after the context expired") } }) } From bfd9246cb52b6cdd50c8a4bec0a4060a46582191 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 14:35:46 +0000 Subject: [PATCH 07/32] docs(mock.go): restore the original Wait and Release godoc The fix makes AdvanceWaiter.Wait and Call.Release match what their original godoc already said, and the expired-context caveat does not change how normal test code uses them. Trap.Wait keeps only a one-line summary; users cannot observe when a trapped call arrives, so a caveat about arrival gives them nothing to act on. --- mock.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/mock.go b/mock.go index b31d01c..e77dbe4 100644 --- a/mock.go +++ b/mock.go @@ -232,9 +232,7 @@ type AdvanceWaiter struct { ch chan struct{} } -// Wait for all timers and ticks to complete, or until context expires. If they have completed by -// the time the context expires, Wait returns nil. Triggered timers and ticks run on other -// goroutines, so a context that has already expired gives them no time to complete. +// Wait for all timers and ticks to complete, or until context expires. func (w AdvanceWaiter) Wait(ctx context.Context) error { select { case <-w.ch: @@ -692,9 +690,7 @@ type Call struct { } // Release the call and wait for it to complete. If the provided context expires before the call completes, it returns -// an error. If the call has completed by the time the context expires, Release returns nil. -// The call completes only after it is released, so a context that has already expired gives it -// no time to complete. +// an error. // // IMPORTANT: If a call is trapped by more than one trap, they all must release the call before it can complete, and // they must do so from different goroutines. @@ -816,9 +812,7 @@ func (t *Trap) callReleased() { var ErrTrapClosed = errors.New("trap closed") -// Wait for a call to be trapped, or until the context expires or the trap is closed. If a call -// has arrived by the time the context expires, Wait returns it. Trapped calls arrive from other -// goroutines, so a context that has already expired gives them no time to arrive. +// Wait for a call to be trapped, or until the context expires or the trap is closed. func (t *Trap) Wait(ctx context.Context) (*Call, error) { var a *apiCall select { From 60c96d5558449f5ce06908c189674e558cc3a3fd Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 14:36:11 +0000 Subject: [PATCH 08/32] test(mock_test.go): drop the AdvanceWaiter.MustWait subtest MustWait now calls Wait, so the Wait subtest covers the behavior. The other Must* wrappers, Trap.MustWait and Call.MustRelease, have no separate test either. --- mock_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/mock_test.go b/mock_test.go index 82e9952..0e62ef6 100644 --- a/mock_test.go +++ b/mock_test.go @@ -606,16 +606,6 @@ func TestAdvanceWaiter_CompletedWithExpiredContext(t *testing.T) { t.Fatalf("Wait() = %v, want nil", err) } }) - - t.Run("MustWait", func(t *testing.T) { - t.Parallel() - tb := &captureFailTB{TB: t} - mClock := quartz.NewMock(tb) - mClock.Advance(time.Second).MustWait(ctx) - if tb.failed { - t.Fatal("MustWait failed the test after the advance completed") - } - }) } // TestExpiredContext_WorkPending verifies that waits still return the context error when the From 41b470bcf27da0421247f41427a31ccf6386e3a4 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 3 Nov 2026 14:36:47 +0000 Subject: [PATCH 09/32] test(mock_test.go): group expired-context tests by method Each method's expired-context coverage now lives under one name: TestAdvanceWaiter_ExpiredContext with Completed and Pending subtests, and TestTrapWait_ExpiredContextNoCall. The test docs state the contract instead of the regression that prompted them. --- mock_test.go | 67 +++++++++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 35 deletions(-) diff --git a/mock_test.go b/mock_test.go index 0e62ef6..3498729 100644 --- a/mock_test.go +++ b/mock_test.go @@ -591,31 +591,24 @@ func TestTimerReset_NegativeDuration(t *testing.T) { } } -// TestAdvanceWaiter_CompletedWithExpiredContext verifies that a completed advance is reported as -// complete when the context has also expired, rather than at random. -func TestAdvanceWaiter_CompletedWithExpiredContext(t *testing.T) { +// TestAdvanceWaiter_ExpiredContext verifies how Wait behaves when the context has already expired: +// it returns nil if the advance has completed, and returns the context error, without blocking, if +// the advance has not completed. +func TestAdvanceWaiter_ExpiredContext(t *testing.T) { t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) + expired, cancel := context.WithCancel(context.Background()) cancel() - t.Run("Wait", func(t *testing.T) { + t.Run("Completed", func(t *testing.T) { t.Parallel() mClock := quartz.NewMock(t) // With no timers or tickers, the advance completes before Advance returns. - if err := mClock.Advance(time.Second).Wait(ctx); err != nil { + if err := mClock.Advance(time.Second).Wait(expired); err != nil { t.Fatalf("Wait() = %v, want nil", err) } }) -} -// TestExpiredContext_WorkPending verifies that waits still return the context error when the -// context has expired and the awaited work has not happened. -func TestExpiredContext_WorkPending(t *testing.T) { - t.Parallel() - expired, cancel := context.WithCancel(context.Background()) - cancel() - - t.Run("AdvanceWaiter", func(t *testing.T) { + t.Run("Pending", func(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() @@ -639,29 +632,33 @@ func TestExpiredContext_WorkPending(t *testing.T) { t.Fatal("Wait() did not return after the context expired") } }) +} - t.Run("Trap", func(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() +// TestTrapWait_ExpiredContextNoCall verifies that Wait returns the context error, without blocking, +// when the context has expired and no call has been trapped. +func TestTrapWait_ExpiredContextNoCall(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + expired, cancelExpired := context.WithCancel(context.Background()) + cancelExpired() - mClock := quartz.NewMock(t) - trap := mClock.Trap().Now() - defer trap.Close() - errs := make(chan error, 1) - go func() { - _, err := trap.Wait(expired) - errs <- err - }() - select { - case err := <-errs: - if !errors.Is(err, context.Canceled) { - t.Fatalf("Wait() = %v, want %v", err, context.Canceled) - } - case <-ctx.Done(): - t.Fatal("Wait() did not return after the context expired") + mClock := quartz.NewMock(t) + trap := mClock.Trap().Now() + defer trap.Close() + errs := make(chan error, 1) + go func() { + _, err := trap.Wait(expired) + errs <- err + }() + select { + case err := <-errs: + if !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) } - }) + case <-ctx.Done(): + t.Fatal("Wait() did not return after the context expired") + } } func TestNoLeak_Go123(t *testing.T) { From f24e6230ceaa6528bda1687c5d2474ed9a40f9d2 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 11:05:12 +0000 Subject: [PATCH 10/32] fix: prefer the context error in AdvanceWaiter.Wait Preferring completion turned a wrong wait context into a flake. With that the same Advance expires failed 10000/10000 without this PR, but only 23/10000 with it (about 6000/10000 under -race). The same holds for t.Context() used in a cleanup and for a test timeout used up by time.Sleep in a synctest bubble. Wait now returns ctx.Err() whenever the context is done, so the mistake fails every time at the line that made it. Done still reports completion without a deadline. #30's mock contexts, mClock.Advance(time.Second).Wait(ctx) on a context --- mock.go | 19 ++++++-------- mock_test.go | 74 +++++++++++++++++++++++++++------------------------- 2 files changed, 46 insertions(+), 47 deletions(-) diff --git a/mock.go b/mock.go index e77dbe4..56384b9 100644 --- a/mock.go +++ b/mock.go @@ -232,25 +232,22 @@ type AdvanceWaiter struct { ch chan struct{} } -// Wait for all timers and ticks to complete, or until context expires. +// Wait for all timers and ticks to complete, or until context expires. If the context has expired, +// Wait returns its error even if the timers and ticks have also completed. To check whether they +// have completed without a deadline, use Done. func (w AdvanceWaiter) Wait(ctx context.Context) error { select { case <-w.ch: - return nil case <-ctx.Done(): - // select picks randomly among ready cases; report completion if it also happened. - select { - case <-w.ch: - return nil - default: - return ctx.Err() - } } + // select picks a random ready case. Checking ctx afterwards makes an expired context fail the + // same way whether or not the timers and ticks completed first. + return ctx.Err() } // MustWait waits for all timers and ticks to complete, and fails the test immediately if the -// context completes first. MustWait must be called from the goroutine running the test or -// benchmark, similar to `t.FailNow()`. +// context expires, even if they have also completed. MustWait must be called from the goroutine +// running the test or benchmark, similar to `t.FailNow()`. func (w AdvanceWaiter) MustWait(ctx context.Context) { w.tb.Helper() if err := w.Wait(ctx); err != nil { diff --git a/mock_test.go b/mock_test.go index 3498729..182bedb 100644 --- a/mock_test.go +++ b/mock_test.go @@ -591,47 +591,49 @@ func TestTimerReset_NegativeDuration(t *testing.T) { } } -// TestAdvanceWaiter_ExpiredContext verifies how Wait behaves when the context has already expired: -// it returns nil if the advance has completed, and returns the context error, without blocking, if -// the advance has not completed. +// TestAdvanceWaiter_ExpiredContext verifies that Wait returns the context error, without blocking, +// when the context has expired, whether or not the advance has completed. func TestAdvanceWaiter_ExpiredContext(t *testing.T) { t.Parallel() - expired, cancel := context.WithCancel(context.Background()) - cancel() - - t.Run("Completed", func(t *testing.T) { - t.Parallel() - mClock := quartz.NewMock(t) + // The deadline passed before Wait, as when the test's timeout ran out first. + expired, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + t.Cleanup(cancel) + + for _, tt := range []struct { + name string + pending bool + }{ // With no timers or tickers, the advance completes before Advance returns. - if err := mClock.Advance(time.Second).Wait(expired); err != nil { - t.Fatalf("Wait() = %v, want nil", err) - } - }) - - t.Run("Pending", func(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - mClock := quartz.NewMock(t) - release := make(chan struct{}) - mClock.AfterFunc(time.Second, func() { <-release }) - w := mClock.Advance(time.Second) + {name: "Completed"}, // The advance cannot complete until the AfterFunc returns. - errs := make(chan error, 1) - go func() { errs <- w.Wait(expired) }() - select { - case err := <-errs: - close(release) - w.MustWait(ctx) - if !errors.Is(err, context.Canceled) { - t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + {name: "Pending", pending: true}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + mClock := quartz.NewMock(t) + release := make(chan struct{}) + if tt.pending { + mClock.AfterFunc(time.Second, func() { <-release }) } - case <-ctx.Done(): - close(release) - t.Fatal("Wait() did not return after the context expired") - } - }) + w := mClock.Advance(time.Second) + errs := make(chan error, 1) + go func() { errs <- w.Wait(expired) }() + select { + case err := <-errs: + close(release) + w.MustWait(ctx) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) + } + case <-ctx.Done(): + close(release) + t.Fatal("Wait() did not return after the context expired") + } + }) + } } // TestTrapWait_ExpiredContextNoCall verifies that Wait returns the context error, without blocking, From a6f7b1fcf7a5763b9d492cac0c29f22a05698ace Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 11:07:32 +0000 Subject: [PATCH 11/32] fix: prefer the context error in Trap.Wait Like AdvanceWaiter.Wait, Trap.Wait now fails every time on an expired context instead of only when select happens to pick ctx.Done(). It checks the context before the select rather than after it: once Wait has received a call it must return it, because the code under test stays blocked until the call is released. A call that is not taken stays in the trap for the next Wait. This also makes a closed trap plus an expired context return the context error every time, where it used to return ErrTrapClosed or the context error at random. --- mock.go | 44 ++++++++++++++++--------------- mock_internal_test.go | 17 +++++++++--- mock_test.go | 60 +++++++++++++++++++++++++++---------------- 3 files changed, 74 insertions(+), 47 deletions(-) diff --git a/mock.go b/mock.go index 56384b9..d13b2e1 100644 --- a/mock.go +++ b/mock.go @@ -809,33 +809,35 @@ func (t *Trap) callReleased() { var ErrTrapClosed = errors.New("trap closed") -// Wait for a call to be trapped, or until the context expires or the trap is closed. +// Wait for a call to be trapped, or until the context expires or the trap is closed. If the +// context has already expired, Wait returns its error and leaves any trapped call for the next +// Wait. func (t *Trap) Wait(ctx context.Context) (*Call, error) { - var a *apiCall + // select picks a random ready case, so check ctx first to make an expired context fail the same + // way whether or not a call is ready or the trap is closed. A call received below must be + // returned: the code under test stays blocked until the call is released. + if err := ctx.Err(); err != nil { + return nil, err + } select { case <-ctx.Done(): - // select picks randomly among ready cases; report a ready call if there is one. - select { - case a = <-t.calls: - default: - return nil, ctx.Err() - } + return nil, ctx.Err() case <-t.done: return nil, ErrTrapClosed - case a = <-t.calls: - } - c := &Call{ - Time: a.Time, - Duration: a.Duration, - Tags: a.Tags, - apiCall: a, - trap: t, - tb: t.mock.tb, + case a := <-t.calls: + c := &Call{ + Time: a.Time, + Duration: a.Duration, + Tags: a.Tags, + apiCall: a, + trap: t, + tb: t.mock.tb, + } + t.mu.Lock() + defer t.mu.Unlock() + t.unreleasedCalls++ + return c, nil } - t.mu.Lock() - defer t.mu.Unlock() - t.unreleasedCalls++ - return c, nil } // MustWait calls Wait() and then if there is an error, immediately fails the diff --git a/mock_internal_test.go b/mock_internal_test.go index 101ef30..2a91c71 100644 --- a/mock_internal_test.go +++ b/mock_internal_test.go @@ -2,17 +2,23 @@ package quartz import ( "context" + "errors" "testing" + "time" ) // The public API cannot make a trapped call ready, or complete, before Trap.Wait or Call.Release // observes the context: catch delivers each call from its own goroutine, and a call completes only // after its last release. These tests build that state directly. +// TestTrapWait_CallReadyWithExpiredContext verifies that Wait returns the context error when the +// context has expired, even if a call is ready, and leaves the call for the next Wait. func TestTrapWait_CallReadyWithExpiredContext(t *testing.T) { t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) - cancel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + expired, cancelExpired := context.WithCancel(context.Background()) + cancelExpired() // A buffered channel holding the call stands in for a catch goroutine blocked sending it. trap := &Trap{ @@ -24,12 +30,15 @@ func TestTrapWait_CallReadyWithExpiredContext(t *testing.T) { want := newCall(clockFunctionNow, nil) trap.calls <- want + if _, err := trap.Wait(expired); !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() error = %v, want %v", err, context.Canceled) + } c, err := trap.Wait(ctx) if err != nil { - t.Fatalf("Wait() error = %v, want nil", err) + t.Fatalf("second Wait() error = %v, want nil", err) } if c.apiCall != want { - t.Fatal("Wait() returned a different call than the ready one") + t.Fatal("second Wait() returned a different call than the ready one") } } diff --git a/mock_test.go b/mock_test.go index 182bedb..a3b73fa 100644 --- a/mock_test.go +++ b/mock_test.go @@ -636,30 +636,46 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { } } -// TestTrapWait_ExpiredContextNoCall verifies that Wait returns the context error, without blocking, -// when the context has expired and no call has been trapped. -func TestTrapWait_ExpiredContextNoCall(t *testing.T) { +// TestTrapWait_ExpiredContext verifies that Wait returns the context error, without blocking, when +// the context has expired, even if the trap is closed. +func TestTrapWait_ExpiredContext(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - expired, cancelExpired := context.WithCancel(context.Background()) - cancelExpired() + expired, cancel := context.WithCancel(context.Background()) + cancel() - mClock := quartz.NewMock(t) - trap := mClock.Trap().Now() - defer trap.Close() - errs := make(chan error, 1) - go func() { - _, err := trap.Wait(expired) - errs <- err - }() - select { - case err := <-errs: - if !errors.Is(err, context.Canceled) { - t.Fatalf("Wait() = %v, want %v", err, context.Canceled) - } - case <-ctx.Done(): - t.Fatal("Wait() did not return after the context expired") + for _, tt := range []struct { + name string + closed bool + }{ + {name: "NoCall"}, + {name: "TrapClosed", closed: true}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + mClock := quartz.NewMock(t) + trap := mClock.Trap().Now() + if tt.closed { + trap.Close() + } else { + defer trap.Close() + } + errs := make(chan error, 1) + go func() { + _, err := trap.Wait(expired) + errs <- err + }() + select { + case err := <-errs: + if !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + } + case <-ctx.Done(): + t.Fatal("Wait() did not return after the context expired") + } + }) } } From a0495acbf58223114a1b1d56a9aebf24d45adf83 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 11:08:27 +0000 Subject: [PATCH 12/32] fix: count a call as released even when Release times out Release released the call before waiting, but only decremented the trap's unreleased count after the call completed. When the wait failed, for example while another trap still held the call, Trap.Close later reported "Closed() with 1 unreleased calls" for a call the test had released. Test_MultipleTrapsDeadlock now uses an already-expired context instead of waiting two seconds, and checks that Close reports nothing. --- mock.go | 3 ++- mock_test.go | 48 ++++++++++++++++++++++++++++++++++-------------- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/mock.go b/mock.go index d13b2e1..b6e75d2 100644 --- a/mock.go +++ b/mock.go @@ -693,6 +693,8 @@ type Call struct { // they must do so from different goroutines. func (c *Call) Release(ctx context.Context) error { c.apiCall.releases.Done() + // The call is released now, whether or not it completes before ctx expires. + c.trap.callReleased() select { case <-ctx.Done(): // select picks randomly among ready cases; report completion if it also happened. @@ -704,7 +706,6 @@ func (c *Call) Release(ctx context.Context) error { case <-c.apiCall.complete: // OK } - c.trap.callReleased() return nil } diff --git a/mock_test.go b/mock_test.go index a3b73fa..9a312ee 100644 --- a/mock_test.go +++ b/mock_test.go @@ -372,25 +372,45 @@ func Test_MultipleTraps(t *testing.T) { } } +// Test_MultipleTrapsDeadlock verifies that Release fails while another trap still holds the call, +// and that the call still counts as released, so Close does not report it. func Test_MultipleTrapsDeadlock(t *testing.T) { t.Parallel() - tRunFail(t, func(t testing.TB) { - testCtx, testCancel := context.WithTimeout(context.Background(), 2*time.Second) - defer testCancel() - mClock := quartz.NewMock(t) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + expired, cancelExpired := context.WithCancel(context.Background()) + cancelExpired() - trap0 := mClock.Trap().Now("0") - defer trap0.Close() - trap1 := mClock.Trap().Now("1") - defer trap1.Close() + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) + trap0 := mClock.Trap().Now("0") + trap1 := mClock.Trap().Now("1") - go func() { - mClock.Now("0", "1") - }() + go func() { + mClock.Now("0", "1") + }() - c0 := trap0.MustWait(testCtx) - c0.MustRelease(testCtx) // deadlocks, test failure - }) + c0, err := trap0.Wait(ctx) + if err != nil { + t.Fatal(err) + } + // The call cannot complete until trap1 releases it too. + if err := c0.Release(expired); !errors.Is(err, context.Canceled) { + t.Fatalf("Release() = %v, want %v", err, context.Canceled) + } + c1, err := trap1.Wait(ctx) + if err != nil { + t.Fatal(err) + } + if err := c1.Release(ctx); err != nil { + t.Fatal(err) + } + + trap0.Close() + trap1.Close() + if tb.failed { + t.Fatal("Close() reported a failure after both calls were released") + } } func Test_UnreleasedCalls(t *testing.T) { From d3a43228bc97a771d0896da88fb3cc3c16cb756d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 11:10:15 +0000 Subject: [PATCH 13/32] fix: hint at other traps only when more than one caught the call Every failed Release asked "did more than one trap capture the call?", even when one trap caught it, so the hint pointed at a cause that could not apply. The mock now records how many traps caught each call. Release names that count when it is above one and otherwise only says the call did not complete after release. --- mock.go | 13 ++++++++--- mock_internal_test.go | 54 ++++++++++++++++++++++++++++++++----------- mock_test.go | 8 ++++--- 3 files changed, 56 insertions(+), 19 deletions(-) diff --git a/mock.go b/mock.go index b6e75d2..68356fe 100644 --- a/mock.go +++ b/mock.go @@ -211,6 +211,7 @@ func (m *Mock) matchCallLocked(c *apiCall) { if len(traps) == 0 { return } + c.traps = len(traps) c.releases.Add(len(traps)) m.mu.Unlock() for _, t := range traps { @@ -642,6 +643,8 @@ type apiCall struct { fn clockFunction releases sync.WaitGroup complete chan struct{} + // traps is the number of traps that caught the call. + traps int } func (a *apiCall) String() string { @@ -700,13 +703,17 @@ func (c *Call) Release(ctx context.Context) error { // select picks randomly among ready cases; report completion if it also happened. select { case <-c.apiCall.complete: + return nil default: - return fmt.Errorf("timed out waiting for release; did more than one trap capture the call?: %w", ctx.Err()) } + if c.apiCall.traps > 1 { + return fmt.Errorf("timed out waiting for the call to complete; %d traps caught it and each "+ + "must release it: %w", c.apiCall.traps, ctx.Err()) + } + return fmt.Errorf("timed out waiting for the call to complete after release: %w", ctx.Err()) case <-c.apiCall.complete: - // OK + return nil } - return nil } // MustRelease releases the call and waits for it to complete. If the provided context expires before the call diff --git a/mock_internal_test.go b/mock_internal_test.go index 2a91c71..b07a22d 100644 --- a/mock_internal_test.go +++ b/mock_internal_test.go @@ -3,13 +3,14 @@ package quartz import ( "context" "errors" + "strings" "testing" "time" ) -// The public API cannot make a trapped call ready, or complete, before Trap.Wait or Call.Release -// observes the context: catch delivers each call from its own goroutine, and a call completes only -// after its last release. These tests build that state directly. +// The public API cannot control when a trapped call becomes ready, or when a released call +// completes: catch delivers each call from its own goroutine, and a released call completes on the +// goroutine of the code under test. These tests build those states directly. // TestTrapWait_CallReadyWithExpiredContext verifies that Wait returns the context error when the // context has expired, even if a call is ready, and leaves the call for the next Wait. @@ -42,19 +43,46 @@ func TestTrapWait_CallReadyWithExpiredContext(t *testing.T) { } } -func TestCallRelease_CompletedWithExpiredContext(t *testing.T) { +// TestCallRelease_ExpiredContext verifies that Release returns nil when the context has expired but +// the call has completed, and otherwise returns the context error. With one trap, the error does +// not suggest that other traps hold the call. +func TestCallRelease_ExpiredContext(t *testing.T) { t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) + expired, cancel := context.WithCancel(context.Background()) cancel() - a := newCall(clockFunctionNow, nil) - a.releases.Add(1) - // Closing complete first stands in for the trapped call returning after Release calls - // releases.Done and before Release checks the context. - close(a.complete) - c := &Call{tb: t, apiCall: a, trap: &Trap{unreleasedCalls: 1}} + for _, tt := range []struct { + name string + completed bool + }{ + // Closing complete first stands in for the trapped call returning after Release calls + // releases.Done and before Release checks the context. + {name: "Completed", completed: true}, + {name: "NotCompleted"}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + a := newCall(clockFunctionNow, nil) + a.traps = 1 + a.releases.Add(1) + if tt.completed { + close(a.complete) + } + c := &Call{tb: t, apiCall: a, trap: &Trap{unreleasedCalls: 1}} - if err := c.Release(ctx); err != nil { - t.Fatalf("Release() = %v, want nil", err) + err := c.Release(expired) + if tt.completed { + if err != nil { + t.Fatalf("Release() = %v, want nil", err) + } + return + } + if !errors.Is(err, context.Canceled) { + t.Fatalf("Release() = %v, want %v", err, context.Canceled) + } + if strings.Contains(err.Error(), "trap") { + t.Fatalf("Release() = %q, want no mention of other traps", err) + } + }) } } diff --git a/mock_test.go b/mock_test.go index 9a312ee..c5b95d7 100644 --- a/mock_test.go +++ b/mock_test.go @@ -373,7 +373,8 @@ func Test_MultipleTraps(t *testing.T) { } // Test_MultipleTrapsDeadlock verifies that Release fails while another trap still holds the call, -// and that the call still counts as released, so Close does not report it. +// with an error that names the number of traps, and that the call still counts as released, so +// Close does not report it. func Test_MultipleTrapsDeadlock(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -395,8 +396,9 @@ func Test_MultipleTrapsDeadlock(t *testing.T) { t.Fatal(err) } // The call cannot complete until trap1 releases it too. - if err := c0.Release(expired); !errors.Is(err, context.Canceled) { - t.Fatalf("Release() = %v, want %v", err, context.Canceled) + err = c0.Release(expired) + if !errors.Is(err, context.Canceled) || !strings.Contains(err.Error(), "2 traps") { + t.Fatalf("Release() = %v, want %v naming 2 traps", err, context.Canceled) } c1, err := trap1.Wait(ctx) if err != nil { From 9b8638b131cc767f53b6f0a2705df47459238ad6 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 11:10:30 +0000 Subject: [PATCH 14/32] docs(mock.go): explain why Release still prefers completion AdvanceWaiter.Wait and Trap.Wait now return the context error whenever the context is done, and Release does not. The reason is not visible in the code: Release acts before it waits, and a second Release panics with "sync: negative WaitGroup counter", so its result is the test's only report of whether the call completed. --- mock.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mock.go b/mock.go index 68356fe..f9dbb68 100644 --- a/mock.go +++ b/mock.go @@ -700,7 +700,9 @@ func (c *Call) Release(ctx context.Context) error { c.trap.callReleased() select { case <-ctx.Done(): - // select picks randomly among ready cases; report completion if it also happened. + // Unlike the waits, Release reports completion even if ctx has also expired: the release + // has already happened and cannot be repeated, so the result is the only way the test + // learns whether the call completed. select picks a random ready case, so check again. select { case <-c.apiCall.complete: return nil From 1396db26888740a790fcc63f10f71f85bef8125f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 11:56:46 +0000 Subject: [PATCH 15/32] fix: count a release before letting the trapped call return Release called releases.Done() and only then decremented the trap's unreleased count. Once Done runs, the code under test can return and the test can call Trap.Close, which reads the count. With Release on another goroutine, -race reported a data race between Close and callReleased on every run. On main, where the decrement came after the call completed, Close also reported "Closed() with 1 unreleased calls" in 4987 of 5000 runs of the same test. Test_UnreleasedCalls gets a subtest for this next to the existing unreleased-call case. --- mock.go | 5 +++-- mock_test.go | 61 +++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/mock.go b/mock.go index f9dbb68..b0ba130 100644 --- a/mock.go +++ b/mock.go @@ -695,9 +695,10 @@ type Call struct { // IMPORTANT: If a call is trapped by more than one trap, they all must release the call before it can complete, and // they must do so from different goroutines. func (c *Call) Release(ctx context.Context) error { - c.apiCall.releases.Done() - // The call is released now, whether or not it completes before ctx expires. + // Count the call as released before letting it return: once releases.Done runs, the code under + // test can return and the test can call Trap.Close, which must see the release. c.trap.callReleased() + c.apiCall.releases.Done() select { case <-ctx.Done(): // Unlike the waits, Release reports completion even if ctx has also expired: the release diff --git a/mock_test.go b/mock_test.go index c5b95d7..fb8c131 100644 --- a/mock_test.go +++ b/mock_test.go @@ -415,24 +415,61 @@ func Test_MultipleTrapsDeadlock(t *testing.T) { } } +// Test_UnreleasedCalls verifies that Close fails the test only when a trapped call has not been +// released. func Test_UnreleasedCalls(t *testing.T) { t.Parallel() - tRunFail(t, func(t testing.TB) { - testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second) - defer testCancel() - mClock := quartz.NewMock(t) - trap := mClock.Trap().Now() - defer trap.Close() + t.Run("NotReleased", func(t *testing.T) { + t.Parallel() + tRunFail(t, func(t testing.TB) { + testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer testCancel() + mClock := quartz.NewMock(t) - go func() { - _ = mClock.Now() - }() + trap := mClock.Trap().Now() + defer trap.Close() + + go func() { + _ = mClock.Now() + }() + + c := trap.MustWait(testCtx) // missing release + trap.Close() // detect unreleased call and fail - c := trap.MustWait(testCtx) // missing release - trap.Close() // detect unreleased call and fail + c.Release(testCtx) // clean up goroutine + }) + }) + + // The call returns before Release, running on another goroutine, returns. Close right after + // the call returns must see it as released. + t.Run("ReleasedOnAnotherGoroutine", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() - c.Release(testCtx) // clean up goroutine + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) + trap := mClock.Trap().Now() + returned := make(chan struct{}) + go func() { + defer close(returned) + mClock.Now() + }() + c, err := trap.Wait(ctx) + if err != nil { + t.Fatal(err) + } + released := make(chan error, 1) + go func() { released <- c.Release(ctx) }() + <-returned + trap.Close() + if err := <-released; err != nil { + t.Fatal(err) + } + if tb.failed { + t.Fatal("Close() reported a failure after the released call returned") + } }) } From a174e8460cb7f2fc4d0d7143219083db91b28c6a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 11:57:19 +0000 Subject: [PATCH 16/32] fix(mock.go): read the unreleased count under the trap lock in Close The trap's mu protects unreleasedCalls, but Close read it twice without mu: once to decide whether to fail and again for the message. On main that produced "Closed() with 0 unreleased calls" in 11 of 1000 runs of the Test_UnreleasedCalls release subtest, a failure whose message contradicts itself. Close now reads the count once under mu, so it never races with Trap.Wait or Release, whatever synchronization the test uses. Close holds Mock.mu when it takes mu. Nothing takes Mock.mu while holding mu, and the field comment now says to keep it that way. --- mock.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/mock.go b/mock.go index b0ba130..036aabf 100644 --- a/mock.go +++ b/mock.go @@ -762,7 +762,8 @@ type Trap struct { calls chan *apiCall done chan struct{} - // mu protects the unreleasedCalls count + // mu protects the unreleasedCalls count. Close takes mu while holding Mock.mu, so do not take + // Mock.mu while holding mu. mu sync.Mutex unreleasedCalls int } @@ -800,9 +801,12 @@ func (t *Trap) Close() { return // already closed default: } - if t.unreleasedCalls != 0 { + t.mu.Lock() + unreleased := t.unreleasedCalls + t.mu.Unlock() + if unreleased != 0 { t.mock.tb.Helper() - t.mock.tb.Errorf("%s Closed() with %d unreleased calls", t, t.unreleasedCalls) + t.mock.tb.Errorf("%s Closed() with %d unreleased calls", t, unreleased) } for i, tr := range t.mock.traps { if t == tr { From ed5bfee69b986050cbba7ffd1257d0b5d0693798 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 11:58:52 +0000 Subject: [PATCH 17/32] test: repeat the expired-context checks that a random select can pass A regression to the random select on main made each of these checks fail in only about half of the runs, so one CI run could miss it. Each check now runs 100 times per test. Against main's mock.go they fail 200 of 200 runs, and each test still takes under a millisecond. --- mock_internal_test.go | 65 ++++++++++++++++++++++++------------------- mock_test.go | 62 +++++++++++++++++++++++------------------ 2 files changed, 71 insertions(+), 56 deletions(-) diff --git a/mock_internal_test.go b/mock_internal_test.go index b07a22d..efcf280 100644 --- a/mock_internal_test.go +++ b/mock_internal_test.go @@ -29,17 +29,20 @@ func TestTrapWait_CallReadyWithExpiredContext(t *testing.T) { done: make(chan struct{}), } want := newCall(clockFunctionNow, nil) - trap.calls <- want - - if _, err := trap.Wait(expired); !errors.Is(err, context.Canceled) { - t.Fatalf("Wait() error = %v, want %v", err, context.Canceled) - } - c, err := trap.Wait(ctx) - if err != nil { - t.Fatalf("second Wait() error = %v, want nil", err) - } - if c.apiCall != want { - t.Fatal("second Wait() returned a different call than the ready one") + // A Wait that picks at random between the ready call and ctx.Done fails only half the + // iterations, so repeat. + for range 100 { + trap.calls <- want + if _, err := trap.Wait(expired); !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() error = %v, want %v", err, context.Canceled) + } + c, err := trap.Wait(ctx) + if err != nil { + t.Fatalf("second Wait() error = %v, want nil", err) + } + if c.apiCall != want { + t.Fatal("second Wait() returned a different call than the ready one") + } } } @@ -62,26 +65,30 @@ func TestCallRelease_ExpiredContext(t *testing.T) { } { t.Run(tt.name, func(t *testing.T) { t.Parallel() - a := newCall(clockFunctionNow, nil) - a.traps = 1 - a.releases.Add(1) - if tt.completed { - close(a.complete) - } - c := &Call{tb: t, apiCall: a, trap: &Trap{unreleasedCalls: 1}} + // A Release that picks at random between completion and ctx.Done fails only half the + // iterations, so repeat. + for range 100 { + a := newCall(clockFunctionNow, nil) + a.traps = 1 + a.releases.Add(1) + if tt.completed { + close(a.complete) + } + c := &Call{tb: t, apiCall: a, trap: &Trap{unreleasedCalls: 1}} - err := c.Release(expired) - if tt.completed { - if err != nil { - t.Fatalf("Release() = %v, want nil", err) + err := c.Release(expired) + if tt.completed { + if err != nil { + t.Fatalf("Release() = %v, want nil", err) + } + continue + } + if !errors.Is(err, context.Canceled) { + t.Fatalf("Release() = %v, want %v", err, context.Canceled) + } + if strings.Contains(err.Error(), "trap") { + t.Fatalf("Release() = %q, want no mention of other traps", err) } - return - } - if !errors.Is(err, context.Canceled) { - t.Fatalf("Release() = %v, want %v", err, context.Canceled) - } - if strings.Contains(err.Error(), "trap") { - t.Fatalf("Release() = %q, want no mention of other traps", err) } }) } diff --git a/mock_test.go b/mock_test.go index fb8c131..691785c 100644 --- a/mock_test.go +++ b/mock_test.go @@ -673,23 +673,27 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { defer cancel() mClock := quartz.NewMock(t) - release := make(chan struct{}) - if tt.pending { - mClock.AfterFunc(time.Second, func() { <-release }) - } - w := mClock.Advance(time.Second) - errs := make(chan error, 1) - go func() { errs <- w.Wait(expired) }() - select { - case err := <-errs: - close(release) - w.MustWait(ctx) - if !errors.Is(err, context.DeadlineExceeded) { - t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) + // A Wait that picks at random between completion and ctx.Done fails only half the + // iterations, so repeat. + for range 100 { + release := make(chan struct{}) + if tt.pending { + mClock.AfterFunc(time.Second, func() { <-release }) + } + w := mClock.Advance(time.Second) + errs := make(chan error, 1) + go func() { errs <- w.Wait(expired) }() + select { + case err := <-errs: + close(release) + w.MustWait(ctx) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) + } + case <-ctx.Done(): + close(release) + t.Fatal("Wait() did not return after the context expired") } - case <-ctx.Done(): - close(release) - t.Fatal("Wait() did not return after the context expired") } }) } @@ -721,18 +725,22 @@ func TestTrapWait_ExpiredContext(t *testing.T) { } else { defer trap.Close() } - errs := make(chan error, 1) - go func() { - _, err := trap.Wait(expired) - errs <- err - }() - select { - case err := <-errs: - if !errors.Is(err, context.Canceled) { - t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + // A Wait that picks at random between the closed trap and ctx.Done fails only half the + // iterations, so repeat. + for range 100 { + errs := make(chan error, 1) + go func() { + _, err := trap.Wait(expired) + errs <- err + }() + select { + case err := <-errs: + if !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + } + case <-ctx.Done(): + t.Fatal("Wait() did not return after the context expired") } - case <-ctx.Done(): - t.Fatal("Wait() did not return after the context expired") } }) } From 5db723437ae1312174fe2870e34ba1c1f4efbb3a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 12:25:07 +0000 Subject: [PATCH 18/32] fix: prefer the context error in Call.Release too Release preferred completion on the grounds that its result was the only report of whether the call completed. That never applied when Release was entered with an expired context: the call completes only after this Release's own releases.Done(), so the completion check could only win when the code under test returned before the select ran. Release(expired) returned nil in 2 of 10000 runs, 28 under -race. Release now reads the context before releasing and returns its error if it had already expired. It still releases the call first, so the code under test is not left blocked. The internal Release tests checked a completed call before release, a state the mock cannot reach. TestCallRelease_ExpiredContext replaces them and Test_MultipleTrapsDeadlock with a public table: one trap, two traps, and two traps with a context that expires while Release waits. Each row also checks that the call returns and that Close reports nothing. --- mock.go | 31 ++++++----- mock_internal_test.go | 56 ++------------------ mock_test.go | 119 +++++++++++++++++++++++++++++------------- 3 files changed, 104 insertions(+), 102 deletions(-) diff --git a/mock.go b/mock.go index 036aabf..8ce5691 100644 --- a/mock.go +++ b/mock.go @@ -690,35 +690,38 @@ type Call struct { } // Release the call and wait for it to complete. If the provided context expires before the call completes, it returns -// an error. +// an error. If the context has already expired, Release still releases the call, and returns the error. // // IMPORTANT: If a call is trapped by more than one trap, they all must release the call before it can complete, and // they must do so from different goroutines. func (c *Call) Release(ctx context.Context) error { + // Read ctx before releasing: the call cannot complete before this release, so an expired ctx + // would otherwise pass only when the code under test returns before the select below. Release + // either way, or the code under test stays blocked. + expired := ctx.Err() // Count the call as released before letting it return: once releases.Done runs, the code under // test can return and the test can call Trap.Close, which must see the release. c.trap.callReleased() c.apiCall.releases.Done() + if expired != nil { + return c.releaseError(expired) + } select { case <-ctx.Done(): - // Unlike the waits, Release reports completion even if ctx has also expired: the release - // has already happened and cannot be repeated, so the result is the only way the test - // learns whether the call completed. select picks a random ready case, so check again. - select { - case <-c.apiCall.complete: - return nil - default: - } - if c.apiCall.traps > 1 { - return fmt.Errorf("timed out waiting for the call to complete; %d traps caught it and each "+ - "must release it: %w", c.apiCall.traps, ctx.Err()) - } - return fmt.Errorf("timed out waiting for the call to complete after release: %w", ctx.Err()) + return c.releaseError(ctx.Err()) case <-c.apiCall.complete: return nil } } +func (c *Call) releaseError(err error) error { + if c.apiCall.traps > 1 { + return fmt.Errorf("timed out waiting for the call to complete; %d traps caught it and each "+ + "must release it: %w", c.apiCall.traps, err) + } + return fmt.Errorf("timed out waiting for the call to complete after release: %w", err) +} + // MustRelease releases the call and waits for it to complete. If the provided context expires before the call // completes, it fails the test. // diff --git a/mock_internal_test.go b/mock_internal_test.go index efcf280..54cd203 100644 --- a/mock_internal_test.go +++ b/mock_internal_test.go @@ -3,17 +3,15 @@ package quartz import ( "context" "errors" - "strings" "testing" "time" ) -// The public API cannot control when a trapped call becomes ready, or when a released call -// completes: catch delivers each call from its own goroutine, and a released call completes on the -// goroutine of the code under test. These tests build those states directly. - // TestTrapWait_CallReadyWithExpiredContext verifies that Wait returns the context error when the // context has expired, even if a call is ready, and leaves the call for the next Wait. +// +// The public API cannot make a call ready before Wait checks the context, because catch delivers +// each call from its own goroutine, so this test builds that state directly. func TestTrapWait_CallReadyWithExpiredContext(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -45,51 +43,3 @@ func TestTrapWait_CallReadyWithExpiredContext(t *testing.T) { } } } - -// TestCallRelease_ExpiredContext verifies that Release returns nil when the context has expired but -// the call has completed, and otherwise returns the context error. With one trap, the error does -// not suggest that other traps hold the call. -func TestCallRelease_ExpiredContext(t *testing.T) { - t.Parallel() - expired, cancel := context.WithCancel(context.Background()) - cancel() - - for _, tt := range []struct { - name string - completed bool - }{ - // Closing complete first stands in for the trapped call returning after Release calls - // releases.Done and before Release checks the context. - {name: "Completed", completed: true}, - {name: "NotCompleted"}, - } { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - // A Release that picks at random between completion and ctx.Done fails only half the - // iterations, so repeat. - for range 100 { - a := newCall(clockFunctionNow, nil) - a.traps = 1 - a.releases.Add(1) - if tt.completed { - close(a.complete) - } - c := &Call{tb: t, apiCall: a, trap: &Trap{unreleasedCalls: 1}} - - err := c.Release(expired) - if tt.completed { - if err != nil { - t.Fatalf("Release() = %v, want nil", err) - } - continue - } - if !errors.Is(err, context.Canceled) { - t.Fatalf("Release() = %v, want %v", err, context.Canceled) - } - if strings.Contains(err.Error(), "trap") { - t.Fatalf("Release() = %q, want no mention of other traps", err) - } - } - }) - } -} diff --git a/mock_test.go b/mock_test.go index 691785c..acf62f8 100644 --- a/mock_test.go +++ b/mock_test.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "runtime/pprof" + "strconv" "strings" "testing" "time" @@ -372,46 +373,94 @@ func Test_MultipleTraps(t *testing.T) { } } -// Test_MultipleTrapsDeadlock verifies that Release fails while another trap still holds the call, -// with an error that names the number of traps, and that the call still counts as released, so -// Close does not report it. -func Test_MultipleTrapsDeadlock(t *testing.T) { +// TestCallRelease_ExpiredContext verifies that Release returns the context error when the context +// expires before the call completes, still releases the call so that it returns, and names the +// trap count only when more than one trap caught the call. Close must not report the call. +func TestCallRelease_ExpiredContext(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - expired, cancelExpired := context.WithCancel(context.Background()) - cancelExpired() + expired, cancel := context.WithCancel(context.Background()) + cancel() - tb := &captureFailTB{TB: t} - mClock := quartz.NewMock(tb) - trap0 := mClock.Trap().Now("0") - trap1 := mClock.Trap().Now("1") + for _, tt := range []struct { + name string + traps int + // whileWaiting gives Release a context that expires 10ms after Release is called, while + // Release waits for the other traps. The result is the same if it expires sooner. + whileWaiting bool + // hint must be in the error; if empty, the error must not mention traps. + hint string + }{ + {name: "OneTrap", traps: 1}, + {name: "TwoTraps", traps: 2, hint: "2 traps"}, + {name: "TwoTrapsWhileWaiting", traps: 2, whileWaiting: true, hint: "2 traps"}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() - go func() { - mClock.Now("0", "1") - }() + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) + var tags []string + var traps []*quartz.Trap + for i := range tt.traps { + tags = append(tags, strconv.Itoa(i)) + traps = append(traps, mClock.Trap().Now(tags[i])) + } + returned := make(chan struct{}) + go func() { + defer close(returned) + mClock.Now(tags...) + }() - c0, err := trap0.Wait(ctx) - if err != nil { - t.Fatal(err) - } - // The call cannot complete until trap1 releases it too. - err = c0.Release(expired) - if !errors.Is(err, context.Canceled) || !strings.Contains(err.Error(), "2 traps") { - t.Fatalf("Release() = %v, want %v naming 2 traps", err, context.Canceled) - } - c1, err := trap1.Wait(ctx) - if err != nil { - t.Fatal(err) - } - if err := c1.Release(ctx); err != nil { - t.Fatal(err) - } + c, err := traps[0].Wait(ctx) + if err != nil { + t.Fatal(err) + } + releaseCtx := expired + if tt.whileWaiting { + var cancelRelease context.CancelFunc + releaseCtx, cancelRelease = context.WithTimeout(ctx, 10*time.Millisecond) + defer cancelRelease() + } + errs := make(chan error, 1) + go func() { errs <- c.Release(releaseCtx) }() + select { + case err = <-errs: + case <-ctx.Done(): + t.Fatal("Release() did not return after its context expired") + } + if releaseCtx.Err() == nil || !errors.Is(err, releaseCtx.Err()) { + t.Fatalf("Release() = %v, want %v", err, releaseCtx.Err()) + } + if tt.hint == "" && strings.Contains(err.Error(), "trap") { + t.Fatalf("Release() = %q, want no mention of other traps", err) + } + if !strings.Contains(err.Error(), tt.hint) { + t.Fatalf("Release() = %q, want it to contain %q", err, tt.hint) + } - trap0.Close() - trap1.Close() - if tb.failed { - t.Fatal("Close() reported a failure after both calls were released") + for _, trap := range traps[1:] { + c, err := trap.Wait(ctx) + if err != nil { + t.Fatal(err) + } + if err := c.Release(ctx); err != nil { + t.Fatal(err) + } + } + select { + case <-returned: + case <-ctx.Done(): + t.Fatal("the call did not return after every trap released it") + } + for _, trap := range traps { + trap.Close() + } + if tb.failed { + t.Fatal("Close() reported a failure after every trap released the call") + } + }) } } From 0d34f2013588cf8b2f45ef47903cdb831e1ea3ef Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 12:26:46 +0000 Subject: [PATCH 19/32] test(mock_test.go): repeat only checks that a random select can pass The 100-iteration loops ran in every row, but only Completed and TrapClosed involve a select that can pick either case; Pending fails the same way every run. TrapWait_ExpiredContext/NoCall passed on main and under every mutation, because Trap.Wait returns before its select when the context has already expired. ExpiresWhileWaiting replaces it: the context is live when Wait is called, so it covers the select's ctx.Done case, which no test reached before. The ReleasedOnAnotherGoroutine comment now says what that subtest catches. --- mock_test.go | 156 ++++++++++++++++++++++++++------------------------- 1 file changed, 79 insertions(+), 77 deletions(-) diff --git a/mock_test.go b/mock_test.go index acf62f8..39c1a41 100644 --- a/mock_test.go +++ b/mock_test.go @@ -490,8 +490,8 @@ func Test_UnreleasedCalls(t *testing.T) { }) }) - // The call returns before Release, running on another goroutine, returns. Close right after - // the call returns must see it as released. + // Release must count the call as released before the call returns, not after it completes: the + // test may call Close as soon as the call returns. t.Run("ReleasedOnAnotherGoroutine", func(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -707,92 +707,94 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { expired, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) t.Cleanup(cancel) - for _, tt := range []struct { - name string - pending bool - }{ - // With no timers or tickers, the advance completes before Advance returns. - {name: "Completed"}, - // The advance cannot complete until the AfterFunc returns. - {name: "Pending", pending: true}, - } { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() + t.Run("Completed", func(t *testing.T) { + t.Parallel() + mClock := quartz.NewMock(t) + // A Wait that picks at random between completion and ctx.Done fails only half the + // iterations, so repeat. + for range 100 { + // With no timers or tickers, the advance completes before Advance returns. + err := mClock.Advance(time.Second).Wait(expired) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) + } + } + }) - mClock := quartz.NewMock(t) - // A Wait that picks at random between completion and ctx.Done fails only half the - // iterations, so repeat. - for range 100 { - release := make(chan struct{}) - if tt.pending { - mClock.AfterFunc(time.Second, func() { <-release }) - } - w := mClock.Advance(time.Second) - errs := make(chan error, 1) - go func() { errs <- w.Wait(expired) }() - select { - case err := <-errs: - close(release) - w.MustWait(ctx) - if !errors.Is(err, context.DeadlineExceeded) { - t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) - } - case <-ctx.Done(): - close(release) - t.Fatal("Wait() did not return after the context expired") - } + t.Run("Pending", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + mClock := quartz.NewMock(t) + release := make(chan struct{}) + // The advance cannot complete until the AfterFunc returns. + mClock.AfterFunc(time.Second, func() { <-release }) + w := mClock.Advance(time.Second) + errs := make(chan error, 1) + go func() { errs <- w.Wait(expired) }() + select { + case err := <-errs: + close(release) + w.MustWait(ctx) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) } - }) - } + case <-ctx.Done(): + close(release) + t.Fatal("Wait() did not return after the context expired") + } + }) } // TestTrapWait_ExpiredContext verifies that Wait returns the context error, without blocking, when -// the context has expired, even if the trap is closed. +// the context expires, even if the trap is closed. func TestTrapWait_ExpiredContext(t *testing.T) { t.Parallel() - expired, cancel := context.WithCancel(context.Background()) - cancel() - for _, tt := range []struct { - name string - closed bool - }{ - {name: "NoCall"}, - {name: "TrapClosed", closed: true}, - } { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() + t.Run("TrapClosed", func(t *testing.T) { + t.Parallel() + expired, cancel := context.WithCancel(context.Background()) + cancel() - mClock := quartz.NewMock(t) - trap := mClock.Trap().Now() - if tt.closed { - trap.Close() - } else { - defer trap.Close() + mClock := quartz.NewMock(t) + trap := mClock.Trap().Now() + trap.Close() + // A Wait that picks at random between the closed trap and ctx.Done fails only half the + // iterations, so repeat. + for range 100 { + if _, err := trap.Wait(expired); !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) } - // A Wait that picks at random between the closed trap and ctx.Done fails only half the - // iterations, so repeat. - for range 100 { - errs := make(chan error, 1) - go func() { - _, err := trap.Wait(expired) - errs <- err - }() - select { - case err := <-errs: - if !errors.Is(err, context.Canceled) { - t.Fatalf("Wait() = %v, want %v", err, context.Canceled) - } - case <-ctx.Done(): - t.Fatal("Wait() did not return after the context expired") - } + } + }) + + t.Run("ExpiresWhileWaiting", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + mClock := quartz.NewMock(t) + trap := mClock.Trap().Now() + defer trap.Close() + // The context is live when Wait is called, so Wait blocks until it expires. The result is + // the same if it expires before Wait checks it. + waitCtx, cancelWait := context.WithTimeout(ctx, 10*time.Millisecond) + defer cancelWait() + errs := make(chan error, 1) + go func() { + _, err := trap.Wait(waitCtx) + errs <- err + }() + select { + case err := <-errs: + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) } - }) - } + case <-ctx.Done(): + t.Fatal("Wait() did not return after the context expired") + } + }) } func TestNoLeak_Go123(t *testing.T) { From 4b4c9350d307a33aa76cb77d1a0c4d64d7218fbf Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 12:32:37 +0000 Subject: [PATCH 20/32] fix: say when MustWait failed although the advance completed With an expired context, MustWait fails even when the advance has completed, and the message "context expired while waiting for clock to advance" then points at the wrong cause. It now says the advance completed and asks whether the context had already expired when MustWait was called. The two messages are in separate select branches, so a TestingT whose Fatalf returns sees one failure, not two. The multi-trap Release error now says each trap must release the call from its own goroutine, as the Release godoc requires. The godoc for Wait and MustWait says "the advance" instead of "timers and ticks", and keeps the first line from main. apiCall.traps is now trapCount, since it holds a number. --- mock.go | 35 ++++++++++++++++++++++------------- mock_test.go | 41 +++++++++++++++++++++++++++++++++-------- 2 files changed, 55 insertions(+), 21 deletions(-) diff --git a/mock.go b/mock.go index 8ce5691..e595205 100644 --- a/mock.go +++ b/mock.go @@ -211,7 +211,7 @@ func (m *Mock) matchCallLocked(c *apiCall) { if len(traps) == 0 { return } - c.traps = len(traps) + c.trapCount = len(traps) c.releases.Add(len(traps)) m.mu.Unlock() for _, t := range traps { @@ -233,25 +233,33 @@ type AdvanceWaiter struct { ch chan struct{} } -// Wait for all timers and ticks to complete, or until context expires. If the context has expired, -// Wait returns its error even if the timers and ticks have also completed. To check whether they -// have completed without a deadline, use Done. +// Wait for all timers and ticks to complete, or until context expires. +// If ctx has expired, Wait returns its error even if the advance has completed. To check for +// completion without a deadline, use Done. func (w AdvanceWaiter) Wait(ctx context.Context) error { select { case <-w.ch: case <-ctx.Done(): } // select picks a random ready case. Checking ctx afterwards makes an expired context fail the - // same way whether or not the timers and ticks completed first. + // same way whether or not the advance completed first. return ctx.Err() } // MustWait waits for all timers and ticks to complete, and fails the test immediately if the -// context expires, even if they have also completed. MustWait must be called from the goroutine +// context expires, even if the advance has completed. MustWait must be called from the goroutine // running the test or benchmark, similar to `t.FailNow()`. func (w AdvanceWaiter) MustWait(ctx context.Context) { w.tb.Helper() - if err := w.Wait(ctx); err != nil { + err := w.Wait(ctx) + if err == nil { + return + } + select { + case <-w.ch: + w.tb.Fatalf("context expired, although the clock advance completed; was the context "+ + "already expired when MustWait was called?: %s", err) + default: w.tb.Fatalf("context expired while waiting for clock to advance: %s", err) } } @@ -643,8 +651,8 @@ type apiCall struct { fn clockFunction releases sync.WaitGroup complete chan struct{} - // traps is the number of traps that caught the call. - traps int + // trapCount is the number of traps that caught the call. + trapCount int } func (a *apiCall) String() string { @@ -690,7 +698,8 @@ type Call struct { } // Release the call and wait for it to complete. If the provided context expires before the call completes, it returns -// an error. If the context has already expired, Release still releases the call, and returns the error. +// an error. +// If the context has already expired, Release still releases the call and returns the error. // // IMPORTANT: If a call is trapped by more than one trap, they all must release the call before it can complete, and // they must do so from different goroutines. @@ -715,9 +724,9 @@ func (c *Call) Release(ctx context.Context) error { } func (c *Call) releaseError(err error) error { - if c.apiCall.traps > 1 { - return fmt.Errorf("timed out waiting for the call to complete; %d traps caught it and each "+ - "must release it: %w", c.apiCall.traps, err) + if c.apiCall.trapCount > 1 { + return fmt.Errorf("timed out waiting for the call to complete; %d traps caught it, "+ + "and each must release it from its own goroutine: %w", c.apiCall.trapCount, err) } return fmt.Errorf("timed out waiting for the call to complete after release: %w", err) } diff --git a/mock_test.go b/mock_test.go index 39c1a41..4293319 100644 --- a/mock_test.go +++ b/mock_test.go @@ -557,31 +557,38 @@ func Test_WithLogger(t *testing.T) { type captureFailTB struct { failed bool + // messages holds the failure messages, in order. + messages []string testing.TB } func (t *captureFailTB) Errorf(format string, args ...any) { t.Helper() t.Logf(format, args...) - t.failed = true + t.fail(fmt.Sprintf(format, args...)) } func (t *captureFailTB) Error(args ...any) { t.Helper() t.Log(args...) - t.failed = true + t.fail(fmt.Sprint(args...)) } func (t *captureFailTB) Fatal(args ...any) { t.Helper() t.Log(args...) - t.failed = true + t.fail(fmt.Sprint(args...)) } func (t *captureFailTB) Fatalf(format string, args ...any) { t.Helper() t.Logf(format, args...) + t.fail(fmt.Sprintf(format, args...)) +} + +func (t *captureFailTB) fail(msg string) { t.failed = true + t.messages = append(t.messages, msg) } func (t *captureFailTB) Fail() { @@ -709,7 +716,8 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { t.Run("Completed", func(t *testing.T) { t.Parallel() - mClock := quartz.NewMock(t) + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) // A Wait that picks at random between completion and ctx.Done fails only half the // iterations, so repeat. for range 100 { @@ -719,6 +727,12 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) } } + + mClock.Advance(time.Second).MustWait(expired) + const want = "although the clock advance completed" + if len(tb.messages) != 1 || !strings.Contains(tb.messages[0], want) { + t.Fatalf("MustWait() failures = %q, want one containing %q", tb.messages, want) + } }) t.Run("Pending", func(t *testing.T) { @@ -726,23 +740,34 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - mClock := quartz.NewMock(t) + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) release := make(chan struct{}) // The advance cannot complete until the AfterFunc returns. mClock.AfterFunc(time.Second, func() { <-release }) w := mClock.Advance(time.Second) errs := make(chan error, 1) - go func() { errs <- w.Wait(expired) }() + go func() { + err := w.Wait(expired) + w.MustWait(expired) + errs <- err + }() select { case err := <-errs: close(release) - w.MustWait(ctx) if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) } case <-ctx.Done(): close(release) - t.Fatal("Wait() did not return after the context expired") + t.Fatal("Wait() or MustWait() did not return after the context expired") + } + const want = "while waiting for clock to advance" + if len(tb.messages) != 1 || !strings.Contains(tb.messages[0], want) { + t.Fatalf("MustWait() failures = %q, want one containing %q", tb.messages, want) + } + if err := w.Wait(ctx); err != nil { + t.Fatal(err) } }) } From ba5a3f9eab7f0d11dd0dca0a14d6b8efa75c7f3d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 15:30:28 +0000 Subject: [PATCH 21/32] fix: say when Release was called with an expired context Release with an already-expired context returns without waiting, but its error said "timed out waiting for the call to complete" and, with two traps, told the user every trap must release the call. Neither is the cause: the call was released and may well complete. That branch now says the context had already expired when Release was called and that the call was released. The timeout message stays for a context that expires while Release waits. TwoTrapsWhileWaiting used a 10ms timeout, which now picks the message by timing: a slow run would see the already-expired one. It uses a test context that expires when Release first asks for Done, after the entry check, so the row no longer depends on the clock. --- mock.go | 4 +++- mock_test.go | 52 +++++++++++++++++++++++++++++++++++----------------- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/mock.go b/mock.go index e595205..9b386ad 100644 --- a/mock.go +++ b/mock.go @@ -713,7 +713,8 @@ func (c *Call) Release(ctx context.Context) error { c.trap.callReleased() c.apiCall.releases.Done() if expired != nil { - return c.releaseError(expired) + return fmt.Errorf("context already expired when Release was called; the call was "+ + "released: %w", expired) } select { case <-ctx.Done(): @@ -723,6 +724,7 @@ func (c *Call) Release(ctx context.Context) error { } } +// releaseError reports a context that expired while Release waited for the call to complete. func (c *Call) releaseError(err error) error { if c.apiCall.trapCount > 1 { return fmt.Errorf("timed out waiting for the call to complete; %d traps caught it, "+ diff --git a/mock_test.go b/mock_test.go index 4293319..9f63f20 100644 --- a/mock_test.go +++ b/mock_test.go @@ -374,8 +374,9 @@ func Test_MultipleTraps(t *testing.T) { } // TestCallRelease_ExpiredContext verifies that Release returns the context error when the context -// expires before the call completes, still releases the call so that it returns, and names the -// trap count only when more than one trap caught the call. Close must not report the call. +// expires before the call completes, still releases the call so that it returns, and mentions other +// traps only when they kept the call from completing while Release waited. Close must not report +// the call. func TestCallRelease_ExpiredContext(t *testing.T) { t.Parallel() expired, cancel := context.WithCancel(context.Background()) @@ -384,15 +385,15 @@ func TestCallRelease_ExpiredContext(t *testing.T) { for _, tt := range []struct { name string traps int - // whileWaiting gives Release a context that expires 10ms after Release is called, while - // Release waits for the other traps. The result is the same if it expires sooner. + // whileWaiting gives Release a context that expires once Release waits for the other + // traps, instead of one that has already expired. whileWaiting bool - // hint must be in the error; if empty, the error must not mention traps. - hint string + // want must be in the error. + want string }{ - {name: "OneTrap", traps: 1}, - {name: "TwoTraps", traps: 2, hint: "2 traps"}, - {name: "TwoTrapsWhileWaiting", traps: 2, whileWaiting: true, hint: "2 traps"}, + {name: "OneTrap", traps: 1, want: "already expired when Release was called"}, + {name: "TwoTraps", traps: 2, want: "already expired when Release was called"}, + {name: "TwoTrapsWhileWaiting", traps: 2, whileWaiting: true, want: "2 traps"}, } { t.Run(tt.name, func(t *testing.T) { t.Parallel() @@ -417,11 +418,9 @@ func TestCallRelease_ExpiredContext(t *testing.T) { if err != nil { t.Fatal(err) } - releaseCtx := expired + var releaseCtx context.Context = expired if tt.whileWaiting { - var cancelRelease context.CancelFunc - releaseCtx, cancelRelease = context.WithTimeout(ctx, 10*time.Millisecond) - defer cancelRelease() + releaseCtx = newExpiresOnWaitCtx(t) } errs := make(chan error, 1) go func() { errs <- c.Release(releaseCtx) }() @@ -433,11 +432,11 @@ func TestCallRelease_ExpiredContext(t *testing.T) { if releaseCtx.Err() == nil || !errors.Is(err, releaseCtx.Err()) { t.Fatalf("Release() = %v, want %v", err, releaseCtx.Err()) } - if tt.hint == "" && strings.Contains(err.Error(), "trap") { - t.Fatalf("Release() = %q, want no mention of other traps", err) + if !strings.Contains(err.Error(), tt.want) { + t.Fatalf("Release() = %q, want it to contain %q", err, tt.want) } - if !strings.Contains(err.Error(), tt.hint) { - t.Fatalf("Release() = %q, want it to contain %q", err, tt.hint) + if !tt.whileWaiting && strings.Contains(err.Error(), "trap") { + t.Fatalf("Release() = %q, want no mention of other traps", err) } for _, trap := range traps[1:] { @@ -611,6 +610,25 @@ func tRunFail(t testing.TB, f func(t testing.TB)) { } } +// expiresOnWaitCtx is a context that is live until its Done channel is first requested, and then +// expires. Release and Trap.Wait check ctx.Err() on entry and call Done only to wait, so it stands +// in for a context that expires while they wait, without depending on timing. +type expiresOnWaitCtx struct { + context.Context + cancel context.CancelFunc +} + +func newExpiresOnWaitCtx(t testing.TB) expiresOnWaitCtx { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + return expiresOnWaitCtx{Context: ctx, cancel: cancel} +} + +func (c expiresOnWaitCtx) Done() <-chan struct{} { + c.cancel() + return c.Context.Done() +} + type testLogger struct { calls []string } From 5de3f3a714633db9c3636e30e09b1c2cbf4669f3 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 15:31:36 +0000 Subject: [PATCH 22/32] fix: decide MustWait's message from the context on entry MustWait chose its "already expired when MustWait was called" hint by checking whether the advance had completed after Wait failed. That was wrong both ways: a context that expired during the wait just before the advance completed got the hint, and a context that had already expired when the advance was still pending did not. MustWait now checks ctx.Err() before it waits and gives the hint exactly then. Completed and Pending each call MustWait twice: with an already-expired context, which must give the hint, and with a context that expires once MustWait waits, which must not. --- mock.go | 12 +++++------- mock_test.go | 32 ++++++++++++++++++++++---------- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/mock.go b/mock.go index 9b386ad..c618b08 100644 --- a/mock.go +++ b/mock.go @@ -251,15 +251,13 @@ func (w AdvanceWaiter) Wait(ctx context.Context) error { // running the test or benchmark, similar to `t.FailNow()`. func (w AdvanceWaiter) MustWait(ctx context.Context) { w.tb.Helper() - err := w.Wait(ctx) - if err == nil { + // Decide from ctx on entry, not from whether the advance completed: it can complete just after + // ctx expires during the wait, or still be pending when ctx had expired before the call. + if err := ctx.Err(); err != nil { + w.tb.Fatalf("context already expired when MustWait was called: %s", err) return } - select { - case <-w.ch: - w.tb.Fatalf("context expired, although the clock advance completed; was the context "+ - "already expired when MustWait was called?: %s", err) - default: + if err := w.Wait(ctx); err != nil { w.tb.Fatalf("context expired while waiting for clock to advance: %s", err) } } diff --git a/mock_test.go b/mock_test.go index 9f63f20..b5719d2 100644 --- a/mock_test.go +++ b/mock_test.go @@ -611,8 +611,8 @@ func tRunFail(t testing.TB, f func(t testing.TB)) { } // expiresOnWaitCtx is a context that is live until its Done channel is first requested, and then -// expires. Release and Trap.Wait check ctx.Err() on entry and call Done only to wait, so it stands -// in for a context that expires while they wait, without depending on timing. +// expires. Release, Trap.Wait and MustWait check ctx.Err() on entry and call Done only to wait, so +// it stands in for a context that expires while they wait, without depending on timing. type expiresOnWaitCtx struct { context.Context cancel context.CancelFunc @@ -629,6 +629,19 @@ func (c expiresOnWaitCtx) Done() <-chan struct{} { return c.Context.Done() } +// wantMessages fails the test unless got has one message per want, each containing its want. +func wantMessages(t testing.TB, got []string, want ...string) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("failures = %q, want %d containing %q", got, len(want), want) + } + for i := range want { + if !strings.Contains(got[i], want[i]) { + t.Fatalf("failure %d = %q, want it to contain %q", i, got[i], want[i]) + } + } +} + type testLogger struct { calls []string } @@ -747,10 +760,9 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { } mClock.Advance(time.Second).MustWait(expired) - const want = "although the clock advance completed" - if len(tb.messages) != 1 || !strings.Contains(tb.messages[0], want) { - t.Fatalf("MustWait() failures = %q, want one containing %q", tb.messages, want) - } + mClock.Advance(time.Second).MustWait(newExpiresOnWaitCtx(t)) + wantMessages(t, tb.messages, "already expired when MustWait was called", + "while waiting for clock to advance") }) t.Run("Pending", func(t *testing.T) { @@ -765,9 +777,11 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { mClock.AfterFunc(time.Second, func() { <-release }) w := mClock.Advance(time.Second) errs := make(chan error, 1) + expiresOnWait := newExpiresOnWaitCtx(t) go func() { err := w.Wait(expired) w.MustWait(expired) + w.MustWait(expiresOnWait) errs <- err }() select { @@ -780,10 +794,8 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { close(release) t.Fatal("Wait() or MustWait() did not return after the context expired") } - const want = "while waiting for clock to advance" - if len(tb.messages) != 1 || !strings.Contains(tb.messages[0], want) { - t.Fatalf("MustWait() failures = %q, want one containing %q", tb.messages, want) - } + wantMessages(t, tb.messages, "already expired when MustWait was called", + "while waiting for clock to advance") if err := w.Wait(ctx); err != nil { t.Fatal(err) } From 340fc3380ca9239f60077cd4bcfc47ca9937ed94 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 15:32:08 +0000 Subject: [PATCH 23/32] test(mock_test.go): expire TrapWait's context on wait, not after 10ms ExpiresWhileWaiting is there to reach the ctx.Done case of Trap.Wait's select. With a 10ms timeout it did so only when Wait checked the context within 10ms; on a slow run the entry check returned first and the row covered nothing new. The expiresOnWaitCtx fixture is live at the entry check and expires once Wait waits, so the row reaches the select every run and no longer waits 10ms. --- mock_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/mock_test.go b/mock_test.go index b5719d2..c3ed22f 100644 --- a/mock_test.go +++ b/mock_test.go @@ -832,10 +832,8 @@ func TestTrapWait_ExpiredContext(t *testing.T) { mClock := quartz.NewMock(t) trap := mClock.Trap().Now() defer trap.Close() - // The context is live when Wait is called, so Wait blocks until it expires. The result is - // the same if it expires before Wait checks it. - waitCtx, cancelWait := context.WithTimeout(ctx, 10*time.Millisecond) - defer cancelWait() + // The context is live when Wait checks it on entry and expires once Wait waits. + waitCtx := newExpiresOnWaitCtx(t) errs := make(chan error, 1) go func() { _, err := trap.Wait(waitCtx) @@ -843,8 +841,8 @@ func TestTrapWait_ExpiredContext(t *testing.T) { }() select { case err := <-errs: - if !errors.Is(err, context.DeadlineExceeded) { - t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) + if !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) } case <-ctx.Done(): t.Fatal("Wait() did not return after the context expired") From 3543e67676e218323d2f396267e3b7660706bbed Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:25:39 +0000 Subject: [PATCH 24/32] fix: say that this trap released the call when Release's context expired With more than one trap, "the call was released" could read as "the call can now return", but it returns only after every trap releases it. The message now says this trap released the call. --- mock.go | 4 ++-- mock_test.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/mock.go b/mock.go index c618b08..91e52ca 100644 --- a/mock.go +++ b/mock.go @@ -711,8 +711,8 @@ func (c *Call) Release(ctx context.Context) error { c.trap.callReleased() c.apiCall.releases.Done() if expired != nil { - return fmt.Errorf("context already expired when Release was called; the call was "+ - "released: %w", expired) + return fmt.Errorf("context already expired when Release was called; this trap released "+ + "the call: %w", expired) } select { case <-ctx.Done(): diff --git a/mock_test.go b/mock_test.go index c3ed22f..cdeb4d0 100644 --- a/mock_test.go +++ b/mock_test.go @@ -391,8 +391,8 @@ func TestCallRelease_ExpiredContext(t *testing.T) { // want must be in the error. want string }{ - {name: "OneTrap", traps: 1, want: "already expired when Release was called"}, - {name: "TwoTraps", traps: 2, want: "already expired when Release was called"}, + {name: "OneTrap", traps: 1, want: "already expired when Release was called; this trap released"}, + {name: "TwoTraps", traps: 2, want: "already expired when Release was called; this trap released"}, {name: "TwoTrapsWhileWaiting", traps: 2, whileWaiting: true, want: "2 traps"}, } { t.Run(tt.name, func(t *testing.T) { @@ -435,7 +435,7 @@ func TestCallRelease_ExpiredContext(t *testing.T) { if !strings.Contains(err.Error(), tt.want) { t.Fatalf("Release() = %q, want it to contain %q", err, tt.want) } - if !tt.whileWaiting && strings.Contains(err.Error(), "trap") { + if !tt.whileWaiting && strings.Contains(err.Error(), "traps caught it") { t.Fatalf("Release() = %q, want no mention of other traps", err) } From 75b2c98928ab63fdd26e526b003c07da2c184058 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:26:49 +0000 Subject: [PATCH 25/32] test(mock_test.go): reach Release's one-trap timeout message Once Release returned early on an already-expired context, no row reached the one-trap "after release" error: a one-trap Now call completes as soon as it is released. The table now traps the TickerFunc's Wait, which does not return until the ticker stops, so a one-trap call stays pending while Release waits. OneTrapWhileWaiting uses that, and each row checks that only the two-trap timeout mentions other traps. --- mock_test.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/mock_test.go b/mock_test.go index cdeb4d0..a9e2849 100644 --- a/mock_test.go +++ b/mock_test.go @@ -385,15 +385,16 @@ func TestCallRelease_ExpiredContext(t *testing.T) { for _, tt := range []struct { name string traps int - // whileWaiting gives Release a context that expires once Release waits for the other - // traps, instead of one that has already expired. + // whileWaiting gives Release a context that expires once Release waits for the call to + // complete, instead of one that has already expired. whileWaiting bool // want must be in the error. want string }{ {name: "OneTrap", traps: 1, want: "already expired when Release was called; this trap released"}, {name: "TwoTraps", traps: 2, want: "already expired when Release was called; this trap released"}, - {name: "TwoTrapsWhileWaiting", traps: 2, whileWaiting: true, want: "2 traps"}, + {name: "OneTrapWhileWaiting", traps: 1, whileWaiting: true, want: "complete after release"}, + {name: "TwoTrapsWhileWaiting", traps: 2, whileWaiting: true, want: "2 traps caught it"}, } { t.Run(tt.name, func(t *testing.T) { t.Parallel() @@ -402,16 +403,21 @@ func TestCallRelease_ExpiredContext(t *testing.T) { tb := &captureFailTB{TB: t} mClock := quartz.NewMock(tb) + // The trapped call is the ticker's Wait, which does not return until the ticker stops, + // so the call cannot complete while Release waits, even with one trap. + tickerCtx, stopTicker := context.WithCancel(ctx) + defer stopTicker() + tkr := mClock.TickerFunc(tickerCtx, time.Hour, func() error { return nil }) var tags []string var traps []*quartz.Trap for i := range tt.traps { tags = append(tags, strconv.Itoa(i)) - traps = append(traps, mClock.Trap().Now(tags[i])) + traps = append(traps, mClock.Trap().TickerFuncWait(tags[i])) } returned := make(chan struct{}) go func() { defer close(returned) - mClock.Now(tags...) + _ = tkr.Wait(tags...) }() c, err := traps[0].Wait(ctx) @@ -435,10 +441,12 @@ func TestCallRelease_ExpiredContext(t *testing.T) { if !strings.Contains(err.Error(), tt.want) { t.Fatalf("Release() = %q, want it to contain %q", err, tt.want) } - if !tt.whileWaiting && strings.Contains(err.Error(), "traps caught it") { - t.Fatalf("Release() = %q, want no mention of other traps", err) + blamesTraps := tt.whileWaiting && tt.traps > 1 + if strings.Contains(err.Error(), "traps caught it") != blamesTraps { + t.Fatalf("Release() = %q, want a mention of other traps: %v", err, blamesTraps) } + stopTicker() for _, trap := range traps[1:] { c, err := trap.Wait(ctx) if err != nil { From db785428a8f144aa0f4bc3bd9476611133bfd566 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:27:24 +0000 Subject: [PATCH 26/32] fix: prefix MustRelease failures with MustRelease and the call MustRelease failed the test with Release's error as is, so the message named Release, which the test never called, and did not say which call failed. It now reads "MustRelease of : ", like Trap.MustWait names its trap. --- mock.go | 2 +- mock_test.go | 29 ++++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/mock.go b/mock.go index 91e52ca..9459d0b 100644 --- a/mock.go +++ b/mock.go @@ -739,7 +739,7 @@ func (c *Call) releaseError(err error) error { func (c *Call) MustRelease(ctx context.Context) { if err := c.Release(ctx); err != nil { c.tb.Helper() - c.tb.Fatal(err.Error()) + c.tb.Fatalf("MustRelease of %s: %s", c.apiCall, err) } } diff --git a/mock_test.go b/mock_test.go index a9e2849..daee145 100644 --- a/mock_test.go +++ b/mock_test.go @@ -376,7 +376,7 @@ func Test_MultipleTraps(t *testing.T) { // TestCallRelease_ExpiredContext verifies that Release returns the context error when the context // expires before the call completes, still releases the call so that it returns, and mentions other // traps only when they kept the call from completing while Release waited. Close must not report -// the call. +// the call, and a failed MustRelease must name MustRelease and the call. func TestCallRelease_ExpiredContext(t *testing.T) { t.Parallel() expired, cancel := context.WithCancel(context.Background()) @@ -469,6 +469,33 @@ func TestCallRelease_ExpiredContext(t *testing.T) { } }) } + + t.Run("MustRelease", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) + trap := mClock.Trap().Now() + defer trap.Close() + returned := make(chan struct{}) + go func() { + defer close(returned) + mClock.Now() + }() + c, err := trap.Wait(ctx) + if err != nil { + t.Fatal(err) + } + c.MustRelease(expired) + select { + case <-returned: + case <-ctx.Done(): + t.Fatal("the call did not return after MustRelease") + } + wantMessages(t, tb.messages, "MustRelease of Now([]): context already expired") + }) } // Test_UnreleasedCalls verifies that Close fails the test only when a trapped call has not been From c488df2d80edc03a95e19afa5e6a349c1a01f0b8 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:27:33 +0000 Subject: [PATCH 27/32] test(mock_test.go): describe the MustWait checks in the test's doc TestAdvanceWaiter_ExpiredContext has also checked MustWait's messages since MustWait started picking them from the context on entry, but its doc comment described only Wait. --- mock_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mock_test.go b/mock_test.go index daee145..8a0c963 100644 --- a/mock_test.go +++ b/mock_test.go @@ -773,7 +773,9 @@ func TestTimerReset_NegativeDuration(t *testing.T) { } // TestAdvanceWaiter_ExpiredContext verifies that Wait returns the context error, without blocking, -// when the context has expired, whether or not the advance has completed. +// when the context has expired, whether or not the advance has completed. MustWait must say the +// context had already expired exactly when it had expired before MustWait was called, and +// otherwise that it expired while waiting. func TestAdvanceWaiter_ExpiredContext(t *testing.T) { t.Parallel() // The deadline passed before Wait, as when the test's timeout ran out first. From 9f0d1866273727e6a9a23ce54069d597b7913eaf Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:53:37 +0000 Subject: [PATCH 28/32] fix: describe the call before MustRelease releases it MustRelease formatted the call for its failure message after Release had let the code under test return. The call's Tags share the caller's variadic slice, so a caller that reused the slice raced with the formatting: -race reported it in 10 of 10 runs, and without -race the message showed the caller's later value in 32 of 2000 runs. --- mock.go | 7 +++++-- mock_test.go | 8 ++++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/mock.go b/mock.go index 9459d0b..b9c1f5f 100644 --- a/mock.go +++ b/mock.go @@ -737,9 +737,12 @@ func (c *Call) releaseError(err error) error { // IMPORTANT: If a call is trapped by more than one trap, they all must release the call before it can complete, and // they must do so from different goroutines. func (c *Call) MustRelease(ctx context.Context) { + c.tb.Helper() + // Describe the call before releasing it: once released, the code under test may return and + // reuse the tags slice that the call refers to. + call := c.apiCall.String() if err := c.Release(ctx); err != nil { - c.tb.Helper() - c.tb.Fatalf("MustRelease of %s: %s", c.apiCall, err) + c.tb.Fatalf("MustRelease of %s: %s", call, err) } } diff --git a/mock_test.go b/mock_test.go index 8a0c963..1cb5dcf 100644 --- a/mock_test.go +++ b/mock_test.go @@ -482,7 +482,11 @@ func TestCallRelease_ExpiredContext(t *testing.T) { returned := make(chan struct{}) go func() { defer close(returned) - mClock.Now() + // Callers may reuse a variadic slice once the call returns, so MustRelease must + // describe the call before it releases it. + tags := []string{"before"} + mClock.Now(tags...) + tags[0] = "after" }() c, err := trap.Wait(ctx) if err != nil { @@ -494,7 +498,7 @@ func TestCallRelease_ExpiredContext(t *testing.T) { case <-ctx.Done(): t.Fatal("the call did not return after MustRelease") } - wantMessages(t, tb.messages, "MustRelease of Now([]): context already expired") + wantMessages(t, tb.messages, "MustRelease of Now([before]): context already expired") }) } From 5dc1db97a001f3c3ec8a80fb95a271c7a1877f2a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 16:53:52 +0000 Subject: [PATCH 29/32] fix: stop naming Release in its expired-context error MustRelease prefixes Release's error, so "context already expired when Release was called" told MustRelease callers about a method they did not call. The error now says only that the context had already expired and that this trap released the call, which reads correctly both alone and as "MustRelease of : ...". MustWait still names itself because its message has no prefix. --- mock.go | 3 +-- mock_test.go | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/mock.go b/mock.go index b9c1f5f..7b16625 100644 --- a/mock.go +++ b/mock.go @@ -711,8 +711,7 @@ func (c *Call) Release(ctx context.Context) error { c.trap.callReleased() c.apiCall.releases.Done() if expired != nil { - return fmt.Errorf("context already expired when Release was called; this trap released "+ - "the call: %w", expired) + return fmt.Errorf("context already expired; this trap released the call: %w", expired) } select { case <-ctx.Done(): diff --git a/mock_test.go b/mock_test.go index 1cb5dcf..78ada62 100644 --- a/mock_test.go +++ b/mock_test.go @@ -391,8 +391,8 @@ func TestCallRelease_ExpiredContext(t *testing.T) { // want must be in the error. want string }{ - {name: "OneTrap", traps: 1, want: "already expired when Release was called; this trap released"}, - {name: "TwoTraps", traps: 2, want: "already expired when Release was called; this trap released"}, + {name: "OneTrap", traps: 1, want: "context already expired; this trap released"}, + {name: "TwoTraps", traps: 2, want: "context already expired; this trap released"}, {name: "OneTrapWhileWaiting", traps: 1, whileWaiting: true, want: "complete after release"}, {name: "TwoTrapsWhileWaiting", traps: 2, whileWaiting: true, want: "2 traps caught it"}, } { @@ -498,7 +498,7 @@ func TestCallRelease_ExpiredContext(t *testing.T) { case <-ctx.Done(): t.Fatal("the call did not return after MustRelease") } - wantMessages(t, tb.messages, "MustRelease of Now([before]): context already expired") + wantMessages(t, tb.messages, "MustRelease of Now([before]): context already expired; this trap") }) } From d60388b0849f060876960c4ec2a3cf77c4c7df21 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 17:20:11 +0000 Subject: [PATCH 30/32] docs(mock.go): tighten the expired-context godoc and comments MustRelease's godoc did not say it also fails on a context that had already expired, or that the call is released either way. Release's godoc ran past 100 columns and added its expired-context sentence as a separate line. AdvanceWaiter.Wait's doc switched between "ctx" and "the context". The MustWait comment argued against an earlier design instead of saying why the entry check exists, and the trapCount comment did not say why reading it without a lock is safe. --- mock.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/mock.go b/mock.go index 7b16625..2efef3a 100644 --- a/mock.go +++ b/mock.go @@ -233,9 +233,9 @@ type AdvanceWaiter struct { ch chan struct{} } -// Wait for all timers and ticks to complete, or until context expires. -// If ctx has expired, Wait returns its error even if the advance has completed. To check for -// completion without a deadline, use Done. +// Wait for all timers and ticks to complete, or until the context expires. +// If the context has expired, Wait returns its error even if the advance has completed. To check +// for completion without a deadline, use Done. func (w AdvanceWaiter) Wait(ctx context.Context) error { select { case <-w.ch: @@ -251,8 +251,8 @@ func (w AdvanceWaiter) Wait(ctx context.Context) error { // running the test or benchmark, similar to `t.FailNow()`. func (w AdvanceWaiter) MustWait(ctx context.Context) { w.tb.Helper() - // Decide from ctx on entry, not from whether the advance completed: it can complete just after - // ctx expires during the wait, or still be pending when ctx had expired before the call. + // A context that had already expired is a mistake in the test, not a slow advance, so it gets + // its own message, decided before waiting. if err := ctx.Err(); err != nil { w.tb.Fatalf("context already expired when MustWait was called: %s", err) return @@ -649,7 +649,8 @@ type apiCall struct { fn clockFunction releases sync.WaitGroup complete chan struct{} - // trapCount is the number of traps that caught the call. + // trapCount is the number of traps that caught the call. matchCallLocked sets it before it + // starts the catch goroutines, and it is read-only after that. trapCount int } @@ -695,9 +696,9 @@ type Call struct { trap *Trap } -// Release the call and wait for it to complete. If the provided context expires before the call completes, it returns -// an error. -// If the context has already expired, Release still releases the call and returns the error. +// Release the call and wait for it to complete. If the provided context expires before the call +// completes, it returns an error. If the context has already expired, Release releases the call +// and returns the context's error without waiting. // // IMPORTANT: If a call is trapped by more than one trap, they all must release the call before it can complete, and // they must do so from different goroutines. @@ -730,8 +731,8 @@ func (c *Call) releaseError(err error) error { return fmt.Errorf("timed out waiting for the call to complete after release: %w", err) } -// MustRelease releases the call and waits for it to complete. If the provided context expires before the call -// completes, it fails the test. +// MustRelease releases the call and waits for it to complete. It fails the test if the context +// expires before the call completes or had already expired; the call is released either way. // // IMPORTANT: If a call is trapped by more than one trap, they all must release the call before it can complete, and // they must do so from different goroutines. From 359d7c236d4c6e65f483034d6a84c47a9c89d12d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 17:20:49 +0000 Subject: [PATCH 31/32] fix: say when Trap.MustWait was called with an expired context Trap.MustWait with a context that had already expired failed with "context expired while waiting for ", which points at a slow call instead of the test's context. Like AdvanceWaiter.MustWait, it now checks the context first and says it had already expired. The closed-trap message is unchanged. --- mock.go | 6 ++++++ mock_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/mock.go b/mock.go index 2efef3a..8e182bf 100644 --- a/mock.go +++ b/mock.go @@ -874,6 +874,12 @@ func (t *Trap) Wait(ctx context.Context) (*Call, error) { // test via tb.Fatalf() func (t *Trap) MustWait(ctx context.Context) *Call { t.mock.tb.Helper() + // As in AdvanceWaiter.MustWait, a context that had already expired is a mistake in the test, + // so it gets its own message, decided before waiting. + if err := ctx.Err(); err != nil { + t.mock.tb.Fatalf("context already expired when MustWait was called for %s: %s", t, err) + return nil + } c, err := t.Wait(ctx) if err != nil { t.mock.tb.Fatalf("context expired while waiting for %s: %s", t, err.Error()) diff --git a/mock_test.go b/mock_test.go index 78ada62..31e99fa 100644 --- a/mock_test.go +++ b/mock_test.go @@ -10,6 +10,7 @@ import ( "strconv" "strings" "testing" + "testing/synctest" "time" "github.com/coder/quartz" @@ -848,6 +849,32 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { func TestTrapWait_ExpiredContext(t *testing.T) { t.Parallel() + t.Run("MustWait", func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) + trap := mClock.Trap().Now() + defer trap.Close() + + expired, cancel := context.WithCancel(t.Context()) + cancel() + trap.MustWait(expired) + + ctx, cancel := context.WithCancel(t.Context()) + done := make(chan struct{}) + go func() { + defer close(done) + trap.MustWait(ctx) + }() + synctest.Wait() // MustWait is blocked waiting for a call. + cancel() + <-done + wantMessages(t, tb.messages, "context already expired when MustWait was called for Trap Now(", + "context expired while waiting for Trap Now(") + }) + }) + t.Run("TrapClosed", func(t *testing.T) { t.Parallel() expired, cancel := context.WithCancel(context.Background()) From b989a40faffec751416770bed473715070128534 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 4 Nov 2026 17:25:20 +0000 Subject: [PATCH 32/32] test: use synctest instead of a context that expires on Done expiresOnWaitCtx expired the first time a method asked for Done, so the tests passed only while the methods read Err before Done. Rewriting an entry check as a select on Done, which changes no behavior, failed them. The tests now start the wait on a goroutine, call synctest.Wait until it blocks, then cancel the context. That works whatever order the methods read the context in. The Trap.Wait ready-call test moves to the public API as TestTrapWait_ExpiredContext/CallReady: synctest.Wait returns once the trap's catch goroutine is blocked handing over the call, so mock_internal_test.go goes away. The Completed MustWait check for a context that expires during the wait is dropped: the advance has already completed, so no wait happens and nothing can expire during it. ReleasedOnAnotherGoroutine now fails after 10s instead of hanging if the call or Release never returns. --- mock_internal_test.go | 45 ------- mock_test.go | 290 ++++++++++++++++++++++-------------------- 2 files changed, 149 insertions(+), 186 deletions(-) delete mode 100644 mock_internal_test.go diff --git a/mock_internal_test.go b/mock_internal_test.go deleted file mode 100644 index 54cd203..0000000 --- a/mock_internal_test.go +++ /dev/null @@ -1,45 +0,0 @@ -package quartz - -import ( - "context" - "errors" - "testing" - "time" -) - -// TestTrapWait_CallReadyWithExpiredContext verifies that Wait returns the context error when the -// context has expired, even if a call is ready, and leaves the call for the next Wait. -// -// The public API cannot make a call ready before Wait checks the context, because catch delivers -// each call from its own goroutine, so this test builds that state directly. -func TestTrapWait_CallReadyWithExpiredContext(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - expired, cancelExpired := context.WithCancel(context.Background()) - cancelExpired() - - // A buffered channel holding the call stands in for a catch goroutine blocked sending it. - trap := &Trap{ - fn: clockFunctionNow, - mock: NewMock(t), - calls: make(chan *apiCall, 1), - done: make(chan struct{}), - } - want := newCall(clockFunctionNow, nil) - // A Wait that picks at random between the ready call and ctx.Done fails only half the - // iterations, so repeat. - for range 100 { - trap.calls <- want - if _, err := trap.Wait(expired); !errors.Is(err, context.Canceled) { - t.Fatalf("Wait() error = %v, want %v", err, context.Canceled) - } - c, err := trap.Wait(ctx) - if err != nil { - t.Fatalf("second Wait() error = %v, want nil", err) - } - if c.apiCall != want { - t.Fatal("second Wait() returned a different call than the ready one") - } - } -} diff --git a/mock_test.go b/mock_test.go index 31e99fa..602c82c 100644 --- a/mock_test.go +++ b/mock_test.go @@ -386,8 +386,8 @@ func TestCallRelease_ExpiredContext(t *testing.T) { for _, tt := range []struct { name string traps int - // whileWaiting gives Release a context that expires once Release waits for the call to - // complete, instead of one that has already expired. + // whileWaiting expires the context once Release is blocked waiting for the call to + // complete, instead of before Release is called. whileWaiting bool // want must be in the error. want string @@ -399,75 +399,71 @@ func TestCallRelease_ExpiredContext(t *testing.T) { } { t.Run(tt.name, func(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - tb := &captureFailTB{TB: t} - mClock := quartz.NewMock(tb) - // The trapped call is the ticker's Wait, which does not return until the ticker stops, - // so the call cannot complete while Release waits, even with one trap. - tickerCtx, stopTicker := context.WithCancel(ctx) - defer stopTicker() - tkr := mClock.TickerFunc(tickerCtx, time.Hour, func() error { return nil }) - var tags []string - var traps []*quartz.Trap - for i := range tt.traps { - tags = append(tags, strconv.Itoa(i)) - traps = append(traps, mClock.Trap().TickerFuncWait(tags[i])) - } - returned := make(chan struct{}) - go func() { - defer close(returned) - _ = tkr.Wait(tags...) - }() - - c, err := traps[0].Wait(ctx) - if err != nil { - t.Fatal(err) - } - var releaseCtx context.Context = expired - if tt.whileWaiting { - releaseCtx = newExpiresOnWaitCtx(t) - } - errs := make(chan error, 1) - go func() { errs <- c.Release(releaseCtx) }() - select { - case err = <-errs: - case <-ctx.Done(): - t.Fatal("Release() did not return after its context expired") - } - if releaseCtx.Err() == nil || !errors.Is(err, releaseCtx.Err()) { - t.Fatalf("Release() = %v, want %v", err, releaseCtx.Err()) - } - if !strings.Contains(err.Error(), tt.want) { - t.Fatalf("Release() = %q, want it to contain %q", err, tt.want) - } - blamesTraps := tt.whileWaiting && tt.traps > 1 - if strings.Contains(err.Error(), "traps caught it") != blamesTraps { - t.Fatalf("Release() = %q, want a mention of other traps: %v", err, blamesTraps) - } + synctest.Test(t, func(t *testing.T) { + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) + // The trapped call is the ticker's Wait, which does not return until the ticker + // stops, so the call cannot complete while Release waits, even with one trap. + tickerCtx, stopTicker := context.WithCancel(t.Context()) + defer stopTicker() + tkr := mClock.TickerFunc(tickerCtx, time.Hour, func() error { return nil }) + var tags []string + var traps []*quartz.Trap + for i := range tt.traps { + tags = append(tags, strconv.Itoa(i)) + traps = append(traps, mClock.Trap().TickerFuncWait(tags[i])) + } + returned := make(chan struct{}) + go func() { + defer close(returned) + _ = tkr.Wait(tags...) + }() - stopTicker() - for _, trap := range traps[1:] { - c, err := trap.Wait(ctx) + c, err := traps[0].Wait(t.Context()) if err != nil { t.Fatal(err) } - if err := c.Release(ctx); err != nil { - t.Fatal(err) + releaseCtx, expire := context.WithCancel(t.Context()) + defer expire() + if !tt.whileWaiting { + expire() } - } - select { - case <-returned: - case <-ctx.Done(): - t.Fatal("the call did not return after every trap released it") - } - for _, trap := range traps { - trap.Close() - } - if tb.failed { - t.Fatal("Close() reported a failure after every trap released the call") - } + errs := make(chan error, 1) + go func() { errs <- c.Release(releaseCtx) }() + if tt.whileWaiting { + synctest.Wait() // Release is blocked waiting for the call to complete. + expire() + } + err = <-errs + if !errors.Is(err, context.Canceled) { + t.Fatalf("Release() = %v, want %v", err, context.Canceled) + } + if !strings.Contains(err.Error(), tt.want) { + t.Fatalf("Release() = %q, want it to contain %q", err, tt.want) + } + blamesTraps := tt.whileWaiting && tt.traps > 1 + if strings.Contains(err.Error(), "traps caught it") != blamesTraps { + t.Fatalf("Release() = %q, want a mention of other traps: %v", err, blamesTraps) + } + + stopTicker() + for _, trap := range traps[1:] { + c, err := trap.Wait(t.Context()) + if err != nil { + t.Fatal(err) + } + if err := c.Release(t.Context()); err != nil { + t.Fatal(err) + } + } + <-returned + for _, trap := range traps { + trap.Close() + } + if tb.failed { + t.Fatal("Close() reported a failure after every trap released the call") + } + }) }) } @@ -484,7 +480,8 @@ func TestCallRelease_ExpiredContext(t *testing.T) { go func() { defer close(returned) // Callers may reuse a variadic slice once the call returns, so MustRelease must - // describe the call before it releases it. + // describe the call before it releases it. If it does not, -race reports a data race + // here, and without -race the message can show "after". tags := []string{"before"} mClock.Now(tags...) tags[0] = "after" @@ -550,10 +547,19 @@ func Test_UnreleasedCalls(t *testing.T) { } released := make(chan error, 1) go func() { released <- c.Release(ctx) }() - <-returned + select { + case <-returned: + case <-ctx.Done(): + t.Fatal("the call did not return after Release") + } trap.Close() - if err := <-released; err != nil { - t.Fatal(err) + select { + case err := <-released: + if err != nil { + t.Fatal(err) + } + case <-ctx.Done(): + t.Fatal("Release() did not return after the call completed") } if tb.failed { t.Fatal("Close() reported a failure after the released call returned") @@ -650,25 +656,6 @@ func tRunFail(t testing.TB, f func(t testing.TB)) { } } -// expiresOnWaitCtx is a context that is live until its Done channel is first requested, and then -// expires. Release, Trap.Wait and MustWait check ctx.Err() on entry and call Done only to wait, so -// it stands in for a context that expires while they wait, without depending on timing. -type expiresOnWaitCtx struct { - context.Context - cancel context.CancelFunc -} - -func newExpiresOnWaitCtx(t testing.TB) expiresOnWaitCtx { - ctx, cancel := context.WithCancel(context.Background()) - t.Cleanup(cancel) - return expiresOnWaitCtx{Context: ctx, cancel: cancel} -} - -func (c expiresOnWaitCtx) Done() <-chan struct{} { - c.cancel() - return c.Context.Done() -} - // wantMessages fails the test unless got has one message per want, each containing its want. func wantMessages(t testing.TB, got []string, want ...string) { t.Helper() @@ -802,50 +789,46 @@ func TestAdvanceWaiter_ExpiredContext(t *testing.T) { } mClock.Advance(time.Second).MustWait(expired) - mClock.Advance(time.Second).MustWait(newExpiresOnWaitCtx(t)) - wantMessages(t, tb.messages, "already expired when MustWait was called", - "while waiting for clock to advance") + wantMessages(t, tb.messages, "already expired when MustWait was called") }) t.Run("Pending", func(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() + synctest.Test(t, func(t *testing.T) { + expired, cancel := context.WithDeadline(t.Context(), time.Now().Add(-time.Second)) + defer cancel() + tb := &captureFailTB{TB: t} + mClock := quartz.NewMock(tb) + release := make(chan struct{}) + defer close(release) + // The advance cannot complete until the AfterFunc returns. + mClock.AfterFunc(time.Second, func() { <-release }) + w := mClock.Advance(time.Second) - tb := &captureFailTB{TB: t} - mClock := quartz.NewMock(tb) - release := make(chan struct{}) - // The advance cannot complete until the AfterFunc returns. - mClock.AfterFunc(time.Second, func() { <-release }) - w := mClock.Advance(time.Second) - errs := make(chan error, 1) - expiresOnWait := newExpiresOnWaitCtx(t) - go func() { - err := w.Wait(expired) - w.MustWait(expired) - w.MustWait(expiresOnWait) - errs <- err - }() - select { - case err := <-errs: - close(release) - if !errors.Is(err, context.DeadlineExceeded) { + if err := w.Wait(expired); !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) } - case <-ctx.Done(): - close(release) - t.Fatal("Wait() or MustWait() did not return after the context expired") - } - wantMessages(t, tb.messages, "already expired when MustWait was called", - "while waiting for clock to advance") - if err := w.Wait(ctx); err != nil { - t.Fatal(err) - } + w.MustWait(expired) + + ctx, cancelWait := context.WithCancel(t.Context()) + done := make(chan struct{}) + go func() { + defer close(done) + w.MustWait(ctx) + }() + synctest.Wait() // MustWait is blocked waiting for the advance. + cancelWait() + <-done + wantMessages(t, tb.messages, "already expired when MustWait was called", + "while waiting for clock to advance") + }) }) } // TestTrapWait_ExpiredContext verifies that Wait returns the context error, without blocking, when -// the context expires, even if the trap is closed. +// the context expires, even if a call is ready or the trap is closed, and that a ready call stays +// for the next Wait. MustWait must say the context had already expired exactly when it had expired +// before MustWait was called. func TestTrapWait_ExpiredContext(t *testing.T) { t.Parallel() @@ -892,29 +875,54 @@ func TestTrapWait_ExpiredContext(t *testing.T) { } }) - t.Run("ExpiresWhileWaiting", func(t *testing.T) { + t.Run("CallReady", func(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() + synctest.Test(t, func(t *testing.T) { + expired, cancel := context.WithCancel(t.Context()) + cancel() + mClock := quartz.NewMock(t) + trap := mClock.Trap().Now() + defer trap.Close() + // A Wait that picks at random between the ready call and ctx.Done fails only half the + // iterations, so repeat. + for range 100 { + go mClock.Now() + synctest.Wait() // The trap is blocked handing over the call. + c, err := trap.Wait(expired) + if err == nil { + c.MustRelease(t.Context()) + t.Fatal("Wait() returned the ready call, want the context error") + } + if !errors.Is(err, context.Canceled) { + t.Fatalf("Wait() = %v, want %v", err, context.Canceled) + } + c, err = trap.Wait(t.Context()) + if err != nil { + t.Fatalf("second Wait() = %v, want the ready call", err) + } + c.MustRelease(t.Context()) + } + }) + }) - mClock := quartz.NewMock(t) - trap := mClock.Trap().Now() - defer trap.Close() - // The context is live when Wait checks it on entry and expires once Wait waits. - waitCtx := newExpiresOnWaitCtx(t) - errs := make(chan error, 1) - go func() { - _, err := trap.Wait(waitCtx) - errs <- err - }() - select { - case err := <-errs: - if !errors.Is(err, context.Canceled) { + t.Run("ExpiresWhileWaiting", func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + mClock := quartz.NewMock(t) + trap := mClock.Trap().Now() + defer trap.Close() + ctx, cancel := context.WithCancel(t.Context()) + errs := make(chan error, 1) + go func() { + _, err := trap.Wait(ctx) + errs <- err + }() + synctest.Wait() // Wait is blocked waiting for a call. + cancel() + if err := <-errs; !errors.Is(err, context.Canceled) { t.Fatalf("Wait() = %v, want %v", err, context.Canceled) } - case <-ctx.Done(): - t.Fatal("Wait() did not return after the context expired") - } + }) }) }