Repository: stretchr/testify
Pull request #1481 (2023-10-08 18:25:52)

assert: collect.FailNow() should not panic
## Summary
`collect.FailNow()` should exit goroutine without a panic to be usable with `require` package.

## Changes
`collect.FailNow()` just does `runtime.Goexit()` instead of `panic()`. For example `FailNow()` from `testing` package [behaves similarly](https://cs.opensource.google/go/go/+/refs/tags/go1.21.2:src/testing/testing.go;l=973).

## Motivation

I just want `require` package to be usable with `EventuallyWithT` e.g. I want this example to pass but it panics:

```go
package main

import (
	"testing"
	"time"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

func TestRequireEventuallyWithT(t *testing.T) {
	state := 0
	require.EventuallyWithT(t, func(c *assert.CollectT) {
		defer func() { state += 1 }()
		require.True(c, state == 2)
	}, 100*time.Millisecond, 10*time.Millisecond)
}
```

See https://go.dev/play/p/Oqd95IT7qxQ

## Related issues
Fixes https://github.com/stretchr/testify/issues/1396
Fixes https://github.com/stretchr/testify/issues/1457


dolmen: @marshall-lee Your review of #1395 would be welcome.
dolmen: #1395 is now merged.

@marshall-lee Please rebase.

@czeslavo Please review.
czeslavo: I'm wondering whether we could document this behavior somewhere. It would be cool to have an example of using `EventuallyWithT` with a series of `require` and `assert` checks with an explanation of what happens underneath (i.e. if one of the `require` checks is not satisfied, this single condition run is aborted, but `EventuallyWithT` will retry if the timeout hasn't passed yet, etc.)
dolmen: @cszczepaniak Could you also help with a review of this MR?
marshall-lee: @czeslavo @cszczepaniak @dolmen Hey I tried to address all the previous review issues. Please review.
marshall-lee: @czeslavo @cszczepaniak @dolmen Hi! Any plans on merging this?

Patch:
diff --git a/assert/assertions.go b/assert/assertions.go
index 45777f0..104a0c9 100644
--- a/assert/assertions.go
+++ b/assert/assertions.go
@@ -1956,6 +1956,9 @@ func Eventually(t TestingT, condition func() bool, waitFor time.Duration, tick t
 
 // CollectT implements the TestingT interface and collects all errors.
 type CollectT struct {
+	// A slice of errors. Non-nil slice denotes a failure.
+	// If it's non-nil but len(c.errors) == 0, this is also a failure
+	// obtained by direct c.FailNow() call.
 	errors []error
 }
 
@@ -1964,9 +1967,10 @@ func (c *CollectT) Errorf(format string, args ...interface{}) {
 	c.errors = append(c.errors, fmt.Errorf(format, args...))
 }
 
-// FailNow panics.
-func (*CollectT) FailNow() {
-	panic("Assertion failed")
+// FailNow stops execution by calling runtime.Goexit.
+func (c *CollectT) FailNow() {
+	c.fail()
+	runtime.Goexit()
 }
 
 // Deprecated: That was a method for internal usage that should not have been published. Now just panics.
@@ -1979,6 +1983,16 @@ func (*CollectT) Copy(TestingT) {
 	panic("Copy() is deprecated")
 }
 
+func (c *CollectT) fail() {
+	if !c.failed() {
+		c.errors = []error{} // Make it non-nil to mark a failure.
+	}
+}
+
+func (c *CollectT) failed() bool {
+	return c.errors != nil
+}
+
 // EventuallyWithT asserts that given condition will be met in waitFor time,
 // periodically checking target function each tick. In contrast to Eventually,
 // it supplies a CollectT to the condition function, so that the condition
@@ -2003,7 +2017,7 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time
 	}
 
 	var lastFinishedTickErrs []error
-	ch := make(chan []error, 1)
+	ch := make(chan *CollectT, 1)
 
 	timer := time.NewTimer(waitFor)
 	defer timer.Stop()
@@ -2023,16 +2037,16 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time
 			go func() {
 				collect := new(CollectT)
 				defer func() {
-					ch <- collect.errors
+					ch <- collect
 				}()
 				condition(collect)
 			}()
-		case errs := <-ch:
-			if len(errs) == 0 {
+		case collect := <-ch:
+			if !collect.failed() {
 				return true
 			}
 			// Keep the errors from the last ended condition, so that they can be copied to t if timeout is reached.
-			lastFinishedTickErrs = errs
+			lastFinishedTickErrs = collect.errors
 			tick = ticker.C
 		}
 	}


Test patch:
diff --git a/assert/assertions_test.go b/assert/assertions_test.go
index 8b87727..064b92f 100644
--- a/assert/assertions_test.go
+++ b/assert/assertions_test.go
@@ -2923,16 +2923,15 @@ func TestEventuallyWithTFalse(t *testing.T) {
 func TestEventuallyWithTTrue(t *testing.T) {
 	mockT := new(errorsCapturingT)
 
-	state := 0
+	counter := 0
 	condition := func(collect *CollectT) {
-		defer func() {
-			state += 1
-		}()
-		True(collect, state == 2)
+		counter += 1
+		True(collect, counter == 2)
 	}
 
 	True(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond))
 	Len(t, mockT.errors, 0)
+	Equal(t, 2, counter, "Condition is expected to be called 2 times")
 }
 
 func TestEventuallyWithT_ConcurrencySafe(t *testing.T) {
@@ -2970,6 +2969,17 @@ func TestEventuallyWithT_ReturnsTheLatestFinishedConditionErrors(t *testing.T) {
 	Len(t, mockT.errors, 2)
 }
 
+func TestEventuallyWithTFailNow(t *testing.T) {
+	mockT := new(CollectT)
+
+	condition := func(collect *CollectT) {
+		collect.FailNow()
+	}
+
+	False(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond))
+	Len(t, mockT.errors, 1)
+}
+
 func TestNeverFalse(t *testing.T) {
 	condition := func() bool {
 		return false
diff --git a/require/requirements_test.go b/require/requirements_test.go
index febf0c1..824b159 100644
--- a/require/requirements_test.go
+++ b/require/requirements_test.go
@@ -5,6 +5,8 @@ import (
 	"errors"
 	"testing"
 	"time"
+
+	"github.com/stretchr/testify/assert"
 )
 
 // AssertionTesterInterface defines an interface to be used for testing assertion methods
@@ -681,3 +683,30 @@ func TestErrorAssertionFunc(t *testing.T) {
 		})
 	}
 }
+
+func TestEventuallyWithTFalse(t *testing.T) {
+	mockT := new(MockT)
+
+	condition := func(collect *assert.CollectT) {
+		True(collect, false)
+	}
+
+	EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond)
+	True(t, mockT.Failed, "Check should fail")
+}
+
+func TestEventuallyWithTTrue(t *testing.T) {
+	mockT := new(MockT)
+
+	counter := 0
+	condition := func(collect *assert.CollectT) {
+		defer func() {
+			counter += 1
+		}()
+		True(collect, counter == 1)
+	}
+
+	EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond)
+	False(t, mockT.Failed, "Check should pass")
+	Equal(t, 2, counter, "Condition is expected to be called 2 times")
+}