Skip to content

Lint when t.Cleanup closes over t#3237

Open
jakebailey wants to merge 3 commits intomainfrom
jabaile/cleanup-t
Open

Lint when t.Cleanup closes over t#3237
jakebailey wants to merge 3 commits intomainfrom
jabaile/cleanup-t

Conversation

@jakebailey
Copy link
Copy Markdown
Member

If t is used in a cleanup, things break because the test has already ended. Add a lint rule to detect this and fix the one case.

Copilot AI review requested due to automatic review settings March 26, 2026 11:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new customlint analyzer to detect testing.T/testing.B/testing.TB variables being captured and used inside Cleanup closures (which can break because the test has already finished), and updates the one affected test case accordingly.

Changes:

  • Register a new cleanup analyzer in the customlint plugin.
  • Implement the analyzer to report identifier uses of captured testing variables within Cleanup function literals.
  • Add analyzer testdata + golden output, and adjust an LSP completion test to avoid capturing t in a cleanup closure.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/lsp/server_completion_test.go Updates cleanup usage to avoid closing over t.
_tools/customlint/plugin.go Registers the new cleanup analyzer in the plugin.
_tools/customlint/cleanup.go Implements the cleanup analyzer logic.
_tools/customlint/testdata/cleanup/cleanup.go Adds test cases that should/shouldn’t be flagged by the new analyzer.
_tools/customlint/testdata/cleanup/cleanup.go.golden Adds expected diagnostics output for the new analyzer.

DefaultLibraryPath: bundled.LibPath(),
}, onServerRequest)
t.Cleanup(func() { assert.NilError(t, closeClient()) })
t.Cleanup(func() { _ = closeClient() })
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closeClient() returns an error when the LSP server goroutines fail to shut down cleanly; ignoring it here can let test failures slip by silently. Consider handling the error without referencing t (e.g., fail-fast via panic or other non-testing.T reporting mechanism) so cleanup issues still cause the test to fail while satisfying the new lint rule.

Suggested change
t.Cleanup(func() { _ = closeClient() })
t.Cleanup(func() {
if err := closeClient(); err != nil {
panic(err)
}
})

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants