[test-parallel] Add t.Parallel() to safe top-level Go tests (batch 1/25) - #50206
Conversation
Adds t.Parallel() to test functions and table-driven subtests in: - cmd/gh-aw/argument_syntax_test.go - cmd/gh-aw/version_test.go - pkg/cli/actions_test.go - pkg/cli/add_description_test.go These tests use only local state (fresh command instances, t.TempDir, independently built binaries) with no shared globals, env vars, working-directory changes, or fixed resources, so they are safe to run in parallel. Verified with go build ./... and go test -race (both default and integration build tags) for cmd/gh-aw and pkg/cli. Skipped candidates in the same batch that mutate the shared global rootCmd/cobra command tree (help_flag_test.go, help_sections_order_test.go, main_help_text_test.go, short_description_test.go) after -race detected data races when adding t.Parallel() to short_description_test.go alongside pre-existing parallel tests touching rootCmd. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50206 does not have the 'implementation' label and has only 7 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds safe parallel execution to selected Go tests to reduce test runtime.
Changes:
- Parallelizes independent top-level tests and table-driven subtests.
- Preserves isolated temporary files and command instances.
Show a summary per file
| File | Description |
|---|---|
cmd/gh-aw/argument_syntax_test.go |
Parallelizes command syntax tests. |
cmd/gh-aw/version_test.go |
Parallelizes build and release-script checks. |
pkg/cli/actions_test.go |
Parallelizes environment conversion cases. |
pkg/cli/add_description_test.go |
Parallelizes workflow description tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| // Find the subcommand - setup step | ||
| foundCmd := findSubcommand(mcpCmd, tt.subcommand) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — changes look correct and safe to approve.
📋 Review Summary
What was reviewed
Four test files received mechanical t.Parallel() additions at both the top-level function and inner t.Run subtest level.
Safety assessment
argument_syntax_test.go: All subtests use freshly constructed*cobra.Commandinstances — no shared mutable state. ✅version_test.go: Subtests build independent binaries into isolated temp dirs or make read-only filesystem checks — no shared state. ✅actions_test.go:convertToGitHubActionsEnvis a pure function; no side effects. ✅add_description_test.go: File-based tests each use their owntestutil.TempDir— no cross-test interference. ✅
Positive highlights
- ✅ Excellent safety analysis in the PR description — skipped files that mutate
rootCmdand documented the rationale clearly. - ✅ Race-detector validation (
go test -race) confirmed before merging. - ✅ Consistent placement of
t.Parallel()as the very first statement in each function/subtest. - ✅ Pre-existing test failures correctly identified as unrelated to this change.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 17 AIC · ⌖ 7.89 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
The changes look correct. All t.Parallel() additions are restricted to tests with no shared mutable state — pure functions, fresh command instances, or isolated t.TempDir() paths. The author correctly excluded tests that mutate shared rootCmd/cobra globals and validated with -race. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.1 AIC · ⌖ 9.22 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — Infrastructure
📊 Infrastructure Signals
All modified files have correct Verdict
|
There was a problem hiding this comment.
One high-confidence blocking issue: TestArgumentNamingConventions was marked t.Parallel() while reading the same shared cobra command globals that this PR itself documents as racy elsewhere.
💡 Themes and rationale
- The PR correctly reverted
t.Parallel()onshort_description_test.goafter-racefound a data race againstcobra.(*Command).Commands()on shared globals (newCmd,removeCmd,enableCmd,disableCmd,compileCmd,runCmd). TestArgumentNamingConventionsinargument_syntax_test.go(part of this diff) reads the identical set of globals and calls.Commands()on them, now running in parallel alongside other newly-parallel tests in the same file/package. This is the same unsafe pattern that was intentionally avoided elsewhere, and should either be reverted for consistency or verified not to race with-racebefore merge.- The remaining
t.Parallel()additions (pure function tests, per-subtestt.TempDir(), independentexec.Commandinvocations) look safe and consistent with the stated safety rules.
Requesting changes to resolve the inconsistency on TestArgumentNamingConventions.
🔎 Code quality review by PR Code Quality Reviewer · auto · 60.5 AIC · ⌖ 5.09 AIC · ⊞ 7.9K
Comment /review to run again
|
|
||
| // TestArgumentNamingConventions verifies that argument names follow conventions | ||
| func TestArgumentNamingConventions(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
This test is marked t.Parallel() but calls cmd.Commands() on the same shared package-level cobra globals (newCmd, removeCmd, enableCmd, disableCmd, compileCmd, runCmd) that this PR's own description says caused a genuine -race failure when parallelized elsewhere in short_description_test.go — this is the same unguarded read pattern and needs the same treatment.
💡 Data race risk on shared cobra command tree
TestArgumentNamingConventions iterates commands = append(commands, cmd.Commands()...) over newCmd, removeCmd, enableCmd, disableCmd, compileCmd, runCmd — these are package-level var X = &cobra.Command{...} singletons defined in cmd/gh-aw/main.go, shared across the whole test binary.
The PR body explicitly states:
cmd/gh-aw/short_description_test.go(initially attempted, reverted after-racefound a genuine data race againstcobra.(*Command).Commands()shared with pre-existing parallel tests)
short_description_test.go reads the exact same six globals (newCmd, removeCmd, enableCmd, disableCmd, compileCmd, runCmd) and calls .Commands() on them — this is the identical unsafe pattern, just in a different file. If Commands() mutates internal cobra state (lazy sort/init on first call) concurrently with any other parallel test that also touches these globals (including TestArgumentSyntaxConsistency/TestMCPSubcommandArgumentSyntax/TestPRSubcommandArgumentSyntax in this same file, all now parallel), this can race.
Fix: either revert t.Parallel() on this specific test (matching the precedent set for short_description_test.go), or verify with go test -race -run TestArgumentNamingConventions ./cmd/gh-aw/... run together with the other parallel tests in the package to confirm no race, and document that verification in the PR.
|
🎉 This pull request is included in a new release. Release: |
Overview
Mechanical, low-risk change enabling parallel execution for safe top-level Go tests. Adds
t.Parallel()calls to test functions (and their subtests) that have no shared mutable state or ordering dependencies, reducing overall test suite runtime. This is batch 1 of 25 in a larger effort to parallelize the Go test suite.Files changed
cmd/gh-aw/argument_syntax_test.go—t.Parallel()added toTestArgumentSyntaxConsistency,TestMCPSubcommandArgumentSyntax,TestPRSubcommandArgumentSyntax,TestArgumentNamingConventions, plus theirt.Runsubtests.cmd/gh-aw/version_test.go—t.Parallel()added toTestVersionIsSetDuringBuild,TestBuildReleaseScriptExists, plus 5 subtests.pkg/cli/actions_test.go—t.Parallel()added toTestConvertToGitHubActionsEnvand its subtest.pkg/cli/add_description_test.go—t.Parallel()added toTestExtractWorkflowDescription,TestExtractWorkflowDescriptionFromFile,TestExtractWorkflowDescriptionFromFile_NonExistentFile, plus subtests.Impact
t.Parallel()markers.