testing: lint against require.Eventually (or assert.Eventually)

[knz]

Describe the problem

There are two design problems in the require.Eventually / assert.Eventually code.

1. the function calls its condition in a separate goroutine. So if the condition function tries to call t.Fail (via t.Error etc) this violates the API of *testing.T.
2. it does not wait for the goroutine to complete before it returns. So if the condition doesn't return before the Eventually function gives up (e.g. because it gets stuck, or because it takes a longer time) and the Eventually logic fails, there will be a dangling goroutine.
3. Then also if the condition function eventually runs t.Fail after the Eventually call completes and also the surrounding test, the t.Fail call will occur after the test has terminated which is also an API violation.

To Reproduce

Example code that demonstrates the problems:

Problem 1:

require.Eventually(t, func() bool {
    require.True(t, false) // invalid use of t.Fail in separate goroutine
}, 3*time.Second, time.Second)

Problem 2:

require.Eventually(t, func() bool {
    time.Sleep(5 *time.Second) // will cause goroutine to dangle and potentially trigger leaktest
}, 3*time.Second, time.Second)

Problem 3:

require.Eventually(t, func() bool {
    time.Sleep(5 *time.Second)
    require.True(t, false) // fail after the test terminates
}, 3*time.Second, time.Second)

Expected behavior

We should disallow any use of require.Eventually outright until the bug is fixed in the require/assert library.

testutils.SucceedsSoon doesn't have this problem.

Jira issue: CRDB-26629

[blathers-crl]

cc @cockroachdb/test-eng

[srosenberg]

I wrote a quick & dirty semgrep rule to find the violations. It gets the majority but not all. There are a few more that are nested inside closures, etc. I'll try ruleguard to get an exhaustive list.

NO_COLOR=true semgrep --verbose --config ~/.semgrep/rules/100796.yml pkg >~/100796_findings.txt

100796_findings.txt

[srosenberg]

Here is another quick pass, this time using revive. The custom linter just looks for any occurrence of (require|assert).XXX inside the anonymous closure passed into Eventually.

The run below also requires a patch in [1].

./revive -config ./test.toml $GOPATH/src/github.com/cockroachdb/cockroach/pkg/...  |sed -E "s|/Users/srosenberg/workspace/go/src/github.com/cockroachdb/cockroach/||g"

This should be pretty close to an exhaustive list for auditing purposes,

potentially unsafe use of require.Equal inside Eventually at pkg/kv/db_test.go:708:5
potentially unsafe use of require.NoError inside Eventually at pkg/ccl/sqlproxyccl/proxy_handler_test.go:1083:4
potentially unsafe use of require.NoError inside Eventually at pkg/kv/kvprober/kvprober_integration_test.go:277:4
potentially unsafe use of require.NoError inside Eventually at pkg/cmd/roachtest/tests/mixed_version_change_replicas.go:216:5
potentially unsafe use of require.Equal inside Eventually at pkg/server/server_test.go:1066:3
potentially unsafe use of require.Equal inside Eventually at pkg/server/admin_test.go:3172:5
potentially unsafe use of require.NoError inside Eventually at pkg/kv/kvserver/intent_resolver_integration_test.go:396:5
potentially unsafe use of require.NotNil inside Eventually at pkg/kv/kvserver/replica_rangefeed_test.go:1070:4
potentially unsafe use of require.NoError inside Eventually at pkg/kv/kvserver/replicate_queue_test.go:1666:4
potentially unsafe use of require.Equal inside Eventually at pkg/kv/kvserver/replica_lease_renewal_test.go:104:5

[1] mgechev/revive#812

[knz]

Thanks for the linter.

NB: we should disallow Eventually regardless because of reason (2) - it leaks goroutines even if you don't use the assert/require functions inside the closure.

[srosenberg]

NB: we should disallow Eventually regardless because of reason (2) - it leaks goroutines even if you don't use the assert/require functions inside the closure.

Agreed. It's odd that Eventually executes the condition (closure) inside a new goroutine; definitely an unwanted side-effect.

[erikgrinaker]

Disallowing Eventually seems pretty heavy-handed here. Using t.Fatal in a goroutine is problematic in general, not just with Eventually, but using assert is fine afaik. A linter would be helpful to catch these cases. I much prefer Eventually over SucceedsSoon because I have better control over the timeout and frequency -- I often find myself having to manually reduce the SucceedsSoon timeout to something reasonable when iterating on tests locally.

As for the dangling goroutine, this can be handled by propagating a context to the closure that's cancelled when the test ends. Again, this is good practice in general, even outside of Eventually -- for example, many tests will simply use a bare channel read instead of a cancellable select, and if a test race/issue prevents the channel send then the test will hang indefinitely until the timeout triggers which is a failure mode that's hard to debug.

[knz]

We have testutils.SucceedsWithin which has a customizable timeout.