diff --git a/assert/assert_assertions.go b/assert/assert_assertions.go index 496049885..70f22c9c3 100644 --- a/assert/assert_assertions.go +++ b/assert/assert_assertions.go @@ -516,7 +516,13 @@ func Eventually[C Conditioner](t T, condition C, timeout time.Duration, tick tim // If the condition is not met before the timeout, the collected errors from the // last tick are copied to t. // -// Calling [CollectT.FailNow] cancels the condition immediately and causes the assertion to fail. +// Calling [CollectT.FailNow] (directly, or transitively through [require] assertions) +// fails the current tick only: the poller will retry on the next tick. This means +// [require]-style assertions inside [EventuallyWith] behave naturally — they abort +// the current evaluation and let the polling loop converge. +// +// To abort the whole assertion immediately (e.g. when the condition can no longer +// be expected to succeed), call [CollectT.Cancel]. // // # Usage // @@ -540,10 +546,15 @@ func Eventually[C Conditioner](t T, condition C, timeout time.Duration, tick tim // The condition function is never executed in parallel: only one goroutine executes it. // It may write to variables outside its scope without triggering race conditions. // +// The condition is wrapped in its own goroutine, so a call to [runtime.Goexit] +// (e.g. via [require] assertions or [CollectT.FailNow]) cleanly aborts only the +// current tick. +// // # Examples // // success: func(c *CollectT) { True(c,true) }, 100*time.Millisecond, 20*time.Millisecond // failure: func(c *CollectT) { False(c,true) }, 100*time.Millisecond, 20*time.Millisecond +// failure: func(c *CollectT) { c.Cancel() }, 100*time.Millisecond, 20*time.Millisecond // // Upon failure, the test [T] is marked as failed and continues execution. func EventuallyWith[C CollectibleConditioner](t T, condition C, timeout time.Duration, tick time.Duration, msgAndArgs ...any) bool { diff --git a/assert/assert_assertions_test.go b/assert/assert_assertions_test.go index e7ba42dc1..e21e7701a 100644 --- a/assert/assert_assertions_test.go +++ b/assert/assert_assertions_test.go @@ -528,6 +528,19 @@ func TestEventuallyWith(t *testing.T) { t.Error("EventuallyWith should mark test as failed") } }) + + t.Run("failure", func(t *testing.T) { + t.Parallel() + + mock := new(mockT) + result := EventuallyWith(mock, func(c *CollectT) { c.Cancel() }, 100*time.Millisecond, 20*time.Millisecond) + if result { + t.Error("EventuallyWith should return false on failure") + } + if !mock.failed { + t.Error("EventuallyWith should mark test as failed") + } + }) } func TestExactly(t *testing.T) { diff --git a/assert/assert_format_test.go b/assert/assert_format_test.go index 20519ed4d..fcea9885b 100644 --- a/assert/assert_format_test.go +++ b/assert/assert_format_test.go @@ -528,6 +528,19 @@ func TestEventuallyWithf(t *testing.T) { t.Error("EventuallyWithf should mark test as failed") } }) + + t.Run("failure", func(t *testing.T) { + t.Parallel() + + mock := new(mockT) + result := EventuallyWithf(mock, func(c *CollectT) { c.Cancel() }, 100*time.Millisecond, 20*time.Millisecond, "test message") + if result { + t.Error("EventuallyWithf should return false on failure") + } + if !mock.failed { + t.Error("EventuallyWithf should mark test as failed") + } + }) } func TestExactlyf(t *testing.T) { diff --git a/docs/doc-site/api/condition.md b/docs/doc-site/api/condition.md index cbf6d8596..bad61b566 100644 --- a/docs/doc-site/api/condition.md +++ b/docs/doc-site/api/condition.md @@ -798,13 +798,23 @@ The supplied [CollectT](https://pkg.go.dev/github.com/go-openapi/testify/v2/asse If the condition is not met before the timeout, the collected errors from the last tick are copied to t. -Calling [CollectT.FailNow](https://pkg.go.dev/CollectT#FailNow) cancels the condition immediately and causes the assertion to fail. +Calling [CollectT.FailNow](https://pkg.go.dev/CollectT#FailNow) (directly, or transitively through [require](https://pkg.go.dev/github.com/go-openapi/testify/v2/assert#require) assertions) +fails the current tick only: the poller will retry on the next tick. This means +[require](https://pkg.go.dev/github.com/go-openapi/testify/v2/assert#require)-style assertions inside [EventuallyWith](https://pkg.go.dev/github.com/go-openapi/testify/v2/assert#EventuallyWith) behave naturally — they abort +the current evaluation and let the polling loop converge. + +To abort the whole assertion immediately (e.g. when the condition can no longer +be expected to succeed), call [CollectT.Cancel](https://pkg.go.dev/CollectT#Cancel). #### Concurrency The condition function is never executed in parallel: only one goroutine executes it. It may write to variables outside its scope without triggering race conditions. +The condition is wrapped in its own goroutine, so a call to [runtime.Goexit](https://pkg.go.dev/runtime#Goexit) +(e.g. via [require](https://pkg.go.dev/github.com/go-openapi/testify/v2/assert#require) assertions or [CollectT.FailNow](https://pkg.go.dev/CollectT#FailNow)) cleanly aborts only the +current tick. + {{% expand title="Examples" %}} {{< tabs >}} {{% tab title="Usage" %}} @@ -824,6 +834,7 @@ It may write to variables outside its scope without triggering race conditions. ) success: func(c *CollectT) { True(c,true) }, 100*time.Millisecond, 20*time.Millisecond failure: func(c *CollectT) { False(c,true) }, 100*time.Millisecond, 20*time.Millisecond + failure: func(c *CollectT) { c.Cancel() }, 100*time.Millisecond, 20*time.Millisecond ``` {{< /tab >}} {{% tab title="Testable Examples (assert)" %}} @@ -924,7 +935,7 @@ func main() { |--|--| | [`assertions.EventuallyWith[C CollectibleConditioner](t T, condition C, timeout time.Duration, tick time.Duration, msgAndArgs ...any) bool`](https://pkg.go.dev/github.com/go-openapi/testify/v2/internal/assertions#EventuallyWith) | internal implementation | -**Source:** [github.com/go-openapi/testify/v2/internal/assertions#EventuallyWith](https://github.com/go-openapi/testify/blob/master/internal/assertions/condition.go#L253) +**Source:** [github.com/go-openapi/testify/v2/internal/assertions#EventuallyWith](https://github.com/go-openapi/testify/blob/master/internal/assertions/condition.go#L264) {{% /tab %}} {{< /tabs >}} diff --git a/internal/assertions/condition.go b/internal/assertions/condition.go index 8cde3fd89..c6ac8d8cd 100644 --- a/internal/assertions/condition.go +++ b/internal/assertions/condition.go @@ -222,7 +222,13 @@ func Consistently[C Conditioner](t T, condition C, timeout time.Duration, tick t // If the condition is not met before the timeout, the collected errors from the // last tick are copied to t. // -// Calling [CollectT.FailNow] cancels the condition immediately and causes the assertion to fail. +// Calling [CollectT.FailNow] (directly, or transitively through [require] assertions) +// fails the current tick only: the poller will retry on the next tick. This means +// [require]-style assertions inside [EventuallyWith] behave naturally — they abort +// the current evaluation and let the polling loop converge. +// +// To abort the whole assertion immediately (e.g. when the condition can no longer +// be expected to succeed), call [CollectT.Cancel]. // // # Usage // @@ -246,10 +252,15 @@ func Consistently[C Conditioner](t T, condition C, timeout time.Duration, tick t // The condition function is never executed in parallel: only one goroutine executes it. // It may write to variables outside its scope without triggering race conditions. // +// The condition is wrapped in its own goroutine, so a call to [runtime.Goexit] +// (e.g. via [require] assertions or [CollectT.FailNow]) cleanly aborts only the +// current tick. +// // # Examples // // success: func(c *CollectT) { True(c,true) }, 100*time.Millisecond, 20*time.Millisecond // failure: func(c *CollectT) { False(c,true) }, 100*time.Millisecond, 20*time.Millisecond +// failure: func(c *CollectT) { c.Cancel() }, 100*time.Millisecond, 20*time.Millisecond func EventuallyWith[C CollectibleConditioner](t T, condition C, timeout time.Duration, tick time.Duration, msgAndArgs ...any) bool { // Domain: condition if h, ok := t.(H); ok { @@ -558,9 +569,19 @@ func (p *conditionPoller) executeCondition(parentCtx, ctx context.Context, failF case <-ctx.Done(): return // timeout = success case fn := <-p.conditionChan: - if err := fn(ctx); err != nil { - close(p.doneChan) // (condition true <=> returns error) = failure for Never and Consistently + var conditionWg sync.WaitGroup + conditionWg.Go(func() { // guards against the condition issue an early GoExit + + if err := fn(ctx); err != nil { + close(p.doneChan) // (condition true <=> returns error) = failure for Never and Consistently + } + }) + conditionWg.Wait() + + select { + case <-p.doneChan: // done: early exit return + default: } } } @@ -577,9 +598,19 @@ func (p *conditionPoller) executeCondition(parentCtx, ctx context.Context, failF failFunc(ctx.Err().Error()) return case fn := <-p.conditionChan: - if err := fn(ctx); err == nil { - close(p.doneChan) // (condition true <=> err == nil) = success for Eventually + var conditionWg sync.WaitGroup + conditionWg.Go(func() { // guards against the condition issue an early GoExit + + if err := fn(ctx); err == nil { + close(p.doneChan) // (condition true <=> err == nil) = success for Eventually + } + }) + conditionWg.Wait() + + select { + case <-p.doneChan: // done: early exit return + default: } } } @@ -660,6 +691,15 @@ func (p *conditionPoller) cancellableContext(parentCtx context.Context, timeout return ctx, cancel } +// Sentinel errors recorded by [CollectT.FailNow] and [CollectT.Cancel]. +// Kept package-private: callers should rely on observable behavior, not on +// the marker shape. They are distinguishable so future tooling can tell apart +// "tick aborted by require" from "user explicitly cancelled the assertion". +var ( + errFailNow = errors.New("collect: failed now (tick aborted)") + errCancelled = errors.New("collect: cancelled (assertion aborted)") +) + // CollectT implements the [T] interface and collects all errors. // // [CollectT] is specifically intended to be used with [EventuallyWith] and @@ -668,16 +708,20 @@ type CollectT struct { // Domain: condition // // Maintainer: - // 1. FailNow() no longer just exits the go routine, but cancels the context of the caller instead before exiting. - // 2. We no longer establish the distinction between c.error nil or empty. Non-empty is an error, full stop. - // 2. Deprecated methods have been removed. + // 1. FailNow() exits the current tick goroutine via runtime.Goexit (matching + // stretchr/testify semantics): require-style assertions abort the current + // evaluation and the poller retries on the next tick. It does NOT cancel + // the EventuallyWith context. + // 2. Cancel() is the explicit escape hatch: it cancels the EventuallyWith + // context before exiting via runtime.Goexit, aborting the whole assertion. + // 3. We no longer establish the distinction between c.errors nil or empty. + // Non-empty is an error, full stop. + // 4. Deprecated methods have been removed. // A slice of errors. Non-empty slice denotes a failure. - // NOTE: When c.FailNow() is called, it cancels the context and exits the goroutine. - // The "failed now" error is appended but may be lost if the goroutine exits before collection. errors []error - // cancelContext cancels the parent context on FailNow() + // cancelContext cancels the parent EventuallyWith context on Cancel(). cancelContext func() } @@ -689,13 +733,33 @@ func (c *CollectT) Errorf(format string, args ...any) { c.errors = append(c.errors, fmt.Errorf(format, args...)) } -// FailNow records a failure and cancels the parent [EventuallyWith] context, -// before exiting the current go routine with [runtime.Goexit]. +// FailNow records a failure for the current tick and exits the condition +// goroutine via [runtime.Goexit]. // -// This causes the assertion to fail immediately without waiting for a timeout. +// It does NOT cancel the [EventuallyWith] context: the poller will retry on +// the next tick. If a later tick succeeds, the assertion succeeds. If no tick +// ever succeeds before the timeout, the errors collected during the LAST tick +// (the one which most recently called FailNow) are reported on the parent t. +// +// To abort the whole assertion immediately, use [CollectT.Cancel]. func (c *CollectT) FailNow() { + c.errors = append(c.errors, errFailNow) + runtime.Goexit() +} + +// Cancel records a failure, cancels the [EventuallyWith] context, then exits +// the condition goroutine via [runtime.Goexit]. +// +// This aborts the whole assertion immediately, without waiting for the timeout. +// The errors collected during the cancelled tick are reported on the parent t. +// +// Use this when the condition can no longer be expected to succeed (e.g. an +// upstream resource has been observed in an unrecoverable state). For ordinary +// per-tick failures (e.g. "value not yet ready"), use [CollectT.FailNow] +// directly or transitively through [require] assertions. +func (c *CollectT) Cancel() { + c.errors = append(c.errors, errCancelled) c.cancelContext() - c.errors = append(c.errors, errors.New("failed now")) // so c.failed() is true (currently lost as not owned by another go routine) runtime.Goexit() } diff --git a/internal/assertions/condition_test.go b/internal/assertions/condition_test.go index 982a829de..1a43919af 100644 --- a/internal/assertions/condition_test.go +++ b/internal/assertions/condition_test.go @@ -294,6 +294,7 @@ func TestConditionEventuallyNoLeak(t *testing.T) { }) } +//nolint:gocognit,gocyclo,cyclop // subtests are actually not complex func TestConditionEventuallyWith(t *testing.T) { t.Parallel() @@ -406,28 +407,88 @@ func TestConditionEventuallyWith(t *testing.T) { } }) - t.Run("should fail with a call to collect.FailNow", func(t *testing.T) { + t.Run("collect.FailNow only fails the current tick (poller retries)", func(t *testing.T) { t.Parallel() mock := new(errorsCapturingT) - counter := 0 + var counter int + var mu sync.Mutex - // The call to FailNow cancels the execution context of EventuallyWith. - // so we don't have to wait for the timeout. + // FailNow on every tick: the poller must keep retrying until the timeout. condition := func(collect *CollectT) { + mu.Lock() counter++ + mu.Unlock() collect.FailNow() } + if EventuallyWith(mock, condition, testTimeout, testTick) { + t.Error("expected EventuallyWith to return false") + } + mu.Lock() + got := counter + mu.Unlock() + if got < 2 { + t.Errorf("expected the condition to be retried multiple times, got %d call(s)", got) + } + }) + + t.Run("collect.FailNow allows convergence on a later tick", func(t *testing.T) { + t.Parallel() + + mock := new(errorsCapturingT) + var counter int + var mu sync.Mutex + + // First few ticks fail via FailNow, then converge. + condition := func(collect *CollectT) { + mu.Lock() + counter++ + n := counter + mu.Unlock() + if n < 3 { + collect.FailNow() + } + } + + if !EventuallyWith(mock, condition, testTimeout, testTick) { + t.Error("expected EventuallyWith to eventually return true") + } + if len(mock.errors) != 0 { + t.Errorf("expected no errors reported on parent t after success, got %d: %v", len(mock.errors), mock.errors) + } + }) + + t.Run("collect.Cancel aborts the whole assertion immediately", func(t *testing.T) { + t.Parallel() + + mock := new(errorsCapturingT) + var counter int + var mu sync.Mutex + + // Cancel must short-circuit: a 30-minute timeout must NOT be waited on. + condition := func(collect *CollectT) { + mu.Lock() + counter++ + mu.Unlock() + collect.Cancel() + } + + start := time.Now() if EventuallyWith(mock, condition, 30*time.Minute, testTick) { t.Error("expected EventuallyWith to return false") } - const expectedErrors = 2 - if len(mock.errors) != expectedErrors { - t.Errorf("expected %d errors (0 accumulated + 2 from EventuallyWith), got %d", expectedErrors, len(mock.errors)) + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Errorf("expected Cancel to short-circuit, but EventuallyWith took %s", elapsed) + } + mu.Lock() + got := counter + mu.Unlock() + if got != 1 { + t.Errorf("expected the condition function to have been called only once, but got: %d", got) } - if counter != 1 { - t.Errorf("expected the condition function to have been called only once, but got: %d", counter) + if len(mock.errors) == 0 { + t.Error("expected at least one error reported on parent t after Cancel") } }) } diff --git a/require/require_assertions.go b/require/require_assertions.go index 99e590932..3025ae25d 100644 --- a/require/require_assertions.go +++ b/require/require_assertions.go @@ -588,7 +588,13 @@ func Eventually[C Conditioner](t T, condition C, timeout time.Duration, tick tim // If the condition is not met before the timeout, the collected errors from the // last tick are copied to t. // -// Calling [CollectT.FailNow] cancels the condition immediately and causes the assertion to fail. +// Calling [CollectT.FailNow] (directly, or transitively through [require] assertions) +// fails the current tick only: the poller will retry on the next tick. This means +// [require]-style assertions inside [EventuallyWith] behave naturally — they abort +// the current evaluation and let the polling loop converge. +// +// To abort the whole assertion immediately (e.g. when the condition can no longer +// be expected to succeed), call [CollectT.Cancel]. // // # Usage // @@ -612,10 +618,15 @@ func Eventually[C Conditioner](t T, condition C, timeout time.Duration, tick tim // The condition function is never executed in parallel: only one goroutine executes it. // It may write to variables outside its scope without triggering race conditions. // +// The condition is wrapped in its own goroutine, so a call to [runtime.Goexit] +// (e.g. via [require] assertions or [CollectT.FailNow]) cleanly aborts only the +// current tick. +// // # Examples // // success: func(c *CollectT) { True(c,true) }, 100*time.Millisecond, 20*time.Millisecond // failure: func(c *CollectT) { False(c,true) }, 100*time.Millisecond, 20*time.Millisecond +// failure: func(c *CollectT) { c.Cancel() }, 100*time.Millisecond, 20*time.Millisecond // // Upon failure, the test [T] is marked as failed and stops execution. func EventuallyWith[C CollectibleConditioner](t T, condition C, timeout time.Duration, tick time.Duration, msgAndArgs ...any) { diff --git a/require/require_assertions_test.go b/require/require_assertions_test.go index 96f590e99..8d1b25d0b 100644 --- a/require/require_assertions_test.go +++ b/require/require_assertions_test.go @@ -452,6 +452,17 @@ func TestEventuallyWith(t *testing.T) { t.Error("EventuallyWith should call FailNow()") } }) + + t.Run("failure", func(t *testing.T) { + t.Parallel() + + mock := new(mockFailNowT) + EventuallyWith(mock, func(c *CollectT) { c.Cancel() }, 100*time.Millisecond, 20*time.Millisecond) + // require functions don't return a value + if !mock.failed { + t.Error("EventuallyWith should call FailNow()") + } + }) } func TestExactly(t *testing.T) { diff --git a/require/require_format_test.go b/require/require_format_test.go index 1c1ed3b78..11b92a772 100644 --- a/require/require_format_test.go +++ b/require/require_format_test.go @@ -452,6 +452,17 @@ func TestEventuallyWithf(t *testing.T) { t.Error("EventuallyWithf should call FailNow()") } }) + + t.Run("failure", func(t *testing.T) { + t.Parallel() + + mock := new(mockFailNowT) + EventuallyWithf(mock, func(c *CollectT) { c.Cancel() }, 100*time.Millisecond, 20*time.Millisecond, "test message") + // require functions don't return a value + if !mock.failed { + t.Error("EventuallyWithf should call FailNow()") + } + }) } func TestExactlyf(t *testing.T) {