From c46795647479928278032ae9b8282e9fe0ac948f Mon Sep 17 00:00:00 2001 From: feiiiiii5 Date: Sun, 27 Sep 2026 09:23:17 +0800 Subject: [PATCH 1/3] assert: report failures instead of panicking in Eventually and Never Eventually, Never and EventuallyWithT dereference `condition` on a goroutine they spawn, so a nil condition panics somewhere the caller's recover cannot reach, and the whole test binary goes down. All three also hand `tick` straight to time.NewTicker, which panics on a non-positive interval, even though the ticker is only ever read after the first result. matchRegexp type-asserts before dereferencing, so a typed nil *regexp.Regexp passes the assertion and then crashes. Guard all three and report a failure with a message, which is what the rest of this package already does: isFunction, a few lines above matchRegexp, returns false for a nil argument rather than dereferencing it. A nil regexp has no pattern, so nothing can match and it is reported as no match, leaving Regexp to fail and NotRegexp to pass. Signed-off-by: feiiiiii5 --- assert/assertions.go | 41 +++++++++++++++++++++++++++++++++ assert/assertions_test.go | 48 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/assert/assertions.go b/assert/assertions.go index 166f63726..10cf845eb 100644 --- a/assert/assertions.go +++ b/assert/assertions.go @@ -1711,6 +1711,11 @@ func ErrorContains(t TestingT, theError error, contains string, msgAndArgs ...in func matchRegexp(rx interface{}, str interface{}) bool { var r *regexp.Regexp if rr, ok := rx.(*regexp.Regexp); ok { + // A typed nil passes the type assertion, and calling MatchString on it + // would dereference nil. There is no pattern, so nothing can match. + if rr == nil { + return false + } r = rr } else { r = regexp.MustCompile(fmt.Sprint(rx)) @@ -2009,6 +2014,18 @@ 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. + if condition == nil { + return Fail(t, "Condition must not be nil", msgAndArgs...) + } + + // The ticker is created unconditionally, but it is only ever read after the + // first result, and time.NewTicker panics on a non-positive interval. + if tick <= 0 { + return Fail(t, "Tick must be positive", msgAndArgs...) + } + ch := make(chan bool, 1) checkCond := func() { ch <- condition() } @@ -2104,6 +2121,18 @@ 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. + if condition == nil { + return Fail(t, "Condition must not be nil", msgAndArgs...) + } + + // The ticker is created unconditionally, but it is only ever read after the + // first result, and time.NewTicker panics on a non-positive interval. + if tick <= 0 { + return Fail(t, "Tick must be positive", msgAndArgs...) + } + var lastFinishedTickErrs []error ch := make(chan *CollectT, 1) @@ -2156,6 +2185,18 @@ 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. + if condition == nil { + return Fail(t, "Condition must not be nil", msgAndArgs...) + } + + // The ticker is created unconditionally, but it is only ever read after the + // first result, and time.NewTicker panics on a non-positive 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..8883a77b3 100644 --- a/assert/assertions_test.go +++ b/assert/assertions_test.go @@ -3634,6 +3634,54 @@ 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)) + }) + } +} + +// A typed nil *regexp.Regexp passes the type assertion in matchRegexp and was +// then dereferenced. +func TestRegexpNilTypedRegexp(t *testing.T) { + t.Parallel() + + var rx *regexp.Regexp + mockT := new(testing.T) + + NotPanics(t, func() { + False(t, Regexp(mockT, rx, "anything")) + }) + True(t, NotRegexp(t, rx, "anything")) +} + func Test_validateEqualArgs(t *testing.T) { t.Parallel() From 0b653404d1e950ce52e2974a9cb2bc1aaa7d10b1 Mon Sep 17 00:00:00 2001 From: feiiiiii5 Date: Sun, 27 Sep 2026 14:05:46 +0800 Subject: [PATCH 2/3] assert: correct two comments on the new Eventually/Never argument checks No behaviour change: stripping comments and blank lines from both revisions leaves them byte for byte identical, and the package's tests still pass. The tick comment said the ticker "is only ever read after the first result", which is true of the tickC channel but reads as though a bad interval would therefore be harmless until the first result. It is not: time.NewTicker is called before the condition has run at all and panics on a non-positive interval, in the caller's goroutine, which aborts the run rather than failing one assertion. The nil-condition comment explained the crash but not the choice. Never could plausibly be read as vacuously satisfied by a nil condition, returning true. It deliberately fails like Eventually does, so that one malformed argument behaves the same way in all three entry points; that is worth saying where the check is. --- assert/assertions.go | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/assert/assertions.go b/assert/assertions.go index 10cf845eb..28f2169e2 100644 --- a/assert/assertions.go +++ b/assert/assertions.go @@ -2016,12 +2016,19 @@ func Eventually(t TestingT, condition func() bool, waitFor time.Duration, tick t // 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 created unconditionally, but it is only ever read after the - // first result, and time.NewTicker panics on a non-positive interval. + // 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...) } @@ -2123,12 +2130,19 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time // 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 created unconditionally, but it is only ever read after the - // first result, and time.NewTicker panics on a non-positive interval. + // 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...) } @@ -2187,12 +2201,19 @@ func Never(t TestingT, condition func() bool, waitFor time.Duration, tick time.D // 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 created unconditionally, but it is only ever read after the - // first result, and time.NewTicker panics on a non-positive interval. + // 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...) } From b2c3eace6fd8b875c92fe7c1fb1cb7418d1b51f7 Mon Sep 17 00:00:00 2001 From: fei <204683769+feiiiiii5@users.noreply.github.com> Date: Tue, 29 Sep 2026 17:51:10 +0800 Subject: [PATCH 3/3] assert: move the typed-nil Regexp case out of this change The two causes are unrelated: one is a nil condition reaching a goroutine this package spawns, the other a typed nil arriving at a type assertion in matchRegexp. Splitting them so each can be reviewed on its own; the Regexp case moves to its own pull request. --- assert/assertions.go | 5 ----- assert/assertions_test.go | 14 -------------- 2 files changed, 19 deletions(-) diff --git a/assert/assertions.go b/assert/assertions.go index 28f2169e2..a97cd8cf5 100644 --- a/assert/assertions.go +++ b/assert/assertions.go @@ -1711,11 +1711,6 @@ func ErrorContains(t TestingT, theError error, contains string, msgAndArgs ...in func matchRegexp(rx interface{}, str interface{}) bool { var r *regexp.Regexp if rr, ok := rx.(*regexp.Regexp); ok { - // A typed nil passes the type assertion, and calling MatchString on it - // would dereference nil. There is no pattern, so nothing can match. - if rr == nil { - return false - } r = rr } else { r = regexp.MustCompile(fmt.Sprint(rx)) diff --git a/assert/assertions_test.go b/assert/assertions_test.go index 8883a77b3..f098a975e 100644 --- a/assert/assertions_test.go +++ b/assert/assertions_test.go @@ -3668,20 +3668,6 @@ func TestEventuallyNonPositiveTick(t *testing.T) { } } -// A typed nil *regexp.Regexp passes the type assertion in matchRegexp and was -// then dereferenced. -func TestRegexpNilTypedRegexp(t *testing.T) { - t.Parallel() - - var rx *regexp.Regexp - mockT := new(testing.T) - - NotPanics(t, func() { - False(t, Regexp(mockT, rx, "anything")) - }) - True(t, NotRegexp(t, rx, "anything")) -} - func Test_validateEqualArgs(t *testing.T) { t.Parallel()