diff --git a/mock.go b/mock.go index 7255aff..8e182bf 100644 --- a/mock.go +++ b/mock.go @@ -211,6 +211,7 @@ func (m *Mock) matchCallLocked(c *apiCall) { if len(traps) == 0 { return } + c.trapCount = len(traps) c.releases.Add(len(traps)) m.mu.Unlock() for _, t := range traps { @@ -232,26 +233,32 @@ 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 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: - return nil case <-ctx.Done(): - return ctx.Err() } + // select picks a random ready case. Checking ctx afterwards makes an expired context fail the + // 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 completes first. MustWait must be called from the goroutine running the test or -// benchmark, similar to `t.FailNow()`. +// 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() - select { - case <-w.ch: + // 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 - 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) } } @@ -642,6 +649,9 @@ type apiCall struct { fn clockFunction releases sync.WaitGroup complete chan struct{} + // 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 } func (a *apiCall) String() string { @@ -686,32 +696,53 @@ 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. +// 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. 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 fmt.Errorf("context already expired; this trap released the call: %w", expired) + } select { case <-ctx.Done(): - return fmt.Errorf("timed out waiting for release; did more than one trap capture the call?: %w", ctx.Err()) + return c.releaseError(ctx.Err()) case <-c.apiCall.complete: - // OK + return nil } - c.trap.callReleased() - return nil } -// MustRelease releases the call and waits for it to complete. If the provided context expires before the call -// completes, it fails the test. +// 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, "+ + "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) +} + +// 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. 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.Fatal(err.Error()) + c.tb.Fatalf("MustRelease of %s: %s", call, err) } } @@ -746,7 +777,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 } @@ -784,9 +816,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 { @@ -804,7 +839,16 @@ 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 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) { + // 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(): return nil, ctx.Err() @@ -830,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 7bc822d..602c82c 100644 --- a/mock_test.go +++ b/mock_test.go @@ -7,8 +7,10 @@ import ( "fmt" "os" "runtime/pprof" + "strconv" "strings" "testing" + "testing/synctest" "time" "github.com/coder/quartz" @@ -372,45 +374,196 @@ func Test_MultipleTraps(t *testing.T) { } } -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 mentions other +// traps only when they kept the call from completing while Release waited. Close must not report +// the call, and a failed MustRelease must name MustRelease and the call. +func TestCallRelease_ExpiredContext(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) + expired, cancel := context.WithCancel(context.Background()) + cancel() + + for _, tt := range []struct { + name string + traps int + // 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 + }{ + {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"}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + 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...) + }() + + c, err := traps[0].Wait(t.Context()) + if err != nil { + t.Fatal(err) + } + releaseCtx, expire := context.WithCancel(t.Context()) + defer expire() + if !tt.whileWaiting { + expire() + } + 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") + } + }) + }) + } - trap0 := mClock.Trap().Now("0") - defer trap0.Close() - trap1 := mClock.Trap().Now("1") - defer trap1.Close() + 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() { - mClock.Now("0", "1") + defer close(returned) + // Callers may reuse a variadic slice once the call returns, so MustRelease must + // 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" }() - - c0 := trap0.MustWait(testCtx) - c0.MustRelease(testCtx) // deadlocks, test failure + 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([before]): context already expired; this trap") }) } +// 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 + c.Release(testCtx) // clean up goroutine + }) + }) + + // 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) + defer cancel() + + 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) }() + select { + case <-returned: + case <-ctx.Done(): + t.Fatal("the call did not return after Release") + } + trap.Close() + 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") + } }) } @@ -449,31 +602,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() { @@ -496,6 +656,19 @@ func tRunFail(t testing.TB, f func(t testing.TB)) { } } +// 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 } @@ -591,6 +764,168 @@ 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. 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. + expired, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + t.Cleanup(cancel) + + t.Run("Completed", func(t *testing.T) { + t.Parallel() + 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 { + // 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.Advance(time.Second).MustWait(expired) + wantMessages(t, tb.messages, "already expired when MustWait was called") + }) + + t.Run("Pending", func(t *testing.T) { + t.Parallel() + 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) + + if err := w.Wait(expired); !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Wait() = %v, want %v", err, context.DeadlineExceeded) + } + 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 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() + + 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()) + cancel() + + 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) + } + } + }) + + t.Run("CallReady", func(t *testing.T) { + t.Parallel() + 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()) + } + }) + }) + + 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) + } + }) + }) +} + func TestNoLeak_Go123(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)