Improve the behavior of `require` assertions when using `assert.EventuallyWithT`

[antoninbas]

Consider the following use of EventuallyWithT.

assert.EventuallyWithT(t, func(c *assert.CollectT) {
        exist, err := myFunc()
        require.NoError(c, err)
        assert.True(c, exist)
}, 1*time.Second, 10*time.Millisecond, "Object was not created")

Here the intention is to fail assert.EventuallyWithT immediately if the require.NoError assertions fails. Instead this code will panic if myFunc() returns an error. This is because of how the FailNow method is implemented on the CollectT object: https://github.com/stretchr/testify/blob/f97607b89807936ac4ff96748d766cf4b9711f78/assert/assertions.go#L1872-L1875

The panic ensures that the call to the condition function "exits" immediately. However it will immediately interrupt test execution. It feels like the implementation could be improved, so that it immediately fails the assert.EventuallyWithT assertion, without crashing the test.

There are 2 possible implementations IMO:

1. change the FailNow implementation as follows:

func (c *CollectT) FailNow() {
        // a new channel in CollectT, to indicate that execution of EventuallyWithT should stop immediately
        c.failed <- struct{}
        // terminate the Goroutine evaluating the condition
	runtime.Goexit()
}

The relevant code in EventuallyWithT could then look like this:

	for tick := ticker.C; ; {
		select {
		case <-timer.C:
			collect.Copy(t)
			return Fail(t, "Condition never satisfied", msgAndArgs...)
		case <-tick:
			tick = nil
			collect.Reset()
			go func() { // this Goroutine will exit if FailNow is called
				condition(collect)
				ch <- len(collect.errors) == 0
			}()
		case v := <-ch:
			if v {
				return true
			}
			tick = ticker.C
		}
		case v := <-collect.failed: // new case
			collect.Copy(t)
			return Fail(t, "Require assertion failed", msgAndArgs...)
		}
	}

2. the other option is to use recover in EventuallyWithT to recover from the panic. I am not going into too many details here, as I think the other solution is better. Panics can be cause by user code in the condition function, and we probably don't want to recover from these.

If the new suggested behavior is not acceptable, I'd like to understand the rationale for the current implementation.

cc @tnqn

[dolmen]

Maintenance work on testify is huge. Please ease work of the maintainers by providing an example running on the Go Playground. Here is a template: https://go.dev/play/p/hZ3USGA87Fk

[antoninbas]

Sure, I have created an example to showcase the current behavior (panic): https://go.dev/play/p/YiRjPaHFMr1

[jchappelow]

This is good improvement on EventuallyWithT that makes "fail fast" functionality more useful. panic in collect.FailNow doesn't work well for this use case. Can this be in a release?

[antoninbas]

For folks trying out the new testify release which includes this change (1.10), I want to point out that while this issue was closed when #1481 was merged, the behavior that was implemented didn't match my original request.

Considering the original example:

assert.EventuallyWithT(t, func(c *assert.CollectT) {
        exist, err := myFunc()
        require.NoError(c, err)
        assert.True(c, exist)
}, 1*time.Second, 10*time.Millisecond, "Object was not created")

The request was for assert.EventuallyWithT to fail immediately if the require.NoError(c, err) assertion failed. Instead, the behavior in testify 1.10.0 is to fail the condition function immediately (i.e., skip remaining assertions) if the require.NoError(c, err) assertion fails.

So in testify 1.10.0, assuming myFunc always returns an error (for the sake of the example), it will still take the full 1 second for the assert.EventuallyWithT assertion to return and for the error to be reported.

While I can't claim that one approach is clearly superior to the other one, I wished the behavior that was originally suggested was implemented. The current behavior can already be obtained with:

assert.EventuallyWithT(t, func(c *assert.CollectT) {
        exist, err := myFunc()
        if !assert.NoError(c, err) {
                return
        }
        assert.True(c, exist)
}, 1*time.Second, 10*time.Millisecond, "Object was not created")

while as far as I know, it is not possible to achieve the behavior originally requested.

[brackendawson]

I'll have a look at this.

[brackendawson]

This is working as designed and the functionality you desire can be achieved without any delays.

Consider this test which will panic because you cannot de-reference the nil i:

func TestIfy(t *testing.T) {
	f := func() (*int, error) {
		return nil, errors.New("my hovercraft is full of eels")
	}
	assert.EventuallyWithT(t, func(c *assert.CollectT) {
		i, _ := f()
		assert.Equal(c, 1, *i)
	}, time.Second, time.Millisecond)
}

The reason that CollectT supports require assertions (and calls to CollectT.FailNow) is to allow us to end the condition function early in a way we are familiar with. This version now fails gracefully because the condition function never tries to deference i:

func TestIfy(t *testing.T) {
	f := func() (*int, error) {
		return nil, errors.New("my hovercraft is full of eels")
	}
	assert.EventuallyWithT(t, func(c *assert.CollectT) {
		i, err := f()
		require.NoError(c, err)
		assert.Equal(c, 1, *i)
	}, time.Second, time.Millisecond)
}

If in your test, a non-nil err is fatal to the entire test, then you should use the outer *testing.T:

func TestIfy(t *testing.T) {
	f := func() (*int, error) {
		return nil, errors.New("my hovercraft is full of eels")
	}
	assert.EventuallyWithT(t, func(c *assert.CollectT) {
		i, err := f()
		require.NoError(t, err) // require with t, not c
		assert.Equal(c, 1, *i)
	}, 10_000*time.Hour, time.Millisecond)
}

This is better because:

• It will fail immediately, not after 10,000 hours.
• The first non-nil error will fail the test. There is no potential for the condition to initially find non-nil errors that you consider fatal but then pass after later finding nil errors.

[antoninbas]

Thanks for following up @brackendawson. While the proposed pattern does work, in my opinion it suffers from a couple of issues:

1. It invokes require.NoError (and hence t.FailNow) inside a goroutine (unless my understanding of the implementation of assert.EventuallyWithT is incorrect), which is not supported in theory and can create issues (https://pkg.go.dev/testing#T.FailNow). In this specific case, we actually get the desired behavior, because it terminates the collect goroutine, without actually failing the test immediately or impacting other goroutines. But this is very dependent on the current implementation.
2. It's not clear that the current behavior (failing the assert.EventuallyWithT assertion without interrupting the test) is actually the most meaningful one. I think it would be legitimate to assume that calling require.NoError on the "outer t" would cause the test to immediately fail in case of error, and stop execution.
3. It is not a documented pattern.

However, I feel like these issues could be solved with documentation, specifying that:

• the condition function is always invoked in its own goroutine
• a failed require assertion on the outer test object as part of a condition function is "valid" and will cause the assert.EventuallyWithT assertion to fail immediately, but will not stop test execution

I think the above documentation changes would provide confidence that this is a supported / intended pattern, and that it will keep working in future testify releases.

[brackendawson]

You are right in that calling FailNow on the outer testing.T from he condition function is unsupported. This could only be remedied by removing the use of a goroutine which requires reverting the initial fix to this issue by changing the CollectT.FailNow implementation back to a panic. While my example works sometimes there isn't a fully safe way to stop a test from the condition function, and there has never been one.

a failed require assertion on the outer test object as part of a condition function is "valid"...

I don't think it is, on further consideration.

...and will cause the assert.EventuallyWithT assertion to fail immediately...

I exepect the assert.EventuallyWithT wont fail at all

...but will not stop test execution.

Execution of the Go test/subtest will be stopped.

What to do

CollectT.FailNow currently says:

FailNow stops execution by calling runtime.Goexit.

But nothing says the condition function is run in a goroutine, so there is no reason to assume it fails and stops the condition function rather than the test. That behaviour has always been the case, so the documentation and example in EventuallyWithT should be updated to make that clear.

I'm going to keep this open until we resolve the documentation issue. If we want a kosher method of stopping the test execution from the condition function then we need a new issue.

[antoninbas]

I exepect the assert.EventuallyWithT wont fail at all

Yes, you're correct, I wasn't very accurate. The assertion doesn't fail, but its execution is stopped.

Execution of the Go test/subtest will be stopped.

That's not what I observe at the moment. The runtime.Goexit call from t.FailNow() is only used to terminate the goroutine spawned for the condition function. While the test is marked as failed, its execution will continue. See below for an example.

Example

C02F16ASMD6R:testify1.10 abas$ cat x_test.go
package main

import (
	"fmt"
	"testing"
	"time"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

func FunctionUnderTest(x int) (error, bool) {
	return fmt.Errorf("error in the building!"), false
}

func TestFunctionUnderTest(t *testing.T) {
	assert.EventuallyWithT(t, func(c *assert.CollectT) {
		err, ok := FunctionUnderTest(-1)
		require.NoError(t, err)
		assert.True(c, ok)
	}, 10*time.Second, 10*time.Millisecond)
	fmt.Println("Still running")
	assert.False(t, true)
}
C02F16ASMD6R:testify1.10 abas$ go test -v -count=1 .
=== RUN   TestFunctionUnderTest
    x_test.go:19:
        	Error Trace:	/Users/abas/bcom/scratch/testify1.10/x_test.go:19
        	            				/usr/local/go-1.23/src/runtime/asm_amd64.s:1700
        	Error:      	Received unexpected error:
        	            	error in the building!
        	Test:       	TestFunctionUnderTest
Still running
    x_test.go:23:
        	Error Trace:	/Users/abas/bcom/scratch/testify1.10/x_test.go:23
        	Error:      	Should be false
        	Test:       	TestFunctionUnderTest
--- FAIL: TestFunctionUnderTest (0.01s)
FAIL
FAIL	foo	0.351s
FAIL

I'm going to keep this open until we resolve the documentation issue. If we want a kosher method of stopping the test execution from the condition function then we need a new issue.

Well, personally when I opened this issue I only wanted a way to fail the assert.EventuallyWithT assertion immediately during one execution of the condition function. But looks like it's not on the table :P

[brackendawson]

You're right, the test keeps running. I don't think this can be guaranteed or documented here though. It's unsupported.

[kotahorii]

Documentation Enhancement Proposal for EventuallyWithT Behavior

Following the recent discussion about the behavior differences between the original request and the current implementation, I'd like to propose comprehensive documentation improvements that would clarify the current behavior and provide clear guidance for developers.

Current Documentation Gap

The main issue is that developers expect one behavior (immediate test failure) but get another (condition function termination). This leads to confusion and potentially incorrect test patterns.

Proposed Documentation Enhancements

1. Enhanced Function Documentation

// EventuallyWithT asserts that given condition will be met in waitFor time,
// periodically checking target function each tick.
//
// Goroutine Behavior:
//   The condition function is executed in its own goroutine for each attempt.
//   This means certain operations have specific behavioral implications:
//
// Using require with CollectT:
//   require.NoError(c, err) // ✓ Terminates condition function, continues EventuallyWithT
//
// Using require with outer testing.T:
//   require.NoError(t, err) // ⚠ Unsupported: calling t.FailNow() from goroutine
//                           // May have unpredictable behavior
//
// Recommended Patterns:
//   // For non-fatal assertions that should retry:
//   assert.EventuallyWithT(t, func(c *assert.CollectT) {
//       result, err := operation()
//       assert.NoError(c, err)  // Retries on failure
//       assert.True(c, result)
//   }, timeout, interval)
//
//   // For conditions that should fail fast:
//   assert.EventuallyWithT(t, func(c *assert.CollectT) {
//       result, err := operation()
//       if !assert.NoError(c, err) {
//           return  // Early termination of this condition attempt
//       }
//       assert.True(c, result)
//   }, timeout, interval)

2. Usage Examples Documentation

// Example: Database connectivity check with retry
assert.EventuallyWithT(t, func(c *assert.CollectT) {
    err := db.Ping()
    assert.NoError(c, err, "Database should be reachable")
}, 30*time.Second, time.Second)

// Example: Early termination on critical failure
assert.EventuallyWithT(t, func(c *assert.CollectT) {
    data, err := fetchCriticalData()
    if !assert.NoError(c, err) {
        return // Skip remaining assertions for this attempt
    }
    assert.NotEmpty(c, data.ImportantField)
}, 10*time.Second, 500*time.Millisecond)

3. Migration Guide for Common Patterns

// Before (problematic pattern):
assert.EventuallyWithT(t, func(c *assert.CollectT) {
    result, err := operation()
    require.NoError(c, err)  // Wanted immediate test failure
    assert.True(c, result)
}, timeout, interval)

// After (correct patterns):
// Option A: Use assert for retryable conditions
assert.EventuallyWithT(t, func(c *assert.CollectT) {
    result, err := operation()
    assert.NoError(c, err)   // Retries on failure
    assert.True(c, result)
}, timeout, interval)

// Option B: Use explicit early return for fail-fast behavior
assert.EventuallyWithT(t, func(c *assert.CollectT) {
    result, err := operation()
    if !assert.NoError(c, err) {
        return  // Explicit early termination
    }
    assert.True(c, result)
}, timeout, interval)

Benefits of Enhanced Documentation

1. Clear Behavior Expectations: Developers understand exactly what happens with require vs assert
2. Best Practices: Provides recommended patterns for common testing scenarios
3. Migration Path: Helps existing code transition to correct patterns
4. Goroutine Safety: Makes explicit the goroutine-related constraints

Implementation Suggestions

• Update function-level godoc with comprehensive behavior description
• Add usage examples to the official documentation
• Include troubleshooting section for common patterns
• Consider adding runtime warnings for potentially problematic usage (calling outer t.FailNow from condition function)

This documentation enhancement would address the core issue raised in this thread while maintaining backward compatibility and providing clear guidance for both new and existing users.

Would the maintainers be interested in a PR implementing these documentation improvements?

[brackendawson]

The best way to provide feedback for such a proposal is for you to open the pull request so I can review the proposed changes in context and test how they render with pkgsite. But I have these comments:

• // Using require with CollectT: // require.NoError(c, err) // ✓ Terminates condition function, continues EventuallyWithT // // Using require with outer testing.T: // require.NoError(t, err) // ⚠ Unsupported: calling t.FailNow() from goroutine // // May have unpredictable behavior

  Describe the behaviors in English without code snippets first, then provide only necessary examples.

• I think we can use just one example.

• The documentation needs to make sense when the require package is generated from the assert package. Or we can move the function to non-generated files to tailor the docs for each.

• Retries on failure

  This isn't precise, sounds like you're saying something re-runs the assertion in the same call of the condition function.