Split oversized test files compiler_jobs_test.go and compiler_safe_outputs_config_test.go - #53818
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey ✅ What looks great:
This looks ready for review whenever you take it out of draft.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. This PR is a pure mechanical split of two oversized test files into smaller ones along functional boundaries, with no logic changes, no new abstractions, no duplicated helpers, and no added complexity — nothing fits the ponytail-review criteria (delete/stdlib/native/yagni/shrink).
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No blocking issues found in the test-file split; the patch only redistributes existing tests into smaller files and I did not find a moved assertion that changed behavior or coverage semantics.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 21 AIC · ⌖ 8.63 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Splits two oversized compiler test suites into focused files without changing production behavior.
Changes:
- Organizes 84 job compiler tests across 11 files.
- Organizes 48 safe-output configuration tests across 8 files.
- Preserves package-level test helpers and functional groupings.
Show a summary per file
| File | Description |
|---|---|
compiler_jobs_test.go |
Retains extraction and helper tests. |
compiler_jobs_builtin_test.go |
Covers built-in job construction. |
compiler_jobs_steps_test.go |
Covers setup and pre-step ordering. |
compiler_jobs_safe_outputs_test.go |
Covers safe-output and reusable workflow jobs. |
compiler_jobs_dependencies_test.go |
Covers dependency chains. |
compiler_jobs_memory_test.go |
Covers memory jobs. |
compiler_jobs_edge_cases_test.go |
Covers custom-job edge cases. |
compiler_jobs_custom_test.go |
Covers custom-job construction. |
compiler_jobs_custom_fields_test.go |
Covers custom-job fields. |
compiler_jobs_state_test.go |
Covers state-push jobs. |
compiler_jobs_engine_env_test.go |
Covers engine environment dependencies. |
compiler_safe_outputs_config_test.go |
Retains the main table-driven config test. |
compiler_safe_outputs_config_handlers_test.go |
Covers handler fields. |
compiler_safe_outputs_config_targets_test.go |
Covers target repositories and patch limits. |
compiler_safe_outputs_config_assignment_test.go |
Covers assignment handlers. |
compiler_safe_outputs_config_staged_test.go |
Covers staged mode. |
compiler_safe_outputs_config_protection_test.go |
Covers protected files and checkout mapping. |
compiler_safe_outputs_config_pr_policy_test.go |
Covers pull-request policies. |
compiler_safe_outputs_config_reporting_test.go |
Covers failure reporting and data mode. |
Review details
- Files reviewed: 18/19 changed files
- Comments generated: 1
- Review effort level: Balanced
| // testBoolPtr is a helper function for bool pointers in config tests | ||
| func testBoolPtr(b bool) *bool { return new(b) } | ||
|
|
||
| // testStringPtr is a helper function for string pointers in config tests | ||
| func testStringPtr(s string) *string { return new(s) } |
There was a problem hiding this comment.
Addressed in d161b043a3: removed the duplicate testBoolPtr/testStringPtr definitions from compiler_safe_outputs_config_targets_test.go and switched the moved call sites to the shared boolPtr/strPtr helpers.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — this is a well-structured refactor with one compilation bug to fix.
📋 Key Themes & Highlights
Key Themes
- Compilation bug:
new(b)/new(s)incompiler_safe_outputs_config_targets_test.go— Go'snewtakes a type not a value; fix isreturn &b/return &s - Helper locality:
testBoolPtr/testStringPtrare defined in_targets_test.gobut consumed across 5+ other test files. This is fine at package scope, but a sharedtesthelpers_test.gowould make the dependency explicit.
Positive Highlights
- ✅ Pure move — no test logic changed
- ✅ Each file gets only the imports it uses
- ✅ Residual files stay within the 500–1000 line band
- ✅ Section banners make each file self-documenting
- ✅ Split boundaries map cleanly onto functional subsystems
One blocking issue (compilation bug) in compiler_safe_outputs_config_targets_test.go — requesting changes.
@copilot please address the review comments above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 78.1 AIC · ⌖ 9.92 AIC · ⊞ 7.8K
Comment /matt to run again
| } | ||
|
|
||
| // testBoolPtr is a helper function for bool pointers in config tests | ||
| func testBoolPtr(b bool) *bool { return new(b) } |
There was a problem hiding this comment.
[/codebase-design] new(b) is a compilation error — new takes a type in Go, not a value. This will fail to build.
💡 Fix
func testBoolPtr(b bool) *bool { return &b }
func testStringPtr(s string) *string { return &s }return &b captures the local copy of the parameter, which is exactly the right behaviour for a pointer-helper.
@copilot please address this.
There was a problem hiding this comment.
Addressed in d161b043a3: the duplicate helpers containing new(b)/new(s) were removed from compiler_safe_outputs_config_targets_test.go; affected tests now use the package-shared helpers, and go test ./pkg/workflow plus make agent-report-progress pass locally.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (7,076 new lines added) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 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
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
|
@copilot A recent author update landed on this PR, but there is still no maintainer-visible resolution on the blocking review. Please refresh the branch if needed, re-run the affected checks, and then run the Current maintainer-visible blocker:
A branch refresh was attempted by sous-chef as well.
|
Confirmed on current head |
|
@copilot A recent author update landed on this PR, but the earlier blocking review and ADR gate still need a concise maintainer-facing resolution summary. Current maintainer-visible blocker:
|
Confirmed on current head |
|
Triage: category= Very large diff (7146+/6849-, 28 files) splitting oversized test files. CHANGES_REQUESTED outstanding, mergeable_state=blocked, ADR-gated. High risk given size — recommend deferring until blockers are resolved and diff is re-verified against main.
|
|
@copilot A recent author update landed on this PR, but the earlier blocking review and ADR gate still need a concise maintainer-facing resolution summary. Current maintainer-visible blocker:
|
Confirmed on current head Ran |
|
@copilot Please address the latest maintainer-visible blockers on this PR and then run the Current maintainer-visible blockers:
A branch refresh was attempted by sous-chef as well.
|
Confirmed on current head Direct ADR link for the gate/PR body: https://github.com/github/gh-aw/blob/copilot/deep-report-split-oversized-test-files/docs/adr/53818-split-oversized-test-files-by-functional-boundary.md Re-ran |
|
@copilot Please address the latest maintainer-visible blockers on this PR and then run the Current maintainer-visible blockers:
|
Current head Ran |
The
developer-code-organizationskill mandates 100–500 line files (800 only for core infrastructure), butpkg/workflow/compiler_jobs_test.go(4,511 lines / 84 test funcs) andcompiler_safe_outputs_config_test.go(3,837 lines) group tests by broad subsystem, making them merge-conflict hotspots and slow to navigate. This splits both along functional boundaries, following thefrontmatter.goprecedent.compiler_jobs_test.go→ 11 filescompiler_jobs_test.gocompiler_jobs_builtin_test.gocompiler_jobs_steps_test.gocompiler_jobs_safe_outputs_test.gocompiler_jobs_dependencies_test.gocompiler_jobs_memory_test.gocompiler_jobs_edge_cases_test.gocompiler_jobs_custom_test.gobuildCustomJobsdeps, permissions, conditionalscompiler_jobs_custom_fields_test.gocompiler_jobs_state_test.gocompiler_jobs_engine_env_test.goengine.envneeds-expression handlingcompiler_safe_outputs_config_test.go→ 8 filesSplit by safe-output concern: handler config fields, target repo & patch limits, auto-enabled handlers & assignment, staged mode, protected files & checkout mapping, PR policy & fork-backed PRs, failure reporting & data mode.
The residual
compiler_safe_outputs_config_test.gois 965 lines because it holds the single table-drivenTestAddHandlerManagerConfigEnvVar; splitting one test function would duplicate its runner body, so it was left intact.Mechanics
funcdeclarations and whitespace-normalized bodies are identical before/after.run: https://github.com/github/gh-aw/actions/runs/32204573204> Generated by 👨🍳 PR Sous Chef · gpt54 · 25.9 AIC · ⌖ 8.32 AIC · ⊞ 9.3K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32210439073> Generated by 👨🍳 PR Sous Chef · gpt54 · 25.8 AIC · ⌖ 8.18 AIC · ⊞ 9.3K · ◷