Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the behavior of require assertions when using assert.EventuallyWithT #1396

Closed
antoninbas opened this issue Jun 2, 2023 · 3 comments · Fixed by #1481
Closed

Improve the behavior of require assertions when using assert.EventuallyWithT #1396

antoninbas opened this issue Jun 2, 2023 · 3 comments · Fixed by #1481
Labels
help wanted pkg-assert Change related to package testify/assert

Comments

@antoninbas
Copy link

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...)
		}
	}
  1. 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
Copy link
Collaborator

dolmen commented Jul 5, 2023

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

@dolmen dolmen added help wanted pkg-assert Change related to package testify/assert labels Jul 5, 2023
@antoninbas
Copy link
Author

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

michaeldwan added a commit to superfly/flyctl that referenced this issue Aug 29, 2023
It seems that some assertions will call `FailNow()` which panics when called on `assert.CollectT`.

See:
- stretchr/testify#1396
- stretchr/testify#1457
michaeldwan added a commit to superfly/flyctl that referenced this issue Aug 29, 2023
It seems that some assertions will call `FailNow()` which panics when called on `assert.CollectT`.

See:
- stretchr/testify#1396
- stretchr/testify#1457
marshall-lee added a commit to marshall-lee/testify that referenced this issue Oct 8, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Oct 13, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Oct 16, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Oct 16, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Oct 16, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Oct 16, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
marshall-lee added a commit to marshall-lee/testify that referenced this issue Nov 25, 2023
@jchappelow
Copy link

jchappelow commented Sep 23, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted pkg-assert Change related to package testify/assert
Projects
None yet
3 participants