[test-parallel] Add t.Parallel() to safe codemod tests in pkg/cli (batch 2) - #53584
Conversation
Reviewed 25 codemod test files (pkg/cli/codemod_allowed_repos_current_test.go through pkg/cli/codemod_github_app_client_id_test.go) for parallel-safety and added t.Parallel() to top-level tests and table-driven subtests that are demonstrably safe: pure functions operating on local content/frontmatter values, no t.Setenv/os.Setenv/os.Chdir, no shared mutable globals, and t.TempDir()-scoped file I/O only. Validated with go test -race for all modified tests and go test ./pkg/cli/... (pre-existing failures in compile_update_check_test.go and engine_definition_test.go are unrelated sandbox network restrictions, reproduced identically on main before 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! Diff is purely mechanical: adds t.Parallel() to existing test functions/subtests across 24 codemod test files, nothing else changes. No dead code, speculative abstractions, reinvented stdlib, or unneeded dependencies to flag. Lean already. Ship.
|
|
🧠 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.
|
|
Great work on systematically parallelizing these test files! 🚀 This PR looks solid — you have:
The PR is well-described with clear criteria and validation results. Since this is still in draft, a quick heads-up: once you are ready to move it out of draft, reviewers will likely appreciate a summary of the cumulative parallelization progress (total tests parallelized across batches) for context. Otherwise, this is ready for review!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
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.
No actionable blocking issues found in the changed lines.
### Review notes
I checked the added t.Parallel() calls across the touched codemod tests for shared mutable state, process-global mutation, and temp-file collisions. The exercised helpers appear to work on per-test local inputs, and the one file-I/O case is scoped to t.TempDir().
The attempted grumpy-coder pass could not run in this environment because the binary is unavailable, so I discarded that advisory path and completed the review manually.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 7.91 AIC · ⌖ 7.91 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The changes look correct. t.Parallel() is consistently added to all top-level test functions and their subtests across both files. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.2 AIC · ⌖ 8.73 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — changes are additions-only (t.Parallel() calls) with no behavioral modifications.
📋 Assessment
Positive Highlights
- ✅ All 182 additions are
t.Parallel()calls only — no test logic or assertions changed - ✅ Both top-level
Test*functions and theirt.Run(...)subtests are correctly parallelized - ✅ PR description clearly documents the safety criteria checked (no
t.Setenv, no global mutable state,t.TempDir()scoping for file I/O) - ✅ No loop-variable rebinding needed (Go 1.22+ semantics correctly noted)
- ✅
go test -racevalidation passed
No Issues Found
All t.Parallel() calls have correct syntax. No shared mutable state, environment mutations, or unsafe patterns were introduced. This batch is safe to merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 20.4 AIC · ⌖ 9.82 AIC · ⊞ 7.8K
Comment /matt to run again
There was a problem hiding this comment.
Pull request overview
Adds safe parallel execution to codemod unit tests in pkg/cli, reducing test-suite runtime without changing assertions or production behavior.
Changes:
- Added
t.Parallel()to eligible top-level tests. - Parallelized independent table-driven and named subtests.
- Preserved isolated temporary-file usage and local test state.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/codemod_github_app_client_id_test.go |
Parallelizes client-ID migration tests. |
pkg/cli/codemod_factory_test.go |
Parallelizes codemod factory tests. |
pkg/cli/codemod_expires_integer_test.go |
Parallelizes expiration conversion tests. |
pkg/cli/codemod_engine_steps_test.go |
Parallelizes engine steps migration tests. |
pkg/cli/codemod_engine_model_test.go |
Parallelizes engine model migration tests. |
pkg/cli/codemod_engine_max_turns_test.go |
Parallelizes max-turns migration tests. |
pkg/cli/codemod_engine_max_runs_test.go |
Parallelizes max-runs migration tests. |
pkg/cli/codemod_engine_env_secrets_test.go |
Parallelizes engine secret cleanup tests. |
pkg/cli/codemod_engine_driver_test.go |
Parallelizes engine driver migration tests. |
pkg/cli/codemod_effective_tokens_to_ai_credits_test.go |
Parallelizes AI-credit migration tests. |
pkg/cli/codemod_discussion_trigger_categories_test.go |
Parallelizes discussion category tests. |
pkg/cli/codemod_discussion_flag_test.go |
Parallelizes discussion flag tests. |
pkg/cli/codemod_difc_proxy_test.go |
Parallelizes DIFC proxy migration tests. |
pkg/cli/codemod_dependabot_permissions_test.go |
Parallelizes Dependabot permission tests. |
pkg/cli/codemod_copilot_requests_feature_test.go |
Parallelizes Copilot permission migration tests. |
pkg/cli/codemod_cli_proxy_mode_test.go |
Parallelizes CLI proxy migration tests. |
pkg/cli/codemod_checkout_persist_credentials_false_test.go |
Parallelizes checkout credential tests. |
pkg/cli/codemod_byok_copilot_test.go |
Parallelizes BYOK feature removal tests. |
pkg/cli/codemod_bots_test.go |
Parallelizes bot trigger migration tests. |
pkg/cli/codemod_bash_single_quoted_args_test.go |
Parallelizes Bash argument rewrite tests. |
pkg/cli/codemod_bash_anonymous_test.go |
Parallelizes anonymous Bash migration tests. |
pkg/cli/codemod_bash_allowlist_unsupported_engine_test.go |
Parallelizes isolated allowlist tests. |
pkg/cli/codemod_assign_to_agent_test.go |
Parallelizes agent assignment migration tests. |
pkg/cli/codemod_allowed_repos_current_test.go |
Parallelizes repository scope migration tests. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 24/24 changed files
- Comments generated: 0
- Review effort level: Balanced
|
🎉 This pull request is included in a new release. Release: |
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
test> Generated by PR Description Updater for #53584 · auto · 48.9 AIC · ⌖ 6.52 AIC · ⊞ 7.6K · ◷