diff --git a/assert/assertions.go b/assert/assertions.go index 166f63726..a97cd8cf5 100644 --- a/assert/assertions.go +++ b/assert/assertions.go @@ -2009,6 +2009,25 @@ func Eventually(t TestingT, condition func() bool, waitFor time.Duration, tick t h.Helper() } + // A nil condition would be dereferenced on the goroutine below, where the + // panic cannot be recovered by the caller and takes the whole binary down. + // Reported as a failure rather than treated as vacuously satisfied: Never is + // given the same treatment as Eventually so that one malformed argument fails + // the same way in all three entry points, instead of Never quietly passing + // because there was no condition for it to violate. + if condition == nil { + return Fail(t, "Condition must not be nil", msgAndArgs...) + } + + // The ticker is built here, before the condition has been called even once, and + // time.NewTicker panics on a non-positive interval. That panic happens in the + // caller's goroutine, so it aborts the test run instead of failing one + // assertion -- and there is nothing to wait for, since a zero or negative tick + // can never be a usable polling interval. + if tick <= 0 { + return Fail(t, "Tick must be positive", msgAndArgs...) + } + ch := make(chan bool, 1) checkCond := func() { ch <- condition() } @@ -2104,6 +2123,25 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time h.Helper() } + // A nil condition would be dereferenced on the goroutine below, where the + // panic cannot be recovered by the caller and takes the whole binary down. + // Reported as a failure rather than treated as vacuously satisfied: Never is + // given the same treatment as Eventually so that one malformed argument fails + // the same way in all three entry points, instead of Never quietly passing + // because there was no condition for it to violate. + if condition == nil { + return Fail(t, "Condition must not be nil", msgAndArgs...) + } + + // The ticker is built here, before the condition has been called even once, and + // time.NewTicker panics on a non-positive interval. That panic happens in the + // caller's goroutine, so it aborts the test run instead of failing one + // assertion -- and there is nothing to wait for, since a zero or negative tick + // can never be a usable polling interval. + if tick <= 0 { + return Fail(t, "Tick must be positive", msgAndArgs...) + } + var lastFinishedTickErrs []error ch := make(chan *CollectT, 1) @@ -2156,6 +2194,25 @@ func Never(t TestingT, condition func() bool, waitFor time.Duration, tick time.D h.Helper() } + // A nil condition would be dereferenced on the goroutine below, where the + // panic cannot be recovered by the caller and takes the whole binary down. + // Reported as a failure rather than treated as vacuously satisfied: Never is + // given the same treatment as Eventually so that one malformed argument fails + // the same way in all three entry points, instead of Never quietly passing + // because there was no condition for it to violate. + if condition == nil { + return Fail(t, "Condition must not be nil", msgAndArgs...) + } + + // The ticker is built here, before the condition has been called even once, and + // time.NewTicker panics on a non-positive interval. That panic happens in the + // caller's goroutine, so it aborts the test run instead of failing one + // assertion -- and there is nothing to wait for, since a zero or negative tick + // can never be a usable polling interval. + if tick <= 0 { + return Fail(t, "Tick must be positive", msgAndArgs...) + } + ch := make(chan bool, 1) checkCond := func() { ch <- condition() } diff --git a/assert/assertions_test.go b/assert/assertions_test.go index 11642e096..f098a975e 100644 --- a/assert/assertions_test.go +++ b/assert/assertions_test.go @@ -3634,6 +3634,40 @@ func TestNeverFailQuickly(t *testing.T) { False(t, Never(mockT, condition, 100*time.Millisecond, time.Second)) } +// A nil condition used to be dereferenced on the goroutine Eventually/Never +// spawn, so the panic could not be recovered by the caller and took the whole +// test binary down. +func TestEventuallyNilCondition(t *testing.T) { + t.Parallel() + + mockT := new(testing.T) + + NotPanics(t, func() { + False(t, Eventually(mockT, nil, 100*time.Millisecond, 20*time.Millisecond)) + }) + False(t, Never(mockT, nil, 100*time.Millisecond, 20*time.Millisecond)) + + mockCollectT := new(CollectT) + False(t, EventuallyWithT(mockCollectT, nil, 100*time.Millisecond, 20*time.Millisecond)) + Len(t, mockCollectT.errors, 1) +} + +// time.NewTicker panics on a non-positive interval, and the ticker is created +// unconditionally even though it is only read after the first result. +func TestEventuallyNonPositiveTick(t *testing.T) { + t.Parallel() + + for _, tick := range []time.Duration{0, -time.Second} { + mockT := new(testing.T) + + NotPanics(t, func() { + False(t, Eventually(mockT, func() bool { return true }, 100*time.Millisecond, tick)) + False(t, Never(mockT, func() bool { return true }, 100*time.Millisecond, tick)) + False(t, EventuallyWithT(mockT, func(*CollectT) {}, 100*time.Millisecond, tick)) + }) + } +} + func Test_validateEqualArgs(t *testing.T) { t.Parallel()