Add dedicated unit coverage for activation step helpers - #49800
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds focused unit coverage for activation-job helper behavior and generated YAML.
Changes:
- Tests activation step generation, skipping, outputs, and environment wiring.
- Covers sanitization and orchestration failures.
- Adds reusable activation test setup helpers.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_activation_steps_test.go |
Adds activation helper unit tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Balanced
| err := addActivationSafeOutputMessagesEnv(ctx) | ||
|
|
||
| require.NoError(t, err) | ||
| assert.Contains(t, strings.Join(ctx.steps, ""), "GH_AW_SAFE_OUTPUT_MESSAGES:") |
| err := addActivationSafeOutputMessagesEnv(ctx) | ||
|
|
||
| require.NoError(t, err) |
| err := compiler.addActivationSkillInstallSteps(ctx) | ||
|
|
||
| require.NoError(t, err) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new test file is well-structured and covers the target helper surface thoroughly — success paths, skip/no-op paths, and error paths are all exercised. Good use of t.Cleanup for the isReleaseBuild global and clear sub-test naming.
One minor note: some tests reach into the production newActivationBuildContext constructor while others use the local newActivationStepsTestContext helper. Both are valid, but a short comment explaining that newActivationStepsTestContext is a lightweight stand-in that bypasses full build-context initialisation would help future readers.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.5 AIC · ⌖ 10.3 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report❌ Test Quality Score: 34/100 — Poor
📊 Metrics (45 tests)
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate -- ADR RequiredThis PR makes significant changes to core business logic (508 new lines in 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 MatterADRs 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on a few structural issues before merge.
📋 Key Themes & Highlights
Key Themes
- Test helper divergence:
newActivationStepsTestContextbypassesnewActivationBuildContext, so derived fields are set by hand. If the constructor changes, the tests won't catch it automatically. (Line 23) - Global variable mutation:
isReleaseBuildis patched inline in two tests — this is a race hazard under-race. (Lines 142, 234) - Test names are descriptive, not specification-style: names like
"adds reaction step"read as implementation notes rather than behavior contracts. (Pervasive)
Positive Highlights
- ✅ Excellent helper-level coverage across 12+ helpers — this directly addresses the gap called out in the issue
- ✅ Error-path assertions (
require.Error) are present for all error-returning helpers - ✅
t.Cleanupis used correctly for global restore, which is an improvement over rawdeferin table-driven tests - ✅ Clear separation: each
Test*function targets exactly one helper method
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.5 AIC · ⌖ 8.43 AIC · ⊞ 7.1K
Comment /matt to run again
| return compiler | ||
| } | ||
|
|
||
| func newActivationStepsTestContext(data *WorkflowData) *activationJobBuildContext { |
There was a problem hiding this comment.
[/codebase-design] newActivationStepsTestContext duplicates logic from newActivationBuildContext — derived fields (hasReaction, reactionIssues, statusCommentIssues, etc.) are set by hand in individual tests instead of flowing through the real constructor. When the production constructor evolves, these tests can silently stop matching real behavior.
💡 Suggested fix
Replace the helper with a thin wrapper around the real constructor:
func newActivationStepsTestContext(data *WorkflowData) *activationJobBuildContext {
if data == nil {
data = &WorkflowData{}
}
return newActivationBuildContext(data, false, "", "test.lock.yml")
}Then update call sites that manually set ctx.hasReaction = true to instead set data.AIReaction = "eyes", driving the field through the real code path and ensuring the test exercises the full initialization.
@copilot please address this.
|
|
||
| func TestActivationStepsAddRepositoryAndOutputSteps(t *testing.T) { | ||
| t.Run("adds repository and output steps", func(t *testing.T) { | ||
| originalIsRelease := isReleaseBuild |
There was a problem hiding this comment.
[/tdd] isReleaseBuild is a package-level variable mutated directly in the test — this is a data race if tests ever run with -parallel or -race. The existing pattern in aw_info_versions_test.go uses defer for restore, but the race window still exists.
💡 Suggested fix
Instead of mutating the global, pass isRelease through the compiler constructor or accept it as a parameter to addActivationVersionCheckStep and addActivationRepositoryAndOutputSteps. This removes the shared-state dependency entirely and makes the tests safe to run in parallel:
t.Run("adds version check for release builds", func(t *testing.T) {
t.Parallel()
compiler := newActivationStepsTestCompiler("v1.2.3")
compiler.isRelease = true // per-instance flag, not global
...
})If refactoring the compiler is too large for this PR, at minimum document the race risk with a //nolint:paralleltest comment and a TODO.
@copilot please address this.
| compiler := newActivationStepsTestCompiler("") | ||
|
|
||
| t.Run("adds reaction step", func(t *testing.T) { | ||
| ctx := newActivationStepsTestContext(&WorkflowData{ |
There was a problem hiding this comment.
[/tdd] Test names describe what the step does rather than the behavior being specified — e.g. "adds reaction step" vs "emits react step with configured emoji when reaction is enabled". With spec-style names, a failing test immediately tells you which contract broke.
💡 Example rename
// Before
t.Run("adds reaction step", ...)
t.Run("skips when reaction is disabled", ...)
// After
t.Run("emits react step wired to configured emoji when reaction is enabled", ...)
t.Run("emits no steps when reaction flag is not set", ...)This is a low-effort, high-readability win — apply the pattern consistently across the file.
@copilot please address this.
| compiler := newActivationStepsTestCompiler("") | ||
|
|
||
| t.Run("adds text output step", func(t *testing.T) { | ||
| ctx := newActivationStepsTestContext(&WorkflowData{ |
There was a problem hiding this comment.
[/tdd] TestActivationStepsAddTextOutputStep tests the "adds text output step" sub-case but only checks that certain strings appear in the joined YAML — it does not assert the full env block is absent for the NeedsTextOutput: false case beyond checking ctx.steps is empty. Consider also asserting that no sanitized step ID leaks into a context that has unrelated steps accumulated prior to the call, i.e. testing isolation between calls.
💡 Missing edge case to add
t.Run("does not emit sanitized step when called after other steps", func(t *testing.T) {
ctx := newActivationStepsTestContext(&WorkflowData{})
ctx.steps = append(ctx.steps, "- name: prior-step\n")
err := compiler.addActivationTextOutputStep(ctx)
require.NoError(t, err)
// Confirm the pre-existing step is unchanged and no sanitized step was added
assert.Len(t, ctx.steps, 1)
assert.NotContains(t, ctx.steps[0], "sanitized")
})@copilot please address this.
|
@copilot Please address the remaining review and check blockers on this PR, then run the Newest signals to close:
Please fix the failing unit-test signal first, then reconcile the remaining review feedback and ADR gate, and finish with a brief maintainer handoff.
|
- Fix CI failure: update model from 'small' to 'claude-haiku-4.5' in pr-code-quality-reviewer.md (TestPRCodeQualityReviewerWorkflowSubAgentModelContract) - Remove unreachable error return from addActivationSkillInstallSteps (always returned nil) - Remove unreachable error return from addActivationSafeOutputMessagesEnv (json.Marshal cannot fail on SafeOutputMessagesConfig which contains only strings and bools) - Update addActivationStatusCommentStep caller to not error-check removed return - Update compiler_activation_job.go to call addActivationSkillInstallSteps without error check - Strengthen test assertions: check serialized message value (not just key) in TestActivationStepsAddSafeOutputMessagesEnv and TestActivationStepsAddStatusCommentStep - Update tests to match new non-error function signatures for skill install and messages env - Complete ADR-49800: change status from Draft to Accepted, update consequences to reflect the unreachable error returns that were removed during focused test authoring Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Run details: https://github.com/github/gh-aw/actions/runs/30757146330
|
|
🎉 This pull request is included in a new release. Release: |
pkg/workflow/compiler_activation_steps.gohad no dedicated unit test file despite containing a dense set of activation-job helpers, including several error-returning paths. This change adds focused coverage for those helpers so activation-step behavior is exercised directly instead of only through broader compiler tests.New activation-step test file
pkg/workflow/compiler_activation_steps_test.goDirect success/error-path coverage
error, especially around:Focused assertions on generated activation YAML
Example of the new direct helper-level coverage pattern: