[test-parallel] test: add t.Parallel() to safe cli tests (batch) - #55506
Conversation
Analyzed 25 test files in pkg/cli round-robin batch (after status_dependency_tree_test.go). Added t.Parallel() to top-level tests and table-driven subtests confirmed safe: no os.Setenv/t.Setenv, os.Chdir, shared globals, fixed ports/paths, or external services. Files updated: - stdin_test.go: TestReadRunIDsFromStdin (+ subtests) - tool_graph_test.go: TestToolGraph, TestToolGraphMultipleSequences, TestToolGraphEmptySequences - trial_command_lipgloss_test.go: TestTrialConfirmationLipglossRendering (+ subtests), TestTrialConfirmationStructure, TestLipglossImportPresent, TestSectionCompositionPattern - trial_safe_output_errors_test.go: TestExtractSafeOutputErrors (+ subtests), TestWorkflowTrialResultSuccessField, TestAggregateTrialResults (+ subtests), TestSanitizeControlChars (+ subtests) - update_actions_content_refs_test.go: all top-level tests (deps/resolver-based, no shared state) - update_actions_workflow_files_test.go: both top-level tests (use t.TempDir, no cwd/env mutation) Remaining 18 files in the batch were left unchanged (unsafe: os.Chdir, t.Setenv, global state mutation, external tools/network, or TestMain). Validated with 'go test -race' on the modified test names and 'go test ./pkg/cli/...' (one pre-existing unrelated network-dependent failure in install_copilot_cli_test.go due to sandboxed environment lacking internet access).
github-actions
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot could not run the full agentic suite for this review because it was automatically requested on a bot-authored pull request. Request a review from Copilot under Reviewers to retry with the full agentic suite. Improved support for bot-authored pull requests is coming soon.
This PR parallelizes several Go test suites by adding t.Parallel() to top-level tests and some table-driven subtests to reduce overall test runtime.
Changes:
- Added
t.Parallel()to multiple top-level tests acrosspkg/cli. - Added
t.Parallel()inside severalt.Run(...)subtests in table-driven tests.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/update_actions_workflow_files_test.go | Marks workflow update tests as parallel. |
| pkg/cli/update_actions_content_refs_test.go | Marks action/skill/plugin ref update tests as parallel. |
| pkg/cli/trial_safe_output_errors_test.go | Marks trial-related tests and table-driven subtests as parallel. |
| pkg/cli/trial_command_lipgloss_test.go | Marks lipgloss rendering tests and subtests as parallel. |
| pkg/cli/tool_graph_test.go | Marks tool graph tests as parallel. |
| pkg/cli/stdin_test.go | Marks stdin parsing tests and table-driven subtests as parallel. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
| @@ -42,6 +43,7 @@ func TestExtractSafeOutputErrors(t *testing.T) { | |||
|
|
|||
| for _, tt := range tests { | |||
| @@ -65,6 +66,7 @@ func TestReadRunIDsFromStdin(t *testing.T) { | |||
|
|
|||
| for _, tt := range tests { | |||
| @@ -150,6 +151,7 @@ func TestTrialConfirmationLipglossRendering(t *testing.T) { | |||
|
|
|||
| for _, tt := range tests { | |||
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #55506 does not have the 'implementation' label and has only 28 new lines of code in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based 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
Non-blocking review: this patch only adds t.Parallel() to tests that stay in-memory or use isolated temp directories, and I don't see a changed-line correctness or concurrency regression worth blocking.
Highlights
- The table-driven subtests added to
stdin_test.go,trial_command_lipgloss_test.go, andtrial_safe_output_errors_test.gooperate on per-subtest local data with no shared mutable state. - The
update_actions_*tests construct fresh dependency stubs and caches inside each test, and the workflow-file tests uset.TempDir(), so the new parallelism does not introduce path collisions. - I discarded the requested sub-agent pass because
grumpy-coderis not available in this sandbox.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 4.26 AIC · ⌖ 6.89 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — approved. Changes are clean and well-reasoned.
📋 Review Summary
Positive Highlights
- ✅
t.Parallel()is added only to demonstrably pure, in-memory tests — the safety rationale in the PR description is sound for each file. - ✅ Top-level and table-driven subtests are both parallelised consistently, matching Go best practice.
- ✅
TestUpdateActionRefsInContent_CacheReusedAcrossLines— thecallCountvariable is local to the closure, so concurrent test execution poses no data-race risk. - ✅ The excluded files (using
os.Chdir,t.Setenv,SetVersionInfo, network/subprocess I/O) are correctly omitted; parallelising them would be unsafe. - ✅ The diff is purely additive (28 insertions, 0 deletions) with no logic or assertion changes.
No actionable issues found.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 16.2 AIC · ⌖ 9.8 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
The t.Parallel() additions are mechanically correct throughout: every table-driven test gets t.Parallel() at both the outer function and inside each t.Run subtest, and standalone tests each get a single top-level call. No shared mutable state is introduced. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 14.5 AIC · ⌖ 8.96 AIC · ⊞ 6.2K
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds
t.Parallel()calls to test functions across six test files inpkg/cli, enabling those tests to run concurrently. This is part of a batch effort to parallelize the safe CLI test suite for faster test execution. No production code or test logic was modified.Change Classification
Key Changes
t.Parallel()to test and subtest functionst.Parallel()to three test functionst.Parallel()to five test functionst.Parallel()to six test functionst.Parallel()to nine test functionst.Parallel()to two test functionsImpact Assessment
No downstream impact identified. Changes are limited to test files, adding
t.Parallel()markers to allow concurrent test execution within thepkg/clipackage. No test logic, assertions, or production code were altered.Commits