[test-parallel] test: add t.Parallel() to safe top-level tests in pkg/cli (batch) - #55472
Conversation
Adds t.Parallel() to 103 top-level test functions across 25 files in pkg/cli, selected round-robin from the tracked test file list. Each candidate was vetted for process-global state mutation (os.Setenv, os.Chdir, os.Stdout/Stderr capture), shared mutable globals, fixed ports/paths, and unsafe loop-variable capture; unsafe tests within mixed files were left unchanged. Validated with 'go test -race ./pkg/cli/...' and 'go test ./...'. The only failures observed are pre-existing and unrelated to this change (network-dependent TestInstallCopilotCLIScriptRootless... in pkg/cli, and TestGitPatchFromHEADCommits/TestMaskOTLP*Script in pkg/workflow), confirmed to fail identically on the base branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #55472 and found no actionable changed-line issues; no review comments were posted and the overall assessment is non-blocking.
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check.
|
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.
|
There was a problem hiding this comment.
The patch is mostly mechanical test churn, and the blanket t.Parallel() additions look like unnecessary complexity for this suite. I’d keep the tests serial unless a specific case is proven to need isolation. net: -1 lines possible.
Generated by ✂️ Ponytail Reviewer for #55472 · codex · mai10 · 5.06 AIC · ⌖ 1.68 AIC · ⊞ 16.7K
Comment /ponytail to run again
| } | ||
|
|
||
| func TestTrustedActivationGatedJobs(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
pkg/cli/runner_guard_activation_gate_test.go:L95: yagni: blanket t.Parallel() on every test in this file. Keep tests serial unless you have a proven need for parallelism.
There was a problem hiding this comment.
Reviewed with Impeccable audit mode (tests-only change).
All added t.Parallel() calls are at top-level test functions. Each test that touches the filesystem uses t.TempDir() for isolation, there are no os.Setenv/os.Chdir calls, and no shared mutable global state was introduced. The parallelisation is safe and the change is straightforward. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 21.3 AIC · ⌖ 8.99 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — approving; changes are mechanically correct and well-scoped.
📋 Key Themes & Highlights
Key Themes
- Mechanical parallelisation — 103
t.Parallel()additions across 25 files; no logic changes - Safety criteria respected — files with
t.Setenv,os.Chdir, or shared mutable globals were explicitly excluded or had only vetted-safe subset parallelised - Race detector validation —
go test -race ./pkg/cli/...passes; one pre-existing unrelated failure documented
Positive Highlights
- ✅ Round-robin batch approach keeps PRs reviewable and cache state advanced correctly
- ✅ Subtests inside parallel top-level tests do not incorrectly inherit
t.Parallel()— parent parallelism is sufficient for table-driven tests - ✅ No loop-variable rebinding issues (Go 1.22+ semantics noted and applied correctly)
- ✅ Pre-existing failures are clearly documented and not caused by this batch
No actionable issues found. Clean batch.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 28.8 AIC · ⌖ 9.67 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
Pull request overview
Adds safe top-level test parallelism across pkg/cli to reduce test-suite runtime without changing production behavior.
Changes:
- Adds
t.Parallel()to 103 isolated or read-only tests. - Leaves tests using process-wide state sequential.
- Covers parsing, security, setup, status, runner-guard, and utility tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/runner_guard_activation_gate_test.go |
Parallelizes isolated activation-gate tests. |
pkg/cli/runner_guard_copilot_allow_tool_test.go |
Parallelizes Copilot allow-tool filtering tests. |
pkg/cli/runner_guard_gvisor_exfiltration_test.go |
Parallelizes gVisor filtering tests. |
pkg/cli/runner_guard_inline_ignore_test.go |
Parallelizes inline-ignore filtering tests. |
pkg/cli/runner_guard_test.go |
Parallelizes path and argument tests. |
pkg/cli/secret_set_command_test.go |
Parallelizes encryption and client-option tests. |
pkg/cli/secrets_command_test.go |
Parallelizes command-construction tests. |
pkg/cli/secrets_test.go |
Parallelizes secret parsing tests. |
pkg/cli/security_regression_test.go |
Parallelizes isolated security regression tests. |
pkg/cli/semver_precise_test.go |
Parallelizes precise-version tests. |
pkg/cli/semver_test.go |
Parallelizes semantic-version tests. |
pkg/cli/setup_cli_action_integration_test.go |
Parallelizes read-only action integration tests. |
pkg/cli/setup_command_test.go |
Parallelizes isolated setup tests. |
pkg/cli/shell_completion_test.go |
Parallelizes pure shell helper tests. |
pkg/cli/shellcheck_test.go |
Parallelizes isolated shellcheck helpers. |
pkg/cli/signal_aware_poll_test.go |
Parallelizes independent polling tests. |
pkg/cli/smoke_copilot_aoai_workflow_contract_test.go |
Parallelizes read-only contract validation. |
pkg/cli/spec_github_url_test.go |
Parallelizes URL parsing tests. |
pkg/cli/spec_manifest_source_test.go |
Parallelizes manifest source tests. |
pkg/cli/spec_test.go |
Parallelizes read-only public API tests. |
pkg/cli/staged_filtering_test.go |
Parallelizes temporary-file parsing tests. |
pkg/cli/stats_helpers_test.go |
Parallelizes percentage helper tests. |
pkg/cli/status_command_test.go |
Parallelizes serialization and rendering tests. |
pkg/cli/status_compiled_test.go |
Parallelizes isolated compilation-status tests. |
pkg/cli/status_dependency_tree_test.go |
Parallelizes dependency-tree tests. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 25/25 changed files
- Comments generated: 0
- Review effort level: Balanced
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds
t.Parallel()to top-level test functions across 25 Go test files inpkg/cli, selected round-robin (next batch afterpkg/cli/run_workflow_validation_test.goin lexicographic order of tracked*_test.gofiles with top-levelTestfunctions).Files changed (25)
103 top-level
Test...functions were marked parallel-safe and updated (Go 1.22+ semantics, no loop-variable rebinding added).Safety criteria applied
Excluded from parallelization (left unchanged) any test that:
t.Setenv,os.Setenv,os.Chdir, or other process-wide stateos.Stdout/os.StderrgloballyFiles with a mix of safe and unsafe tests (e.g.
runner_guard_test.go,secret_set_command_test.go,secrets_test.go,security_regression_test.go,setup_command_test.go,shell_completion_test.go,shellcheck_test.go,spec_test.go,status_command_test.go) only hadt.Parallel()added to the vetted-safe subset; unsafe tests in the same file were left untouched.Validation
go build ./pkg/cli/...— passesgo vet ./pkg/cli/...— passesgo test -race ./pkg/cli/...— passes except one pre-existing, unrelated, network-dependent failure (TestInstallCopilotCLIScriptRootlessModeUsesRealScriptWithToolcacheAndNoSudo, fails identically onmaindue to a 404 against a real release download URL; not touched by this batch)go test ./...— passes except the same pre-existing pkg/cli failure plus pre-existing, unrelated failures inpkg/workflow(TestGitPatchFromHEADCommits,TestMaskOTLPHeadersScript,TestMaskOTLPAttributesScript) — none of these files are part of this batchNext run
Cache state advanced to
pkg/cli/status_dependency_tree_test.goso the next daily run continues round-robin from there.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.