assert: report failures instead of panicking in Eventually and Never

[feiiiiii5]

Summary

Eventually, Never and EventuallyWithT panic instead of reporting a failure when condition is nil or tick is not positive, and Regexp/NotRegexp panic on a typed nil *regexp.Regexp; this guards all three.

Changes

• Eventually, Never, EventuallyWithT: Fail with a message when condition == nil or tick <= 0, before the ticker is built and before the goroutine is spawned.
• matchRegexp (used by Regexp and NotRegexp): a typed nil *regexp.Regexp is reported as no match instead of being dereferenced.
• Three regression tests, one per cause.

Motivation

A nil condition panics on a goroutine these functions spawn, so it is not something the caller can recover():

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x0]

goroutine 1 [running]:
github.com/stretchr/testify/assert.Eventually.func1()
	assert/assertions.go:2013
created by github.com/stretchr/testify/assert.Eventually in goroutine 1

One nil that reaches a helper takes the whole test binary down, with the stack trace pointing into testify rather than at the offending call. tick <= 0 reaches time.NewTicker and panics with non-positive interval for NewTicker. The ticker is only ever read after the first result (tickC starts nil, and is only assigned in the case v := <-ch branch), so a non-positive interval can never be useful — it is created unconditionally, before it can be known to be needed.

The nil-guard pattern is already in this file: isFunction, a few lines above matchRegexp, returns false for a nil argument instead of dereferencing it, which is why assert.Equal(t, nil, someFunc) reports a failure rather than crashing.

I went with a failure report rather than a panic with a better message, because that is what every other assertion in this package does. If you would rather tick <= 0 mean "check once, then wait for the timer" — which the existing tickC = nil mechanism could express without a ticker at all — that is a small change on top and I am happy to switch to it.

For the typed nil regexp I only added the crash guard, which means Regexp(t, (*regexp.Regexp)(nil), s) fails with a message that renders the nil pointer rather than saying "nil regexp". An explicit guard in the two public functions would read better; I left that out to keep the diff to the crash.

Tests fail on 87a7b9d by aborting the binary with exactly the trace above, and pass with the fix:

$ go test ./assert/ -run 'TestEventuallyNilCondition|TestEventuallyNonPositiveTick|TestRegexpNilTypedRegexp'
--- PASS: TestRegexpNilTypedRegexp (0.00s)
--- PASS: TestEventuallyNilCondition (0.00s)
--- PASS: TestEventuallyNonPositiveTick (0.00s)
ok  	github.com/stretchr/testify/assert	0.394s

Full suite green on both this branch and 87a7b9d (8 packages), and green here under -race. gofmt -l . and go vet ./... are clean.

Related issues

Refs #1970

I did not use Closes on purpose: that issue also asks whether nil guards are wanted for a separate family (Subset, ElementsMatch, IsIncreasing and friends, where Subset/NotSubset were reported in #1918 and closed without a comment). That question is still open and this PR should not close it.

[dolmen]

The nil Regexp case must be handled in a separate PR.

[feiiiiii5]

Split, as asked: b2c3eac takes the Regexp case out of #1971, and it is now its own pull request.

#1971 is left with only the nil condition and non-positive tick guards on Eventually, Never and EventuallyWithT — the two arguments that reach time.NewTicker and the goroutine this package spawns, where a panic cannot be recovered by the caller.

#1972 carries just matchRegexp: a typed nil passes the type assertion and MatchString then dereferences it, so Regexp and NotRegexp panicked instead of reporting a result. A typed nil is a *regexp.Regexp that was declared and never assigned, so there is no pattern behind it and nothing can match — false, which NotRegexp then inverts as usual.

I put the two in separate commits rather than rewriting the branch, so the split is visible in the history.

For the second one, the regression on 87a7b9d is a panic rather than a wrong value:

--- FAIL: TestRegexpNilTypedRegexp
        Error:  func (assert.PanicTestFunc)(0x10291e580) should not panic
               Panic value: runtime error: invalid memory address or nil pointer dereference

go build ./..., go vet ./assert/ and gofmt -l assert/ are clean, and go test ./assert/ -run TestRegexp -count=1 passes with the fix.

If the split is not what you had in mind — or if the second one is better as a third change on top of #1971 than as its own pull request — say so and I will rearrange it.

[feiiiiii5]

Correction: the split-off pull request is #1974, not #1972 as I wrote above. Its scope is exactly what I described there — only matchRegexp and its one test.

[feiiiiii5]

@dolmen done in b2c3eac — the matchRegexp case and its test are out of #1971, and that work is now #1974 on its own branch off master.