[test-parallel] test: add t.Parallel() to safe tests in pkg/cli, pkg/colorwriter, pkg/console - #55558
Conversation
Add t.Parallel() to top-level and table-driven Go tests in pkg/cli, pkg/colorwriter, and pkg/console that were verified to be free of shared/global mutable state (no os.Setenv/t.Setenv, os.Chdir, or global stdout/stderr swaps). Tests using these unsafe patterns (e.g. TestWorkflowResolutionWindowsCompatibility, TestGetMarkdownWorkflowFilesExcludesREADME, TestHelperProcess, TestGolden_TableRendering, spinner/confirm/accessibility/print tests) were intentionally left unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
changed the title
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
![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 /codebase-design — changes are consistent with the project's parallelization pattern; approving with no blocking issues.
📋 Review Summary
What was checked
- All 112 additions are mechanical
t.Parallel()insertions — no logic changes. - Tests with
t.Runsubtests correctly receivet.Parallel()on the parent only; subtests are not forced parallel, which is the right conservative approach. - The exclusion list (tests using
os.Chdir,os.Setenv/t.Setenv, global stdio swaps) is accurate and matches the PR description. TestProgressBarConcurrencyalready tests concurrent updates internally; adding outert.Parallel()is safe sinceNewProgressBarcreates independent instances.
Positive Highlights
- ✅ Well-documented exclusion rationale in the PR description
- ✅
-racevalidation passed across all affected packages - ✅ No scope creep — diff is only
t.Parallel()lines, nothing else
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 25.6 AIC · ⌖ 13.7 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
The changes look correct. All added t.Parallel() calls are in tests that do not mutate shared global state (no os.Setenv, os.Chdir, or package-level shared variables). Subtest patterns are consistent and there are no loop-variable capture issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 26.3 AIC · ⌖ 9.04 AIC · ⊞ 6.2K
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
Non-blocking from a code-quality perspective: I didn't find a correctness or concurrency regression in the changed lines.
What I checked
- The diff only adds
t.Parallel()to top-level tests. - The tests left non-parallelized are the ones touching global process state (
os.Chdir, stdio swapping, env mutation), which is the main failure mode for this kind of change. - The parallelized tests I spot-checked operate on per-test temp dirs, local buffers, or injected tty/env hooks rather than shared mutable globals.
- I discarded sub-agent input because no result was available.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 7.07 AIC · ⌖ 8.05 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Adds t.Parallel() to tests verified as isolated, improving test-suite concurrency without changing production behavior.
Changes:
- Parallelizes safe CLI, colorwriter, and console tests.
- Leaves tests using global environment, streams, or working directories sequential.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/workflows_path_test.go |
Parallelizes isolated path tests. |
pkg/cli/workflows_test.go |
Parallelizes stateless filtering tests. |
pkg/cli/yaml_frontmatter_utils_test.go |
Parallelizes pure frontmatter tests. |
pkg/cli/yamllint_test.go |
Parallelizes isolated yamllint tests. |
pkg/colorwriter/spec_test.go |
Parallelizes public API specification tests. |
pkg/console/banner_test.go |
Parallelizes banner rendering tests. |
pkg/console/console_formatting_test.go |
Parallelizes formatting tests. |
pkg/console/format_test.go |
Parallelizes file-size formatting tests. |
pkg/console/golden_test.go |
Parallelizes independent golden tests. |
pkg/console/input_test.go |
Parallelizes non-TTY input tests. |
pkg/console/list_test.go |
Parallelizes isolated list tests. |
pkg/console/progress_test.go |
Parallelizes independent progress tests. |
pkg/console/prompt_form_test.go |
Parallelizes locally scoped form tests. |
pkg/console/render_formatting_test.go |
Parallelizes stateless value-formatting tests. |
pkg/console/render_slice_test.go |
Parallelizes isolated slice-rendering tests. |
pkg/console/render_test.go |
Parallelizes independent rendering tests. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 16/16 changed files
- Comments generated: 0
- Review effort level: Balanced
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (112 new lines in 📄 Draft ADR generated:
📄 Draft ADR: 55558-enable-parallel-go-test-execution.md# ADR-55558: Enable Parallel Go Test Execution via Systematic t.Parallel() Adoption
**Date**: 2026-08-25
**Status**: Draft
**Deciders**: Unknown
---
### Context
The Go test suite runs serially by default unless individual tests call `t.Parallel()`. As the test suite grows, serial execution becomes a bottleneck in CI. A round-robin automated workflow (`daily-go-test-parallelizer`) batches through test files, identifies top-level `Test...` functions that are safe for parallel execution, and adds `t.Parallel()` incrementally. "Safe" is defined as: no calls to `os.Setenv`/`t.Setenv`, no `os.Chdir`, no swap of global `os.Stdout`/`os.Stdin`/`os.Stderr`, and no reliance on shared/package-level mutable state. This ADR covers the decision to adopt this incremental parallelisation strategy across `pkg/cli`, `pkg/colorwriter`, and `pkg/console`.
### Decision
We will systematically enable parallel execution for Go tests by adding `t.Parallel()` to top-level test functions that are verified safe under a defined set of heuristics (no global-state mutation, no process-wide I/O redirection, no directory changes). The identification and application is automated via a daily workflow that processes files in lexicographic batches, allowing the rollout to proceed incrementally without blocking other work.
### Alternatives Considered
#### Alternative 1: Keep all tests sequential
All tests remain serial, relying on Go's default behaviour. No risk of inter-test interference. However, as the suite grows this significantly increases CI wall-clock time, making fast feedback loops harder to maintain. This was the status quo before this decision.
#### Alternative 2: Pass `-parallel N` at `go test` invocation without code changes
Setting `-parallel` at the CLI level allows the test binary to run up to N tests concurrently without touching source files. However, tests that share mutable global state or redirect os-level I/O would silently interfere, causing non-deterministic failures that are hard to diagnose. Explicit `t.Parallel()` in source code makes the intent visible, allows per-test granularity, and ensures the author has verified safety at the call site.
#### Alternative 3: Enable parallelism only via a build tag or environment variable
A compile-time or run-time flag could gate `t.Parallel()` so CI uses it but developers don't. This adds complexity (two code paths, conditional logic in tests) for marginal benefit; `t.Parallel()` in source is standard Go idiom and has no cost when tests are run sequentially.
### Consequences
#### Positive
- Faster CI feedback: parallel test execution reduces wall-clock time for packages where multiple independent tests previously waited on each other.
- Race-condition discovery: running tests in parallel surfaces hidden data races (especially with `-race`) that would be invisible in serial runs.
- Incremental and safe: the batch approach means each PR introduces a bounded set of changes that have been individually verified before merging.
#### Negative
- Tests incorrectly marked parallel could cause intermittent failures if global state is inadvertently shared; each batch requires careful human review before merging.
- Ongoing maintenance burden: newly added test functions must be evaluated for parallelism eligibility, and the automated workflow must continue to run to cover new tests.
#### Neutral
- Tests explicitly excluded from parallelisation (e.g. those using `os.Chdir` or swapping stdio) remain serial and are documented in each PR's "Intentionally left unchanged" list.
- The round-robin cursor is persisted to cache-memory so batches resume where they left off, decoupling progress from any single PR.
---
*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
🎉 This pull request is included in a new release. Release: |
Summary
This PR adds
t.Parallel()calls to safe, independent test functions acrosspkg/cli,pkg/colorwriter, andpkg/consoleto enable concurrent test execution and reduce overall test suite runtime. Changes are purely additive to existing test files and do not alter test logic, assertions, or production code. Generated automatically by the "Daily Go Test Parallelizer" workflow.Change Classification
Key Changes
t.Parallel()to 4 test functionst.Parallel()to 3 test functionst.Parallel()to 11 test functionst.Parallel()to 5 test functionst.Parallel()to 4 test functionst.Parallel()to 3 test functionst.Parallel()to 13 test functionst.Parallel()to 1 test functiont.Parallel()to 4 test functions (TestGolden_ErrorWithSuggestions, TestGolden_MessageFormatting, TestGolden_ProgressBarNonTTY, TestGolden_InfoSection)t.Parallel()to TestPromptSecretInputt.Parallel()to TestNewListItem, TestShowInteractiveList_EmptyItemst.Parallel()to 10 test functions (TestNewProgressBar, TestProgressBarUpdate, TestProgressBarMultipleUpdates, TestFormatBytes, TestProgressBarPercentageCalculation, TestProgressBarOutputFormat, TestProgressBarEdgeCases, TestProgressBarConcurrency, TestProgressBarNonTTYFallback, TestProgressBarModeSelection)t.Parallel()to 4 test functions (TestPromptWrappersReturnNonNilForms, TestPromptFormClearsCompletedQuestion, TestPromptFormDoesNotClearAccessibleOrNonTTYQuestion, TestIsCancelled)t.Parallel()to all test functionst.Parallel()to multiple test functionst.Parallel()to 23 test functions (TestRenderStruct_, TestRenderSlice_)Impact Assessment
Commits