Unable to use require within EventuallyWith

[boekkooi-impossiblecloud]

Describe the bug
Good day,

Currently I'm trying to migrate a project to go-openapi/testify and it seems to work well (thank you!).
I did run into one issue and that is that I'm unable to use (for example) require.Error inside assert.EventuallyWith as this always results in a condition never satisfied.
It looks like require.Error calls CollectT.FailNow which cancels the context causing failFunc(ctx.Err().Error()) to be called.

I'm not sure if this expected behavior or not. Could you please advice?

Thanks in advance and have a great day!

Steps to Reproduce

func TestExample(t *testing.T) {
	ctx, ctxCancel := context.WithTimeout(context.Background(), 10*time.Millisecond)
	t.Cleanup(ctxCancel)

	assert.EventuallyWith(t, func(collect *assert.CollectT) {
		require.Error(collect, ctx.Err())
	}, time.Second, time.Millisecond)
}

Expected behavior

=== RUN   TestExample
--- PASS: TestExample (0.01s)
PASS

Actual behavior

=== RUN   TestExample
    condition.go:480: context canceled
    testing.go:110: 
        	Error Trace:	/home/warnar/.asdf/installs/golang/1.26.2/go/src/runtime/asm_amd64.s:1771
        	Error:      	condition never satisfied
        	Test:       	TestExample
--- FAIL: TestExample (0.00s)

FAIL

Environment

GOVERSION='go1.26.2'

Additional context

[fredbi]

Hello. Ok I am going to investigate it. Stay tuned.

NOTE: Your feedback is highly valuable, precisely at this moment, because this project is about to bring a significant change to the "async" tests family using go1.25 sync test.

[fredbi]

@boekkooi-impossiblecloud I see the problem I think.

When using "require" what really happens is a Goexit exiting the current go routine when the assertion is not valid.
So it tries once, exit the go routine and this finishes the test immediately.

If you replace require by assert then it tries a number of times and eventually finds the expected timeout error.
It seems to me that this is what would be expected.

I am curious what would be your expectation there

[boekkooi-impossiblecloud]

@fredbi thanks for the very fast feedback!

As I mentioned I'm migrating a project to go-openapi/testify and the migration tool changed assert.EventuallyWithT to assert.EventuallyWith which resulted in the issues.

Personally I would expect the usage of require inside the condition to cancel everything within the context of the condition that is passed into EventuallyWith and try again after the tick (or stop once timeout is hit) similar to what stretchr/testify/ is doing.

Now, I see the case of saying that require and should be used within the scope of the test run. However, this means that when using EventuallyWith we no longer have an easy way "out" as we now always have to write if assert. { return } instead of require..

Personally I prefer the avoiding a BC break with stretchr/testify/ but a better error message saying that require was called causing the condition never satisfied would also work.

[fredbi]

yes the migration tool might be a little buggy there, knowing that the behavior of the async testing functions has been modified (even though the interface didn't change). These tests can be super tricky, and it is difficult to automate the move there.

About the "easy way out". Well EventuallyWith stops as soon as the condition is verified and tries till it times out.
The problem comes from require directly calling GoExit and the change that totally wrecks your use case is that this is always executed at least once. So perhaps I should change that part indeed.

[fredbi]

Pending further investigation I believe that the issue you're facing lies in the fact that the first execution of the condition is not spawned in an independent goroutine. If confirmed, your report indicates that this was definitely not a good idea from my side.

[boekkooi-impossiblecloud]

@fredbi I'm not sure this is the actual issue when I do the following it also seems to not work as I would expect.

func TestExample(t *testing.T) {
	ctx, ctxCancel := context.WithTimeout(context.Background(), 10*time.Millisecond)
	t.Cleanup(ctxCancel)
	var counter int
	assert.EventuallyWith(t, func(c *assert.CollectT) {
		if counter == 1 {
			require.Error(c, ctx.Err())
		}
		assert.Equal(c, 2, counter)
		counter++
	}, time.Second, time.Millisecond)
}

Actual behavior

=== RUN   TestExample
    condition.go:480: context canceled
    testing.go:110: 
        	Error Trace:	/home/warnar/.asdf/installs/golang/1.26.1/packages/pkg/mod/golang.org/toolchain@v0.0.1-go1.26.2.linux-amd64/src/runtime/asm_amd64.s:1771
        	Error:      	condition never satisfied
        	Test:       	TestExample
    condition.go:323: 
        	Error Trace:	/home/warnar/.asdf/installs/golang/1.26.1/packages/pkg/mod/golang.org/toolchain@v0.0.1-go1.26.2.linux-amd64/src/runtime/asm_amd64.s:1771
        	Error:      	Not equal: 
        	            	expected: 2
        	            	actual  : 0
--- FAIL: TestExample (0.00s)

Expected :2
Actual   :0

Expected behavior

=== RUN   TestExample
--- PASS: TestExample (0.00s)
PASS

[fredbi]

thanks. I think I've nailed it. In the new implementation, the condition doesn't spawn a new go routine at every execution. Rather, there is one single go routine that serializes all executions of the condition. In the (improved) design, this avoided race conditions between instances of a condition and could guarantee that things like counters etc would move as expected.

But this mechanism doesn't survive the early GoExit.

So I've managed to find a not-to-hard fix: conditions should now be wrapped in their own go routine, so even if they exit, the runner remains alive. With this addition, your example test passes just fine.

I think I am going to catch this opportunity to also guard there against a panic inside the condition.

[fredbi]

This was a good feedback and I had to revisit some assumptions quite a bit.
#90

There are a few forthcoming follow-up PRs for documentation, guard against panic, migration tool enhancement, but essentially PR #90 should get the desired behavior with require.

[boekkooi-impossiblecloud]

Good morning,

I just pulled in the latest master and it works like a charm now.

Thank you for this very fast fix and the nice back and forth 😄