[test-parallel] test: add t.Parallel() to safe table-driven subtests in pkg/cli - #56954
Conversation
Adds t.Parallel() to table-driven subtests (t.Run) across 10 test files in pkg/cli that were verified safe: they operate only on in-memory data, per-test t.TempDir()/temp files, or read-only fixture data, with no os.Setenv/os.Chdir, shared globals, or process-wide mutation. Top-level tests in these files already had t.Parallel(). Files updated: - experiments_grader_observations_test.go - firewall_log_test.go - firewall_policy_test.go - forecast_compliance_fixtures_formal_test.go - forecast_montecarlo_test.go - forecast_test.go - format_preservation_test.go - frontmatter_editor_test.go - gateway_logs_test.go - gateway_logs_timeline_rpcentry_test.go Validated with `go test -race` on affected tests and full `go test ./pkg/cli/...` (one unrelated pre-existing network-dependent failure in install_copilot_cli_test.go, untouched by this change). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ 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. ✅
|
|
Lean already. Ship. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
🏗️ ADR RequiredThis PR triggered the design-decision gate because it adds 101 new lines under a default business-logic directory ( Evidence reviewed
Gate resultI could not find an ADR on the PR body or branch that documents this PR's decision. Inferred decision from the diff
Proposed draft ADRBecause the PR branch could not be updated from this checkout state, here is the draft ADR content to add as # ADR-56954: Parallelize Safe CLI Subtests
**Date**: 2026-08-29
**Status**: Draft
**Deciders**: pelikhan, adr-writer agent
---
### Context
This pull request updates existing `pkg/cli` test files by adding `t.Parallel()` to table-driven subtests that appear to operate only on in-memory values, `t.TempDir()`-scoped files, or read-only fixtures. The PR description frames the work as part of a broader daily test parallelization effort, and the diff shows that excluded tests were left unchanged when they touched process-wide state such as environment variables, working directory changes, or shared stderr redirection. Although the production code is unchanged, the PR makes an explicit design choice about the repository's acceptable unit-test concurrency model in a business-logic package. Because the design gate was triggered by more than 100 added lines under `pkg/`, that choice needs to be documented.
### Decision
We will parallelize table-driven subtests in `pkg/cli` when they are demonstrably isolated from shared process-wide state and rely only on per-test temporary directories, immutable fixtures, or in-memory data. For this PR, that means adding `t.Parallel()` inside selected `t.Run(...)` blocks across existing CLI test files while intentionally leaving non-isolated tests serialized. We chose this approach because it improves test throughput without requiring production refactors, while preserving a conservative safety boundary for tests that mutate global state.
### Alternatives Considered
#### Alternative 1: Keep the Subtests Serialized
Retain the current execution model and avoid adding `t.Parallel()` to these `pkg/cli` subtests.
This was considered because serialized tests are simpler to reason about and avoid accidental flakes from hidden shared state. It was not chosen because the PR evidence shows a curated set of tests already isolated enough for concurrency, so keeping them serialized would preserve avoidable runtime cost without improving correctness.
#### Alternative 2: Parallelize More Aggressively Across All Test Cases
Apply `t.Parallel()` broadly to all subtests in the batch, including tests with environment mutation, directory changes, stderr reassignment, or shared package-level state.
This was considered because it could yield larger wall-clock speedups and reduce the need for case-by-case review. It was not chosen because the PR evidence explicitly calls out excluded unsafe patterns, and broad parallelization would increase the risk of nondeterministic failures and misleading test results.
### Consequences
#### Positive
- Safe `pkg/cli` subtests can run concurrently, reducing unnecessary serialization in the test suite.
- The repository gains a clearer convention that concurrency in tests is allowed when isolation is demonstrable.
- Future parallelization work can reuse the same safety criteria already illustrated by this PR.
#### Negative
- The tests now depend on the continued validity of assumptions about isolation, so later changes may introduce flakes if shared state is added carelessly.
- Reviewers and maintainers must spend effort checking whether newly parallelized tests still avoid global mutations.
- Some future refactors may need to remove `t.Parallel()` or split helpers when hidden coupling is discovered.
#### Neutral
- The decision changes test execution characteristics, not production runtime behavior.
- Unsafe subtests remain serialized, so the repository will continue to use a mixed concurrency model in the test suite.
- No new libraries, frameworks, or deployment architecture changes are introduced by this decision.
---
*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*Next actionPlease add this ADR to the PR branch and update the PR to reference it.
|
![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 — approving; no actionable issues found.
📋 Summary
Positive Highlights
- ✅ All
t.Parallel()calls are correctly placed as the first statement inside each subtest closure - ✅ Go 1.26 is in use — loop variable capture (
ttin range loops) is automatically handled by the compiler, so parallelising table-driven subtests is safe without manualtt := ttcopies - ✅ Safety analysis in the PR description is thorough and accurate: files with
os.Setenv,os.Chdir, globalos.Stderrreassignment, and package-level variable swaps are correctly excluded - ✅ Only subtests whose parent
Test*functions already callt.Parallel()at the top level are updated — correct pattern
No issues requiring changes.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 18.7 AIC · ⌖ 10.4 AIC · ⊞ 7.7K
Comment /matt to run again
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.
Verdict
No blocking issues in the changed lines.
Review notes
This PR only adds `t.Parallel()` to test subcases that appear to stay within per-test temp dirs, local value construction, and read-only fixtures. I did a changed-lines review for shared process-wide state hazards and did not find an actionable race or correctness regression in the modified hunks.The grumpy-coder sub-agent did not return any usable findings, so this verdict is based on the direct review only.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 6.29 AIC · ⌖ 7.4 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The changes correctly add t.Parallel() to table-driven subtests across pkg/cli. All tests use t.TempDir() for isolation (no shared mutable state), and the module uses Go 1.22+ where loop-variable capture is no longer an issue. No blocking concerns found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 19.7 AIC · ⌖ 10.5 AIC · ⊞ 6.2K
There was a problem hiding this comment.
🟢 Approval recommended
The parallelized subtests use independent or read-only state and introduce no identified concurrency hazards.
Pull request overview
Adds safe subtest parallelism across pkg/cli tests to reduce execution time without changing production behavior.
Changes:
- Adds
t.Parallel()to independent table-driven subtests. - Limits parallelization to read-only data, fixtures, and isolated temporary directories.
File summaries
| File | Description |
|---|---|
pkg/cli/experiments_grader_observations_test.go |
Parallelizes grader-related subtests. |
pkg/cli/firewall_log_test.go |
Parallelizes firewall log cases. |
pkg/cli/firewall_policy_test.go |
Parallelizes policy and artifact tests. |
pkg/cli/forecast_compliance_fixtures_formal_test.go |
Parallelizes fixture compliance cases. |
pkg/cli/forecast_montecarlo_test.go |
Parallelizes Monte Carlo and fixture cases. |
pkg/cli/forecast_test.go |
Parallelizes sample-limit cases. |
pkg/cli/format_preservation_test.go |
Parallelizes formatting assertions. |
pkg/cli/frontmatter_editor_test.go |
Parallelizes frontmatter editing cases. |
pkg/cli/gateway_logs_test.go |
Parallelizes gateway parsing cases. |
pkg/cli/gateway_logs_timeline_rpcentry_test.go |
Parallelizes RPC timeline conversion cases. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
🎉 This pull request is included in a new release. Release: |
test