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:
|
// 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:
- 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...)
}
}
- 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
Consider the following use of
EventuallyWithT.Here the intention is to fail
assert.EventuallyWithTimmediately if therequire.NoErrorassertions fails. Instead this code will panic ifmyFunc()returns an error. This is because of how theFailNowmethod is implemented on theCollectTobject:testify/assert/assertions.go
Lines 1872 to 1875 in f97607b
The panic ensures that the call to the
conditionfunction "exits" immediately. However it will immediately interrupt test execution.It feels like the implementation could be improved, so that it immediately fails the
assert.EventuallyWithTassertion, without crashing the test.There are 2 possible implementations IMO:
FailNowimplementation as follows:The relevant code in
EventuallyWithTcould then look like this:recoverinEventuallyWithTto 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 theconditionfunction, 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