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:

testify/assert/assertions.go

Lines 1872 to 1875 in f97607b

	// FailNow panics.
	func (c *CollectT) FailNow() {
	panic("Assertion failed")
	}

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.