chore: standardize on *_internal_test.go for white-box tests (#25601)

ethanndickson · web-flow · commit c650aabbef3e · 2026-05-22T20:24:38.000+10:00
My agent added `//nolint:testpackage` to a test file on one of my PRs.
Again. This PR cleans it up across the entire repo and updates the
in-repo conventions so future agents stop doing it.

The repo already has a precedent for white-box tests that need to touch
unexported symbols: `*_internal_test.go` (145+ existing files). The
`testpackage` linter's default `skip-regexp` exempts that filename
suffix, so the `//nolint:testpackage` directive is unnecessary in every
case where someone reached for it. This PR renames 51 such files to
`*_internal_test.go` via `git mv` so blame and history follow, and
strips the dead directive from 2 files that were already correctly named
(`coderd/oauth2provider/authorize_internal_test.go`,
`coderd/x/chatd/advisor_internal_test.go`).

`.claude/docs/TESTING.md` now documents the rule explicitly under *Test
Package Naming*, which is imported into the root `AGENTS.md` via
`@.claude/docs/TESTING.md`. The rule: prefer `package foo_test`; if you
need internal access, rename the file to `*_internal_test.go` rather
than adding a nolint directive.
diff --git a/.claude/docs/TESTING.md b/.claude/docs/TESTING.md
@@ -30,7 +30,16 @@ issues.
 
 ### Test Package Naming
 
-- **Test packages**: Use `package_test` naming (e.g., `identityprovider_test`) for black-box testing
+- **Black-box tests**: Default to a `package foo_test` test file (e.g.,
+  `identityprovider_test`). This is what the `testpackage` linter enforces.
+- **White-box / internal tests**: When a test needs to touch unexported
+  symbols, put it in a file named `*_internal_test.go` with `package foo`.
+  The `testpackage` linter's `skip-regexp` already exempts that filename
+  suffix, so no `//nolint:testpackage` directive is needed.
+- **Do not add `//nolint:testpackage`.** If a test needs internal access,
+  rename the file to `*_internal_test.go` instead. A directive plus a
+  justification comment is strictly worse than the established naming
+  convention, and the repo standardizes on the latter.
 
 ## RFC Protocol Testing
 
@@ -50,7 +59,7 @@ issues.
 
 ### Test File Structure
 
-```
+```text
 coderd/
 ├── oauth2.go                    # Implementation
 ├── oauth2_test.go              # Main tests
@@ -69,20 +78,20 @@ coderd/
 
 ### Running Tests
 
-| Command | Purpose |
-|---------|---------|
-| `make test` | Run all Go tests |
-| `make test RUN=TestFunctionName` | Run specific test |
-| `go test -v ./path/to/package -run TestFunctionName` | Run test with verbose output |
-| `make test-race` | Run tests with Go race detector |
-| `make test-e2e` | Run end-to-end tests |
+| Command                                              | Purpose                         |
+|------------------------------------------------------|---------------------------------|
+| `make test`                                          | Run all Go tests                |
+| `make test RUN=TestFunctionName`                     | Run specific test               |
+| `go test -v ./path/to/package -run TestFunctionName` | Run test with verbose output    |
+| `make test-race`                                     | Run tests with Go race detector |
+| `make test-e2e`                                      | Run end-to-end tests            |
 
 ### Frontend Testing
 
-| Command | Purpose |
-|---------|---------|
-| `pnpm test` | Run frontend tests |
-| `pnpm check` | Run code checks |
+| Command      | Purpose            |
+|--------------|--------------------|
+| `pnpm test`  | Run frontend tests |
+| `pnpm check` | Run code checks    |
 
 ## Common Testing Issues
 
@@ -218,6 +227,7 @@ func BenchmarkFunction(b *testing.B) {
 ```
 
 Run benchmarks with:
+
 ```bash
 go test -bench=. -benchmem ./package/path
 ```
diff --git a/agent/reconnectingpty/reconnectingpty_internal_test.go b/agent/reconnectingpty/reconnectingpty_internal_test.go
@@ -1,4 +1,3 @@
-//nolint:testpackage // Tests private env helpers directly.
 package reconnectingpty
 
 import (
diff --git a/agent/write_secret_files_internal_test.go b/agent/write_secret_files_internal_test.go
@@ -1,4 +1,4 @@
-package agent //nolint:testpackage // Exercises internal agent secrets handling.
+package agent
 
 import (
 	"testing"
diff --git a/aibridge/intercept/apidump/apidump_internal_test.go b/aibridge/intercept/apidump/apidump_internal_test.go
@@ -1,4 +1,4 @@
-package apidump //nolint:testpackage // tests unexported internals
+package apidump
 
 import (
 	"bytes"
diff --git a/aibridge/intercept/apidump/headers_internal_test.go b/aibridge/intercept/apidump/headers_internal_test.go
@@ -1,4 +1,4 @@
-package apidump //nolint:testpackage // tests unexported internals
+package apidump
 
 import (
 	"bytes"
diff --git a/aibridge/intercept/apidump/streaming_internal_test.go b/aibridge/intercept/apidump/streaming_internal_test.go
@@ -1,4 +1,4 @@
-package apidump //nolint:testpackage // shares test helpers with apidump_test.go
+package apidump
 
 import (
 	"bytes"
diff --git a/aibridge/intercept/chatcompletions/base_internal_test.go b/aibridge/intercept/chatcompletions/base_internal_test.go
@@ -1,4 +1,4 @@
-package chatcompletions //nolint:testpackage // tests unexported internals
+package chatcompletions
 
 import (
 	"context"
diff --git a/aibridge/intercept/chatcompletions/blocking_internal_test.go b/aibridge/intercept/chatcompletions/blocking_internal_test.go
@@ -1,4 +1,4 @@
-package chatcompletions //nolint:testpackage // tests unexported internals
+package chatcompletions
 
 import (
 	"context"
diff --git a/aibridge/intercept/chatcompletions/paramswrap_internal_test.go b/aibridge/intercept/chatcompletions/paramswrap_internal_test.go
@@ -1,4 +1,4 @@
-package chatcompletions //nolint:testpackage // tests unexported internals
+package chatcompletions
 
 import (
 	"fmt"
diff --git a/aibridge/intercept/chatcompletions/streaming_internal_test.go b/aibridge/intercept/chatcompletions/streaming_internal_test.go
@@ -1,4 +1,4 @@
-package chatcompletions //nolint:testpackage // tests unexported internals
+package chatcompletions
 
 import (
 	"io"
diff --git a/aibridge/intercept/messages/base_internal_test.go b/aibridge/intercept/messages/base_internal_test.go
@@ -1,4 +1,4 @@
-package messages //nolint:testpackage // tests unexported internals
+package messages
 
 import (
 	"context"
diff --git a/aibridge/intercept/messages/blocking_internal_test.go b/aibridge/intercept/messages/blocking_internal_test.go
@@ -1,4 +1,4 @@
-package messages //nolint:testpackage // tests unexported internals
+package messages
 
 import (
 	"io"
diff --git a/aibridge/intercept/messages/reqpayload_internal_test.go b/aibridge/intercept/messages/reqpayload_internal_test.go
@@ -1,4 +1,4 @@
-package messages //nolint:testpackage // tests unexported internals
+package messages
 
 import (
 	"testing"
diff --git a/aibridge/intercept/messages/streaming_internal_test.go b/aibridge/intercept/messages/streaming_internal_test.go
@@ -1,4 +1,4 @@
-package messages //nolint:testpackage // tests unexported internals
+package messages
 
 import (
 	"context"
diff --git a/aibridge/intercept/responses/base_internal_test.go b/aibridge/intercept/responses/base_internal_test.go
@@ -1,4 +1,4 @@
-package responses //nolint:testpackage // tests unexported internals
+package responses
 
 import (
 	"context"
diff --git a/aibridge/intercept/responses/blocking_internal_test.go b/aibridge/intercept/responses/blocking_internal_test.go
@@ -1,4 +1,4 @@
-package responses //nolint:testpackage // tests unexported internals
+package responses
 
 import (
 	"context"
diff --git a/aibridge/intercept/responses/reqpayload_internal_test.go b/aibridge/intercept/responses/reqpayload_internal_test.go
@@ -1,4 +1,4 @@
-package responses //nolint:testpackage // tests unexported internals
+package responses
 
 import (
 	"encoding/json"
diff --git a/aibridge/intercept/responses/streaming_internal_test.go b/aibridge/intercept/responses/streaming_internal_test.go
@@ -1,4 +1,4 @@
-package responses //nolint:testpackage // tests unexported internals
+package responses
 
 import (
 	"io"
diff --git a/aibridge/internal/integrationtest/apidump_internal_test.go b/aibridge/internal/integrationtest/apidump_internal_test.go
@@ -1,4 +1,4 @@
-package integrationtest //nolint:testpackage // tests unexported internals
+package integrationtest
 
 import (
 	"bufio"
diff --git a/aibridge/internal/integrationtest/bridge_internal_test.go b/aibridge/internal/integrationtest/bridge_internal_test.go
@@ -1,4 +1,4 @@
-package integrationtest //nolint:testpackage // tests unexported internals
+package integrationtest
 
 import (
 	"bytes"
diff --git a/aibridge/internal/integrationtest/circuit_breaker_internal_test.go b/aibridge/internal/integrationtest/circuit_breaker_internal_test.go
@@ -1,4 +1,4 @@
-package integrationtest //nolint:testpackage // tests unexported internals
+package integrationtest
 
 import (
 	"fmt"
diff --git a/aibridge/internal/integrationtest/keypool_failover_internal_test.go b/aibridge/internal/integrationtest/keypool_failover_internal_test.go
@@ -1,4 +1,4 @@
-package integrationtest //nolint:testpackage // tests unexported internals
+package integrationtest
 
 import (
 	"fmt"
diff --git a/aibridge/internal/integrationtest/metrics_internal_test.go b/aibridge/internal/integrationtest/metrics_internal_test.go
@@ -1,4 +1,4 @@
-package integrationtest //nolint:testpackage // tests unexported internals
+package integrationtest
 
 import (
 	"bytes"
diff --git a/aibridge/internal/integrationtest/responses_internal_test.go b/aibridge/internal/integrationtest/responses_internal_test.go
@@ -1,4 +1,4 @@
-package integrationtest //nolint:testpackage // tests unexported internals
+package integrationtest
 
 import (
 	"context"
diff --git a/aibridge/internal/integrationtest/trace_internal_test.go b/aibridge/internal/integrationtest/trace_internal_test.go
@@ -1,4 +1,4 @@
-package integrationtest //nolint:testpackage // tests unexported internals
+package integrationtest
 
 import (
 	"context"
diff --git a/aibridge/passthrough_internal_test.go b/aibridge/passthrough_internal_test.go
@@ -1,4 +1,4 @@
-package aibridge //nolint:testpackage // tests unexported newPassthroughRouter
+package aibridge
 
 import (
 	"crypto/tls"
diff --git a/aibridge/provider/anthropic_internal_test.go b/aibridge/provider/anthropic_internal_test.go
@@ -1,4 +1,4 @@
-package provider //nolint:testpackage // tests unexported internals
+package provider
 
 import (
 	"bytes"
diff --git a/aibridge/provider/copilot_internal_test.go b/aibridge/provider/copilot_internal_test.go
@@ -1,4 +1,4 @@
-package provider //nolint:testpackage // tests unexported internals
+package provider
 
 import (
 	"bytes"
diff --git a/aibridge/provider/openai_internal_test.go b/aibridge/provider/openai_internal_test.go
@@ -1,4 +1,4 @@
-package provider //nolint:testpackage // tests unexported internals
+package provider
 
 import (
 	"bytes"
diff --git a/coderd/oauth2provider/authorize_internal_test.go b/coderd/oauth2provider/authorize_internal_test.go
@@ -1,4 +1,3 @@
-//nolint:testpackage // Internal test for unexported hashOAuth2State helper.
 package oauth2provider
 
 import (
diff --git a/coderd/templates_meta_update_internal_test.go b/coderd/templates_meta_update_internal_test.go
@@ -1,4 +1,4 @@
-package coderd //nolint:testpackage // Tests the unexported resolveTemplateMetaUpdate helper.
+package coderd
 
 import (
 	"reflect"
diff --git a/coderd/x/chatd/advisor_internal_test.go b/coderd/x/chatd/advisor_internal_test.go
@@ -1,4 +1,4 @@
-package chatd //nolint:testpackage // Accesses unexported advisor helpers.
+package chatd
 
 import (
 	"context"
diff --git a/coderd/x/chatd/attachments_internal_test.go b/coderd/x/chatd/attachments_internal_test.go
@@ -1,4 +1,4 @@
-package chatd //nolint:testpackage
+package chatd
 
 import (
 	"context"
diff --git a/coderd/x/chatd/chatdebug/model_coverage_internal_test.go b/coderd/x/chatd/chatdebug/model_coverage_internal_test.go
@@ -1,4 +1,4 @@
-package chatdebug //nolint:testpackage // Checks unexported normalized structs against fantasy source types.
+package chatdebug
 
 import (
 	"reflect"
diff --git a/coderd/x/chatd/chatdebug/model_normalization_internal_test.go b/coderd/x/chatd/chatdebug/model_normalization_internal_test.go
@@ -1,4 +1,4 @@
-package chatdebug //nolint:testpackage // Uses unexported normalization helpers.
+package chatdebug
 
 import (
 	"context"
diff --git a/coderd/x/chatd/chatdebug/recorder_internal_test.go b/coderd/x/chatd/chatdebug/recorder_internal_test.go
@@ -1,4 +1,4 @@
-package chatdebug //nolint:testpackage // Uses unexported recorder helpers.
+package chatdebug
 
 import (
 	"context"
diff --git a/coderd/x/chatd/chatdebug/reuse_step_internal_test.go b/coderd/x/chatd/chatdebug/reuse_step_internal_test.go
@@ -1,4 +1,4 @@
-package chatdebug //nolint:testpackage // Uses unexported recorder helpers.
+package chatdebug
 
 import (
 	"context"
diff --git a/coderd/x/chatd/chatdebug/transport_internal_test.go b/coderd/x/chatd/chatdebug/transport_internal_test.go
@@ -1,4 +1,4 @@
-package chatdebug //nolint:testpackage // Uses unexported recorder helpers.
+package chatdebug
 
 import (
 	"bytes"
diff --git a/coderd/x/chatd/chatloop/chatloop_run_internal_test.go b/coderd/x/chatd/chatloop/chatloop_run_internal_test.go
@@ -1,4 +1,4 @@
-package chatloop //nolint:testpackage // Uses internal symbols.
+package chatloop
 
 import (
 	"context"
diff --git a/coderd/x/chatd/chatloop/compaction_internal_test.go b/coderd/x/chatd/chatloop/compaction_internal_test.go
@@ -1,4 +1,4 @@
-package chatloop //nolint:testpackage // Uses internal symbols.
+package chatloop
 
 import (
 	"context"
diff --git a/coderd/x/chatd/chattool/askuserquestion_internal_test.go b/coderd/x/chatd/chattool/askuserquestion_internal_test.go
@@ -1,4 +1,4 @@
-package chattool //nolint:testpackage // Uses internal symbols.
+package chattool
 
 import (
 	"testing"
diff --git a/coderd/x/chatd/chattool/createworkspace_internal_test.go b/coderd/x/chatd/chattool/createworkspace_internal_test.go
@@ -1,4 +1,4 @@
-package chattool //nolint:testpackage // Uses internal symbols.
+package chattool
 
 import (
 	"context"
diff --git a/coderd/x/chatd/configcache_internal_test.go b/coderd/x/chatd/configcache_internal_test.go
@@ -1,4 +1,4 @@
-package chatd //nolint:testpackage // Uses internal cache state.
+package chatd
 
 import (
 	"context"
diff --git a/coderd/x/chatd/dialvalidation_internal_test.go b/coderd/x/chatd/dialvalidation_internal_test.go
@@ -1,4 +1,4 @@
-package chatd //nolint:testpackage // Uses internal symbols.
+package chatd
 
 import (
 	"context"
diff --git a/coderd/x/chatd/instruction_internal_test.go b/coderd/x/chatd/instruction_internal_test.go
@@ -1,4 +1,4 @@
-package chatd //nolint:testpackage // Uses internal symbols.
+package chatd
 
 import (
 	"encoding/json"
diff --git a/coderd/x/chatd/quickgen_internal_test.go b/coderd/x/chatd/quickgen_internal_test.go
@@ -1,4 +1,4 @@
-package chatd //nolint:testpackage // Keeps internal helper tests in-package.
+package chatd
 
 import (
 	"context"
diff --git a/coderd/x/chatd/store_chat_attachment_internal_test.go b/coderd/x/chatd/store_chat_attachment_internal_test.go
@@ -1,4 +1,4 @@
-package chatd //nolint:testpackage
+package chatd
 
 import (
 	"context"
diff --git a/coderd/x/chatd/title_override_internal_test.go b/coderd/x/chatd/title_override_internal_test.go
@@ -1,4 +1,4 @@
-package chatd //nolint:testpackage // Tests internal title override helpers.
+package chatd
 
 import (
 	"context"
diff --git a/coderd/x/chatd/usagelimit_internal_test.go b/coderd/x/chatd/usagelimit_internal_test.go
@@ -1,4 +1,4 @@
-package chatd //nolint:testpackage // Keeps chatd unit tests in the package.
+package chatd
 
 import (
 	"testing"
diff --git a/codersdk/pagination_internal_test.go b/codersdk/pagination_internal_test.go
@@ -1,4 +1,3 @@
-//nolint:testpackage
 package codersdk
 
 import (
diff --git a/codersdk/workspacesdk/agentconn_internal_test.go b/codersdk/workspacesdk/agentconn_internal_test.go
@@ -1,4 +1,3 @@
-//nolint:testpackage // This test exercises the internal query builder directly because agent requests need a live tailnet connection.
 package workspacesdk
 
 import (
diff --git a/provisionerd/runner/runner_internal_test.go b/provisionerd/runner/runner_internal_test.go
@@ -1,4 +1,4 @@
-package runner //nolint:testpackage // Tests unexported failure classification helpers.
+package runner
 
 import (
 	"testing"

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,3 @@`
`1`		`-//nolint:testpackage // Tests private env helpers directly.`
`2`	`1`	`package reconnectingpty`
`3`	`2`
`4`	`3`	`import (`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-package agent //nolint:testpackage // Exercises internal agent secrets handling.`
	`1`	`+package agent`
`2`	`2`
`3`	`3`	`import (`
`4`	`4`	`"testing"`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-package apidump //nolint:testpackage // tests unexported internals`
	`1`	`+package apidump`
`2`	`2`
`3`	`3`	`import (`
`4`	`4`	`"bytes"`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-package chatcompletions //nolint:testpackage // tests unexported internals`
	`1`	`+package chatcompletions`
`2`	`2`
`3`	`3`	`import (`
`4`	`4`	`"context"`