[test-parallel] test: add t.Parallel() to safe test cases in pkg/cli - #54184
Conversation
Analyzed 25 files (pkg/cli/logs_orchestrator_unit_test.go through pkg/cli/mcp_argument_validation_test.go) with per-file parallel-safety-checker sub-agents. Added t.Parallel() only to top-level tests and subtests confirmed safe: - logs_overview_test.go: 5 top-level tests, no shared state - logs_parsing_fallback_test.go: 3 top-level tests + 1 table-driven subtest set, uses t.TempDir() per test, no globals - logs_patch_test.go: 2 top-level tests, isolated temp dirs - logs_summary_file_test.go: 3 top-level tests, isolated temp dirs Files judged unsafe (env mutation, shared globals, os.Chdir, or unrebound loop-variable closures without go1.22 safety guarantees) were left unchanged: logs_orchestrator_unit_test.go, logs_output_hint_test.go, logs_parallel_test.go, logs_parse_test.go, logs_parsing_test.go, logs_rate_limit_test.go, logs_report_file_test.go, logs_report_test.go, logs_report_tools_isvalidtoolname_test.go, logs_run_processor_test.go, logs_safe_output_chains_test.go, logs_skill_activations_test.go, logs_summary_integration_test.go, logs_summary_test.go, logs_timeout_detection_test.go, logs_timeout_integration_test.go, logs_timeout_test.go, mcp_add_integration_test.go, mcp_add_test.go, mcp_argument_validation_test.go. logs_usage_activity_test.go already had t.Parallel() throughout. Validated with `go test -race ./pkg/cli/...` on the changed tests (all pass) and confirmed unrelated failures elsewhere in the suite (httptest port binding, TTY ANSI detection) are pre-existing sandbox limitations unrelated to this change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
changed the title
|
Compliant core-team agentic development PR: conservative scope, explicit per-file safety analysis before adding
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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 failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54184 does not have the 'implementation' label and has only 14 new lines of code in business logic directories (threshold: 100).
|
![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. All t.Parallel() additions follow correct Go parallelism conventions.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Table-driven subtest in
logs_parsing_fallback_test.gocorrectly adds both outer and innert.Parallel()calls — the idiomatic Go pattern - ✅ All modified tests use per-test isolation (
t.TempDir(),testutil.TempDir) rather than shared state — parallel execution is safe - ✅ PR description documents the safety analysis and pre-validates with
-race, which is the right bar for parallelism changes - ✅ Unchanged files (env mutation, global state) are correctly excluded from this PR
No actionable issues found.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 14.5 AIC · ⌖ 9.8 AIC · ⊞ 7.8K
Comment /matt to run again
There was a problem hiding this comment.
The changes look correct. All parallelized tests use t.TempDir() (parallel-safe), operate on isolated temporary directories or pure in-memory data, and share no mutable global state. The table-driven test in logs_parsing_fallback_test.go correctly adds t.Parallel() to both the parent and each subtest, following Go best practices. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.9 AIC · ⌖ 8.78 AIC · ⊞ 5.7K
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. I don't see a correctness or concurrency regression in the added t.Parallel() calls.
Highlights
- The changed tests are isolated to per-test temp dirs or pure in-memory struct checks.
pkg/cli/logs_patch_test.gousestestutil.TempDir, which allocates a unique directory per test and guards shared setup withsync.Once.- I did not find any new shared global mutation, env mutation, or fixed-path reuse introduced by this diff.
- The requested
grumpy-codersub-agent could not be launched because the binary is unavailable in this environment, so this review is based on an independent pass only.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 8.82 AIC · ⌖ 8.73 AIC · ⊞ 7K
Comment /review to run again
Test Quality Sentinel 🧪 — Infrastructure-Only ReportSummaryScore: N/A — Infrastructure Only This PR adds DetailsChanged Test Files
Infrastructure Quality ✅
ClassificationInfrastructure-Only PR — This PR improves test parallelization without adding or modifying test assertions. Behavioral test coverage remains unchanged. Recommendation✅ APPROVED — No violations detected. GoLeak infrastructure is in place. All build tags are correct. This is a pure infrastructure enhancement that improves test suite speed without compromising safety.
|
There was a problem hiding this comment.
Pull request overview
Adds safe parallel execution to isolated pkg/cli tests, reducing test-suite runtime.
Changes:
- Parallelizes 13 top-level tests.
- Parallelizes fallback-parser table subtests.
- Preserves filesystem isolation through per-test temporary directories.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_overview_test.go |
Parallelizes pure data-structure tests. |
pkg/cli/logs_parsing_fallback_test.go |
Parallelizes isolated parser tests and subtests. |
pkg/cli/logs_patch_test.go |
Parallelizes isolated artifact and command-help tests. |
pkg/cli/logs_summary_file_test.go |
Parallelizes summary-file tests using unique temporary directories. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
|
🎉 This pull request is included in a new release. Release: |
Summary
This PR adds
t.Parallel()calls to safe test cases across four test files inpkg/clito enable Go's parallel test execution. The changes are limited to inserting the directive into existing test functions and subtests; no test logic or assertions were modified.Change Classification
Key Changes
t.Parallel()to 5 test functionst.Parallel()to 4 test functions and 1 subtestt.Parallel()to 2 test functionst.Parallel()to 3 test functionsImpact Assessment
No downstream impact identified. This change only affects test execution concurrency in
pkg/cli; production code behavior is unchanged. Test suite runtime may decrease as these tests now execute in parallel.Commits