x/tools/gopls/internal/test/integration: violates testing.T.FailNow precondition

[adonovan]

The documentation of testing.T says:

A test ends when its Test function returns or calls any of the methods FailNow, Fatal, Fatalf, SkipNow, Skip, or Skipf. Those methods, as well as the Parallel method, must be called only from the goroutine running the Test function.

However, this precondition is violated by gopls' integration test framework. The integration.Env captures the root testing.T, and uses it for many assertions (calls to t.Fatal) performed by the Env itself. But it is common for integration tests to create new subtests using t.Run and then call methods of Env that call Env.T.Fatal.

I don't know whether this is a real problem--perhaps the testing package must already be robust to this kind of misuse--but it does result in the testing package emitting messages like this one:

testing.go:1682: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test

(source: https://logs.chromium.org/logs/golang/buildbucket/cr-buildbucket/8720772441964325377/+/u/step/22/log/2)

I wonder whether this is something we need to be more careful about.

[gabyhelp]

Related Issues

• testing: invalid `testing.go:1343: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test` error #58129 (closed)
• testing: complain loudly during concurrent use of T.FatalX and T.SkipX #15758
• testing: detect t.FailNow() called from wrong goroutine #24678 (closed)
• all: T.FailNow used in goroutines in standard library tests #17900 (closed)
• testing: t.Failed() returns false during panic #49929
• testing: t.FailNow should panic if called from wrong goroutine #3800 (closed)
• testing: t.FailNow() in a deferred function masks test panic #29207
• proposal: testing: remove or improve T.FailNow #24680 (closed)
• testing: t.Fatal does not fatal tests if defers do not complete #22989 (closed)

Related Code Changes

• testing: explain how SkipNow and FailNow stop execution

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[findleyr]

Hmm, indeed.

This is, of course, why you're not supposed to close over testing.T. I think it would be reasonable to revert this decision: yes all helpers would have to accept a T, but especially with most new tests being marker tests, that's probably fine.

[adonovan]

all helpers would have to accept a T, but especially with most new tests being marker tests, that's probably fine.

I agree that would be a reasonable fix. I'm still not sure this is actually a problem in practice. (While studying the crash in the log above, I thought at first it might be relevant, but I'm not so sure now.)