[test-parallel] Add t.Parallel() to safe top-level Go tests in pkg/cli codemod suite - #53491
Conversation
Analyzed 25 test files (batch starting after last processed file pkg/cli/codemod_safe_output_dispatch_repository_key_test.go) and added t.Parallel() to top-level test functions confirmed safe: - All codemod_*.go Apply implementations are pure closures operating on local content/frontmatter with only compiled regexp package vars (thread-safe for concurrent reads), so their tests are safe to run in parallel. - codespace_test.go: added only to TestIs403PermissionError and TestGetCodespacePermissionErrorMessage (TestIsRunningInCodespace uses t.Setenv and was left untouched). - commands_utils_test.go: added to top-level tests that use per-test temp dirs and no process-wide state; left subtests unparallelized where isolation could not be verified. Skipped commands_compile_workflow_test.go, commands_file_watching_test.go, commands_test.go, and commands_new_test.go due to os.Chdir/os.Setenv/ package-global state and other cross-test dependencies identified during review. Verified with 'go test -race' targeted at all modified tests (pass) and a full 'go test ./pkg/cli/...' run showing only pre-existing, unrelated failures (network-port-bound TestRunCompileUpdateCheck and TTY-detection TestRenderScheduleCalendarCell_UsesANSIInColorTerminal), reproduced identically on main without these changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ 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. Reviewed the full diff (102 additions, 0 deletions): every changed line is a single t.Parallel() call added to existing test functions, with no other modifications. No abstractions, wrappers, dependencies, or speculative flexibility introduced.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. 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.
|
|
![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 — changes are safe and well-reasoned; approving.
📋 Analysis
Key Observations
- ✅
TestIsRunningInCodespacecorrectly left un-parallelized (t.Setenvmakes it unsafe) - ✅ All table-driven subtests that received a parallel top-level create their own
tmpDirviatestutil.TempDir(t, ...)— no shared state - ✅ Subtests remain sequential within parallel top-level functions, which is the correct pattern
- ✅ PR description clearly explains the safety analysis for each file
No correctness issues found in the changed lines.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 21.8 AIC · ⌖ 12.6 AIC · ⊞ 7.8K
Comment /matt to run again
There was a problem hiding this comment.
Pull request overview
Adds safe top-level parallelism to pkg/cli Go tests, reducing suite runtime without changing test behavior or production code.
Changes:
- Parallelizes isolated codemod tests.
- Parallelizes pure or temp-directory-isolated CLI utility and Codespaces tests.
- Leaves process-global tests sequential.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/commands_utils_test.go |
Parallelizes isolated utility tests. |
pkg/cli/codespace_test.go |
Parallelizes pure Codespaces helpers. |
pkg/cli/codemod_workflow_dispatch_required_test.go |
Parallelizes workflow-dispatch codemod tests. |
pkg/cli/codemod_user_rate_limit_test.go |
Parallelizes rate-limit migration tests. |
pkg/cli/codemod_upload_assets_test.go |
Parallelizes upload-assets migration tests. |
pkg/cli/codemod_top_level_env_secrets_test.go |
Parallelizes secret-detection codemod tests. |
pkg/cli/codemod_toolset_singular_test.go |
Parallelizes toolset migration tests. |
pkg/cli/codemod_timeout_minutes_test.go |
Parallelizes timeout migration tests. |
pkg/cli/codemod_steps_run_secrets_env_test.go |
Parallelizes step-expression migration tests. |
pkg/cli/codemod_slash_command_test.go |
Parallelizes slash-command migration tests. |
pkg/cli/codemod_serena_mcp_location_test.go |
Parallelizes Serena location migration tests. |
pkg/cli/codemod_serena_import_test.go |
Parallelizes Serena import migration tests. |
pkg/cli/codemod_schema_file_test.go |
Parallelizes schema-file codemod tests. |
pkg/cli/codemod_schedule_test.go |
Parallelizes schedule migration tests. |
pkg/cli/codemod_sandbox_runtime_profile_test.go |
Parallelizes runtime-profile migration tests. |
pkg/cli/codemod_sandbox_mcp_internal_test.go |
Parallelizes sandbox MCP migration tests. |
pkg/cli/codemod_sandbox_agent_test.go |
Parallelizes sandbox-agent migration tests. |
pkg/cli/codemod_sandbox_agent_false_removal_test.go |
Parallelizes agent-false removal tests. |
pkg/cli/codemod_safe_output_require_title_prefix_test.go |
Parallelizes title-prefix migration tests. |
pkg/cli/codemod_safe_output_merge_pr_constraints_test.go |
Parallelizes merge-constraint migration tests. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 20/20 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
The PR correctly adds t.Parallel() to top-level test functions and their subtests across the codemod test suite in pkg/cli. The changes are purely mechanical — no range-loop variable capture issues, no shared mutable state, no side-effecting setup that would be unsafe under parallelism. LGTM ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.5 AIC · ⌖ 8.77 AIC · ⊞ 5.7K
|
🎉 This pull request is included in a new release. Release: |
Summary
Daily automated pass adding
t.Parallel()to top-level Go test functions inpkg/cliwhere it is demonstrably safe, per the round-robin batch starting afterpkg/cli/codemod_safe_output_dispatch_repository_key_test.go.Analysis
codemod_*.goApplyimplementations were inspected directly and confirmed to be pure closures: they operate only on localcontent/frontmatterinputs and package-levelregexp.MustCompile(...)vars (safe for concurrent reads, no mutation). Their tests are safe to parallelize.codespace_test.go: addedt.Parallel()only toTestIs403PermissionErrorandTestGetCodespacePermissionErrorMessage.TestIsRunningInCodespaceusest.Setenvand was left untouched.commands_utils_test.go: added to top-level tests using per-test temp dirs / pure string inputs and no process-wide state. Left unparallelized where table-driven subtests share a tmpDir and could not be proven collision-free without further changes.Explicitly skipped (not modified)
commands_compile_workflow_test.go,commands_file_watching_test.go,commands_test.go,commands_new_test.go- use os.Chdir, global os.Stderr redirection, package-global version state, or other cross-test process-wide dependencies.Validation
go test -race ./pkg/cli/...targeted at all modified top-level tests - all pass.go test ./pkg/cli/...(full suite) - only pre-existing, environment-related failures remain (TestRunCompileUpdateCheck needs a local TCP port, TestRenderScheduleCalendarCell_UsesANSIInColorTerminal needs a TTY); reproduced identically on main without these changes, confirming they are unrelated to this PR.No test behavior, assertions, or production code were changed - only t.Parallel() additions in the selected batch.