Repository: stretchr/testify
Pull request #1427 (2023-07-15 18:37:32)

assert: check early in Eventually, EventuallyWithT, and Never
## Summary
Addresses #1424.

`Eventually` and `EventuallyWithT` current must always wait at least the polling duration before they can succeed. This PR starts checking the condition immediately. The assertion still fails if the initial check of the condition takes longer than the configured timeout.

## Changes
<!-- * Description of change 1 -->
<!-- * Description of change 2 -->
<!-- ... -->

## Motivation
This will provide a small optimization to callers, because tests can complete more quickly than they did before if conditions are met already.

## Related issues
<!-- Put `Closes #XXXX` for each issue number this PR fixes/closes -->


dolmen: This PR changes more than what it claims.

Please keep it focused on #1424 and move the changes related to the tick separately. Because it seems that runs of the condition will not overlap anymore.
connorszczepaniak-wk: @dolmen What extra changes are you referring to? The changes I made here were all to facilitate the addressing of #1424. How would you do it differently?
cszczepaniak: @dolmen What more does it change? The changes I made here are to facilitate addressing #1424. How would you do it differently?
cszczepaniak: #1439 could also address this issue. @dolmen what are your thoughts on this PR's approach vs. addressing #1439 instead?
dolmen: I intend to merge #1395 first. Could you help with your own review now, and we will work on your own changes after the merge?
cszczepaniak: @dolmen After #1395, I think we should pursue either the approach in this PR or the approach outlined in #1439, but not both. I will wait until we have the input needed to decide on #1439
dolmen: @cszczepaniak I just merged #1481 which uses `runtime.Goexit`. The single goroutine approach is incompatible.
cszczepaniak: @dolmen sounds good. I don't think that affects this PR, though.

Patch:
diff --git a/assert/assertions.go b/assert/assertions.go
index 104a0c9..538f2e2 100644
--- a/assert/assertions.go
+++ b/assert/assertions.go
@@ -1931,6 +1931,7 @@ func Eventually(t TestingT, condition func() bool, waitFor time.Duration, tick t
 	}
 
 	ch := make(chan bool, 1)
+	checkCond := func() { ch <- condition() }
 
 	timer := time.NewTimer(waitFor)
 	defer timer.Stop()
@@ -1938,18 +1939,23 @@ func Eventually(t TestingT, condition func() bool, waitFor time.Duration, tick t
 	ticker := time.NewTicker(tick)
 	defer ticker.Stop()
 
-	for tick := ticker.C; ; {
+	var tickC <-chan time.Time
+
+	// Check the condition once first on the initial call.
+	go checkCond()
+
+	for {
 		select {
 		case <-timer.C:
 			return Fail(t, "Condition never satisfied", msgAndArgs...)
-		case <-tick:
-			tick = nil
-			go func() { ch <- condition() }()
+		case <-tickC:
+			tickC = nil
+			go checkCond()
 		case v := <-ch:
 			if v {
 				return true
 			}
-			tick = ticker.C
+			tickC = ticker.C
 		}
 	}
 }
@@ -2019,35 +2025,42 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time
 	var lastFinishedTickErrs []error
 	ch := make(chan *CollectT, 1)
 
+	checkCond := func() {
+		collect := new(CollectT)
+		defer func() {
+			ch <- collect
+		}()
+		condition(collect)
+	}
+
 	timer := time.NewTimer(waitFor)
 	defer timer.Stop()
 
 	ticker := time.NewTicker(tick)
 	defer ticker.Stop()
 
-	for tick := ticker.C; ; {
+	var tickC <-chan time.Time
+
+	// Check the condition once first on the initial call.
+	go checkCond()
+
+	for {
 		select {
 		case <-timer.C:
 			for _, err := range lastFinishedTickErrs {
 				t.Errorf("%v", err)
 			}
 			return Fail(t, "Condition never satisfied", msgAndArgs...)
-		case <-tick:
-			tick = nil
-			go func() {
-				collect := new(CollectT)
-				defer func() {
-					ch <- collect
-				}()
-				condition(collect)
-			}()
+		case <-tickC:
+			tickC = nil
+			go checkCond()
 		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 = collect.errors
-			tick = ticker.C
+			tickC = ticker.C
 		}
 	}
 }
@@ -2062,6 +2075,7 @@ func Never(t TestingT, condition func() bool, waitFor time.Duration, tick time.D
 	}
 
 	ch := make(chan bool, 1)
+	checkCond := func() { ch <- condition() }
 
 	timer := time.NewTimer(waitFor)
 	defer timer.Stop()
@@ -2069,18 +2083,23 @@ func Never(t TestingT, condition func() bool, waitFor time.Duration, tick time.D
 	ticker := time.NewTicker(tick)
 	defer ticker.Stop()
 
-	for tick := ticker.C; ; {
+	var tickC <-chan time.Time
+
+	// Check the condition once first on the initial call.
+	go checkCond()
+
+	for {
 		select {
 		case <-timer.C:
 			return true
-		case <-tick:
-			tick = nil
-			go func() { ch <- condition() }()
+		case <-tickC:
+			tickC = nil
+			go checkCond()
 		case v := <-ch:
 			if v {
 				return Fail(t, "Condition satisfied", msgAndArgs...)
 			}
-			tick = ticker.C
+			tickC = ticker.C
 		}
 	}
 }


Test patch:
diff --git a/assert/assertions_test.go b/assert/assertions_test.go
index 064b92f..dc7c5bd 100644
--- a/assert/assertions_test.go
+++ b/assert/assertions_test.go
@@ -2980,6 +2980,49 @@ func TestEventuallyWithTFailNow(t *testing.T) {
 	Len(t, mockT.errors, 1)
 }
 
+// Check that a long running condition doesn't block Eventually.
+// See issue 805 (and its long tail of following issues)
+func TestEventuallyTimeout(t *testing.T) {
+	mockT := new(testing.T)
+
+	NotPanics(t, func() {
+		done, done2 := make(chan struct{}), make(chan struct{})
+
+		// A condition function that returns after the Eventually timeout
+		condition := func() bool {
+			// Wait until Eventually times out and terminates
+			<-done
+			close(done2)
+			return true
+		}
+
+		False(t, Eventually(mockT, condition, time.Millisecond, time.Microsecond))
+
+		close(done)
+		<-done2
+	})
+}
+
+func TestEventuallySucceedQuickly(t *testing.T) {
+	mockT := new(testing.T)
+
+	condition := func() bool { return true }
+
+	// By making the tick longer than the total duration, we expect that this test would fail if
+	// we didn't check the condition before the first tick elapses.
+	True(t, Eventually(mockT, condition, 100*time.Millisecond, time.Second))
+}
+
+func TestEventuallyWithTSucceedQuickly(t *testing.T) {
+	mockT := new(testing.T)
+
+	condition := func(t *CollectT) {}
+
+	// By making the tick longer than the total duration, we expect that this test would fail if
+	// we didn't check the condition before the first tick elapses.
+	True(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, time.Second))
+}
+
 func TestNeverFalse(t *testing.T) {
 	condition := func() bool {
 		return false
@@ -3007,27 +3050,13 @@ func TestNeverTrue(t *testing.T) {
 	False(t, Never(mockT, condition, 100*time.Millisecond, 20*time.Millisecond))
 }
 
-// Check that a long running condition doesn't block Eventually.
-// See issue 805 (and its long tail of following issues)
-func TestEventuallyTimeout(t *testing.T) {
+func TestNeverFailQuickly(t *testing.T) {
 	mockT := new(testing.T)
 
-	NotPanics(t, func() {
-		done, done2 := make(chan struct{}), make(chan struct{})
-
-		// A condition function that returns after the Eventually timeout
-		condition := func() bool {
-			// Wait until Eventually times out and terminates
-			<-done
-			close(done2)
-			return true
-		}
-
-		False(t, Eventually(mockT, condition, time.Millisecond, time.Microsecond))
-
-		close(done)
-		<-done2
-	})
+	// By making the tick longer than the total duration, we expect that this test would fail if
+	// we didn't check the condition before the first tick elapses.
+	condition := func() bool { return true }
+	False(t, Never(mockT, condition, 100*time.Millisecond, time.Second))
 }
 
 func Test_validateEqualArgs(t *testing.T) {