Uh oh!
There was an error while loading. Please reload this page.
[test-parallel] Add t.Parallel() to safe Go tests in pkg/cli (daily batch) - #54144
Conversation
Analyzed batch of 25 test files (round-robin from cache-memory state, starting after pkg/cli/logs_empty_runs_test.go) using parallel-safety-checker sub-agents. Added t.Parallel() only to top-level test functions confirmed safe: no t.Setenv/os.Setenv/os.Chdir, no shared mutable globals, no fixed ports/paths outside per-test temp dirs, and no ordering dependencies. Files updated: - logs_engine_filter_test.go - logs_extract_zip_test.go - logs_firewall_filter_test.go - logs_formatting_test.go - logs_github_rate_limit_usage_test.go - logs_json_test.go - logs_mcp_tool_usage_test.go - logs_missing_data_test.go - logs_missing_tool_test.go - logs_noop_test.go Table-driven subtests were intentionally left unparallelized where they capture the loop variable without rebinding, or share temp files/dirs across iterations (would need production/test refactor beyond scope). Files judged unsafe (t.Setenv, os.Stdout/Stderr redirection, shared package-level stubs, httptest fixed ports, timing-dependent behavior, or insufficient visibility into called production code) were left unchanged: logs_episode_test.go, logs_fallback_integration_test.go, logs_filtering_test.go, logs_firewall_parse_test.go, logs_flatten_test.go, logs_github_api_test.go, logs_json_clean_test.go, logs_json_stderr_order_test.go, logs_mcp_failure_test.go, logs_missing_tool_integration_test.go, logs_orchestrator_filters_test.go, logs_orchestrator_pagination_test.go, logs_orchestrator_test.go. Validation: go build ./pkg/cli/...; go test -race with -run targeting all modified top-level tests (all passed); go test ./pkg/cli/... full run shows only pre-existing, unrelated sandbox failures (httptest port binding, TTY color detection) reproduced identically on the unmodified tree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
🧠 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
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54144 does not have the 'implementation' label and has only 53 new lines of code in business logic directories (threshold: 100).
|
❌ 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.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — approving; the parallelization is safe and well-reasoned.
📋 Assessment
Positive Highlights
- ✅ Safety exclusion list is thorough and clearly documented (
t.Setenv,os.Stdout/os.Stderrredirection, shared stubs, fixed ports, timing-dependent tests all correctly left out) - ✅ Each
t.Parallel()is placed as the very first statement in the top-level test function — correct Go convention - ✅
t.TempDir()usage throughout the zip-extraction tests ensures parallel-safe filesystem isolation - ✅ Validation (
go test -race) was run on all modified tests before merging - ✅ Table-driven subtests intentionally left unparallelized where loop-variable rebinding would be required — avoids silent data-race introduction
No actionable issues
All t.Parallel() additions are in functions that are stateless, use only t.TempDir() for filesystem access, and carry no shared mutable globals. The PR scope is strictly additive (53 lines, 0 deletions), making regression risk minimal.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.6 AIC · ⌖ 9.85 AIC · ⊞ 7.8K
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.
The changes are straightforward and safe: only t.Parallel() calls added to top-level test functions. All shared state uses t.TempDir() (parallel-safe). No os.Setenv, os.Chdir, or other global state mutations detected. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.7 AIC · ⌖ 8.79 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Verdict
No blocking issues found in the changed lines.
Highlights
I checked the added t.Parallel() calls against the affected tests’ fixture patterns and did not find a changed-line correctness or isolation bug worth blocking on. The touched tests use per-test temp dirs or purely local data, and the author explicitly left riskier cases (env mutation, stdio redirection, fixed-port/timing-sensitive tests) untouched.
I also attempted to collect advisory findings from grumpy-coder, but no output was available, so this review is based on my own pass only.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 11 AIC · ⌖ 4.85 AIC · ⊞ 4K
Comment /review 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 runtime without changing assertions or production behavior.
Changes:
- Adds
t.Parallel()to independently isolated tests. - Retains sequential execution for table-driven subtests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_noop_test.go | Parallelizes noop extraction tests. |
pkg/cli/logs_missing_tool_test.go | Parallelizes missing-tool tests. |
pkg/cli/logs_missing_data_test.go | Parallelizes missing-data tests. |
pkg/cli/logs_mcp_tool_usage_test.go | Parallelizes MCP aggregation tests. |
pkg/cli/logs_json_test.go | Parallelizes logs-data and JSON tests. |
pkg/cli/logs_github_rate_limit_usage_test.go | Parallelizes rate-limit parsing tests. |
pkg/cli/logs_formatting_test.go | Parallelizes formatting tests. |
pkg/cli/logs_firewall_filter_test.go | Parallelizes firewall tests. |
pkg/cli/logs_extract_zip_test.go | Parallelizes ZIP extraction tests. |
pkg/cli/logs_engine_filter_test.go | Parallelizes engine/runtime filter tests. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 10/10 changed files
- Comments generated: 0
- Review effort level: Balanced
🎉 This pull request is included in a new release. Release: |
Daily Go Test Parallelizer
Analyzed a batch of 25
pkg/clitest files (round-robin, starting right afterpkg/cli/logs_empty_runs_test.goper cache state), using per-fileparallel-safety-checkersub-agents.Changes
Added
t.Parallel()to the start of top-level test functions confirmed safe (not.Setenv/os.Setenv/os.Chdir, no shared mutable globals, no fixed ports/paths outside per-test temp dirs, no ordering dependencies):logs_engine_filter_test.go—TestMatchEngineFilter,TestMatchRuntimeFilterlogs_extract_zip_test.go— all 6 top-level testslogs_firewall_filter_test.go— all 3 top-level testslogs_formatting_test.go—TestFormatNumber,TestFormatFileSizelogs_github_rate_limit_usage_test.go— all 11 top-level testslogs_json_test.go— all 17 top-level testslogs_mcp_tool_usage_test.go— all 4 top-level testslogs_missing_data_test.go— both top-level testslogs_missing_tool_test.go— both top-level testslogs_noop_test.go— all 4 top-level testsTable-driven subtests were intentionally left unparallelized where the loop variable is captured without rebinding, or where iterations share a temp file/dir (would require production/test refactor, which is out of scope).
Left unchanged (judged unsafe)
logs_episode_test.go,logs_fallback_integration_test.go,logs_filtering_test.go,logs_firewall_parse_test.go,logs_flatten_test.go,logs_github_api_test.go,logs_json_clean_test.go,logs_json_stderr_order_test.go,logs_mcp_failure_test.go,logs_missing_tool_integration_test.go,logs_orchestrator_filters_test.go,logs_orchestrator_pagination_test.go,logs_orchestrator_test.go— these uset.Setenv, redirectos.Stdout/os.Stderr, mutate shared package-level stubs, bind fixed ports viahttptest, or have timing-dependent behavior.Validation
go build ./pkg/cli/...— passesgo test -race ./pkg/cli/... -run '<all modified top-level tests>'— all passgo test ./pkg/cli/...full run — only pre-existing, unrelated sandbox failures remain (httptestport binding, TTY color detection), reproduced identically on the unmodified treeNo assertions, production code, or dependencies were changed.