Disable nonsensical usetesting context linting rule

[marten-seemann]

usetesting enforces the use of t.Context everywhere, even in t.Cleanup. This is incorrect, as that context is cancelled just before Cleanup is called.

Furthermore, in many cases, context.Background is used like this:

func TestSomething(t *testing.T) {
     err := doSomething(context.Background())
     require.NoError(t, err)
}

Here it doesn't make any difference if context.Background or t.Context is used:

• In both cases, if doSomething doesn't block indefinitely, it will return eventually.
• In both cases, if doSomething blocks longer than the test timeout, go test will fail.

There are a few situations where t.Context is useful:

• Saving one LOC when using a context.WithCancel.
• When using a context in a Goroutine.

Also see https://github.com/smallstep/bouncer/pull/260.

[hslatman]

I'm still inclined to say that using t.Context() is fine to use generally where context.Background() is used currently. Instead of sprinkling context.Background() (and/or context.TODO()) all over the place, the default would now be t.Context() in tests.

Yes, there are cases where it must not be used, but that forces one to think about proper use of the ctx. In fact, one would already need to think about proper use of independent contexts in cleanup before, which now becomes even clearer when it's done (no two context.Background() calls, but t.Context() and context.Background() or context.WithoutCancel(t.Context()), for example). and it even could invite/lead to improvements to handling the ctx in application code.

[marten-seemann]

1. I did some more digging on the Go issue: testing: reconsider adding Context method to testing.T golang/go#36532. t.Context was never intended as the default context to use. Notably, Russ Cox even argued that t.Context has so few use cases that it shouldn't even be added to the standard library: testing: reconsider adding Context method to testing.T golang/go#36532 (comment).

2. Looking at the current master of the standard library itself, it seems like the Go team regards t.Context as a tool that's applicable in a limited number of cases, but not as a default:

❯ rg --type go "t\.Context" | grep -v context.Context | grep -v Request.Context  | grep -v st.Context | grep -v "//"
src/database/sql/sql_test.go:	ctx := t.Context()
src/database/sql/sql_test.go:	ctx := t.Context()
src/database/sql/sql_test.go:			ctx := t.Context()
src/database/sql/sql_test.go:	ctx := t.Context()
src/database/sql/sql_test.go:	ctx := t.Context()
src/database/sql/sql_test.go:	ctx := t.Context()
src/database/sql/sql_test.go:	ctx := t.Context()
src/database/sql/sql_test.go:	ctx := t.Context()
src/database/sql/sql_test.go:	ctx := t.Context()
src/database/sql/sql_test.go:	ctx := t.Context()
src/runtime/semasleep_test.go:		case <-t.Context().Done():
src/syscall/syscall_linux_test.go:	cmd := testenv.CommandContext(t, t.Context(), exe, arg, "-test.v")
src/syscall/syscall_linux_test.go:	cmd := testenv.CommandContext(t, t.Context(), exe, arg, "-test.v")
src/testing/synctest/synctest_test.go:			<-t.Context().Done()
src/testing/synctest/example_test.go:		ctx, cancel := context.WithCancel(t.Context())
src/testing/synctest/example_test.go:		ctx, cancel := context.WithTimeout(t.Context(), timeout)
src/testing/testing_test.go:	ctx := t.Context()
src/testing/testing_test.go:		innerCtx = t.Context()
src/log/slog/handler_test.go:			err := test.h.Handle(t.Context(), r)
src/crypto/internal/fips140cache/cache_test.go:	ctx, cancel := context.WithCancel(t.Context())

This is the complete list of occurrences of t.Context. On the other hand, context.Background is used extensively:

❯ rg --glob '*_test.go' "context.Background()" | wc -l
     324

3. The author of the usetesting linter just disabled the context linters (fix: disable context-related checks by default ldez/usetesting#11), after he was made aware of the huge number of false positives. This will be shipped in the next golangci-lint release. So all my PR here does now is enable the behavior that will soon become the default :)

[hslatman]

1. I did some more digging on the Go issue: testing: reconsider adding Context method to testing.T golang/go#36532. t.Context was never intended as the default context to use. Notably, Russ Cox even argued that t.Context has so few use cases that it shouldn't even be added to the standard library: testing: reconsider adding Context method to testing.T golang/go#36532 (comment).

And yet there are many proponents, the proposal was accepted, and it reduces some boilerplate.

2. Looking at the current master of the standard library itself, it seems like the Go team regards `t.Context` as a tool that's applicable in a limited number of cases, but not as a default:

It's relatively new, and would require touching many files, potentially blocking other PRs or at least causing more work. It can happen over time, package by package. Time will tell.

3. The author of the `usetesting` linter just disabled the context linters ([fix: disable context-related checks by default ldez/usetesting#11](https://github.com/ldez/usetesting/pull/11)), after he was made aware of the huge number of false positives. This will be shipped in the next golangci-lint release. So all my PR here does now is enable the behavior that will soon become the default :)

Marking the "92% of cases" as false positive is subjective, imo. The t.Context() is a single context.Background() for the test case, which is perfectly fine to use in a unit test by default. We're not supposed to just create new contexts using context.Background in application code; why would we need to do that in tests?

Anyhow, it's fine that the default for the linter will be changed. It'll end up being a matter of code review, and personal preference then.

[marten-seemann]

Given that the default is going to change with the next golangci-lint update, can we get this PR merged please, before this linter introduces more t.Cleanup bugs and creates even more churn across our code base?

[hslatman]

I disagree with disabling context.TODO.

[marten-seemann]

Yeah, context.TODO is a weird one, and I’m not a fan of the decision to include it in the stdlib. In most cases, I consider it a red flag, both in testing and in production code.