Add unit tests for compiler_yaml_prompt.go prompt-chunking logic - #49972
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds dedicated unit coverage for prompt chunking, imports, expression mappings, and path resolution.
Changes:
- Adds focused compiler prompt tests.
- Extends the agentic-workflows reference catalog.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_yaml_prompt_test.go |
Adds prompt-processing unit tests. |
.github/skills/agentic-workflows/SKILL.md |
Lists the observability optimization guide. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (2)
pkg/workflow/compiler_yaml_prompt_test.go:162
- This substitution case is also vacuous: the input omits the required
$in${{ ... }}, and the row has no output expectation. It therefore passes without testing the legacy substitution path.
name: "inline markdown with import inputs substitution",
importedMarkdown: "Role: {{ github.aw.inputs.role }}",
importInputs: map[string]any{"role": "engineer"},
},
pkg/workflow/compiler_yaml_prompt_test.go:108
- Only the read-error fallback is exercised for ordered imports with
InlinedImportsenabled. Add the corresponding successful-read case and verify that the imported body and its expression mappings are returned instead of a runtime-import macro; otherwise the ordered compile-time inlining branch remains unprotected.
func TestProcessOrderedPromptImportsFileReadFallback(t *testing.T) {
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Balanced
| { | ||
| name: "markdown with import inputs substitution", | ||
| promptImports: []parser.PromptImportEntry{ | ||
| {Markdown: "Hello {{ github.aw.inputs.name }}!"}, | ||
| }, | ||
| importInputs: map[string]any{"name": "World"}, | ||
| }, |
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| chunks, _ := extractPromptChunksFromMarkdown(tt.body) |
| {Markdown: "# Inline Content"}, | ||
| {ImportPath: ".github/workflows/extra.md"}, | ||
| }, | ||
| wantChunkContain: []string{"# Inline Content", "{{#runtime-import .github/workflows/extra.md}}"}, |
| if len(result) < tt.wantMinMappings { | ||
| t.Errorf("enrichExpressionMappings() got %d mappings, want at least %d", len(result), tt.wantMinMappings) | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 86/100 — Excellent
📊 Metrics (12 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Good coverage addition — the test suite is well-structured and exercises all the key functions. Two blocking issues need fixing before merge:
1. Error returns silently discarded (all test functions)
Every chunks, _ := ... call silently swallows errors. If a function returns an error under an unexpected condition the test passes with wrong results. All error returns should be checked with t.Fatalf.
2. Fragile wantChunkCount == 0 assertion guard
if tt.wantChunkCount > 0 && ... means the assertion is dead code when the field is 0 (Go zero-value). Test cases that explicitly expect 0 chunks (wantChunkCount: 0) are never verified. Use -1 as the "unchecked" sentinel and guard with >= 0, or use a *int pointer field.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 39.8 AIC · ⌖ 7.62 AIC · ⊞ 5.4K
| } | ||
| if !found { | ||
| t.Errorf("processOrderedPromptImports() expected chunks to contain %q; got: %v", sub, chunks) | ||
| } |
There was a problem hiding this comment.
Error returns are silently discarded throughout the test file.
Nearly every call uses chunks, _ := ..., so if a function returns an unexpected error the test silently passes with empty/wrong results instead of failing loudly.
Consider checking errors explicitly:
chunks, err := c.processOrderedPromptImports(data)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}This pattern applies to all usages at lines ~100, ~146, ~203, ~247, ~274, ~456, ~515, ~529, and ~693.
@copilot please address this.
| t.Errorf("processOrderedPromptImports() expected chunks to contain %q; got: %v", sub, chunks) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Fragile wantChunkCount == 0 guard causes silent test misses.
The assertion:
if tt.wantChunkCount > 0 && len(chunks) != tt.wantChunkCount {skips the assertion entirely when wantChunkCount is 0 (the zero-value default). This means test cases that expect exactly 0 chunks are never verified unless additional guard conditions are met. The same pattern recurs at line ~458.
Prefer an explicit sentinel so intent is unambiguous:
type optionalInt struct{ set bool; val int }or simply use -1 as "not set" and check >= 0:
if tt.wantChunkCount >= 0 && len(chunks) != tt.wantChunkCount {
t.Errorf(...)
}(initialise the field to -1 in cases where you don't care about the count)
@copilot please address this.
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (693 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. 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 — requesting changes on assertion quality gaps.
📋 Key Themes & Highlights
Key Themes
- Error returns ignored: All
chunks, _ := ...calls discard errors, hiding real failures. Affects every test function. - Ambiguous zero-count sentinel:
wantChunkCount > 0guard makeswantChunkCount: 0unenforceable without a secondary condition — this is confusing and fragile. - Substitution not asserted: Two test cases exercise
importInputssubstitution but never verify the output contains the substituted value. - Expression mapping content unchecked:
TestBuildMainWorkflowPromptChunksExpressionExtractiononly checkslen > 0, not what the mappings contain.
Positive Highlights
- ✅ Excellent breadth — all 11 functions now have dedicated unit coverage
- ✅ Good use of
t.TempDir()for filesystem-dependent tests - ✅ Fallback behaviour (runtime-import macro on missing file) is explicitly tested
- ✅ Table-driven test structure is clear and consistent
- ✅ Build tag
//go:build !integrationis correctly applied
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 48 AIC · ⌖ 11.3 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
| if !found { | ||
| t.Errorf("processOrderedPromptImports() expected chunks to contain %q; got: %v", sub, chunks) | ||
| } |
There was a problem hiding this comment.
[/tdd] Error return is silently discarded — if processOrderedPromptImports returns a non-nil error, the test will not catch it and may assert on garbage chunks.
💡 Fix
Change the call to:
chunks, err := c.processOrderedPromptImports(data)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}This pattern recurs on lines 146, 203, 247, 274, 456, 515, 529, and 693 — all error returns should be asserted nil.
@copilot please address this.
| t.Errorf("processOrderedPromptImports() expected chunks to contain %q; got: %v", sub, chunks) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
[/tdd] wantChunkCount == 0 is used ambiguously — the guard if tt.wantChunkCount > 0 skips the count assertion entirely when the expected count is 0, meaning a test case that sets wantChunkCount: 0 never actually asserts zero chunks unless the imports list is also empty.
💡 Suggested fix
Replace the two-condition check with a single consistent pattern:
if tt.wantChunkCount >= 0 {
if len(chunks) != tt.wantChunkCount {
t.Errorf("got %d chunks, want %d", len(chunks), tt.wantChunkCount)
}
}Or initialise wantChunkCount to -1 as the sentinel "do not check" value so that 0 reliably means "assert empty".
The same pattern appears in TestBuildMainWorkflowPromptChunks around line 458.
@copilot please address this.
| promptImports: []parser.PromptImportEntry{}, | ||
| wantChunkCount: 0, | ||
| }, | ||
| { |
There was a problem hiding this comment.
[/tdd] The "markdown with import inputs substitution" case (line 60) sets importInputs and wantChunkContain is left nil — the test exercises the substitution code path but never asserts the substituted value was used. If the substitution silently fails, this test still passes.
💡 Fix
Add an assertion:
wantChunkContain: []string{"Hello World!"},Similarly, the "inline markdown with import inputs substitution" case in TestProcessLegacyPromptImports (around line 177) has the same gap.
@copilot please address this.
| chunks, _ := c.processPromptImportEntries(data) | ||
| if len(chunks) == 0 { | ||
| t.Error("processPromptImportEntries() expected chunks from ordered path") | ||
| } |
There was a problem hiding this comment.
[/tdd] TestBuildMainWorkflowPromptChunksExpressionExtraction only asserts len(mappings) == 0 but never checks the content of the returned mappings. A bug that returns a non-empty slice of zero-value mappings would pass undetected.
💡 Fix
Add a content assertion:
if mappings[0].Content != "github.event.issue.number" {
t.Errorf("expected expression content %q, got %q", "github.event.issue.number", mappings[0].Content)
}@copilot please address this.
| }{ | ||
| { | ||
| name: "empty body", | ||
| body: "", |
There was a problem hiding this comment.
[/tdd] TestExtractPromptChunksFromMarkdown has an empty-body case that asserts wantChunks: 1 but leaves wantChunkContain empty — so it only checks that exactly one chunk is returned without verifying the chunk content is the empty string (or at least not nil). This makes the test brittle: any non-empty string accidentally returned would still pass.
💡 Fix
Either assert the single chunk is an empty string:
wantChunkContain: []string{""},Or document intentionally that chunk content is unchecked for the empty-body case with an inline comment.
@copilot please address this.
|
@copilot sous-chef triage: Please refresh the branch if needed and then run the
|
- Use ${{ github.aw.inputs.* }} form in import-inputs substitution cases
and assert the substituted output (Hello World!, Role: engineer)
- Add wantChunkSequence field to ordered/legacy import tests to assert
exact chunk interleaving order, not just "appears somewhere"
- Replace wantMinMappings with exact wantMappingCount in
TestEnrichExpressionMappings so zero-mapping cases can fail
- TestExtractPromptChunksFromMarkdown: capture mappings, assert
XML-comment strings are absent from output, and assert raw ${{ }}
expressions are replaced by env-var placeholders in chunks
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Addressed all four reviewer items in commit
|
🔍 PR TriageCategory: test · Risk: low · Total score: 43/100
Adds a dedicated unit test file for Recommended action:
|
compiler_yaml_prompt.go(336 lines, 11 functions) had no dedicated test file despite containing non-trivial prompt-chunking, import-interleaving, and expression-mapping logic that relies entirely on indirect integration coverage.Added:
pkg/workflow/compiler_yaml_prompt_test.goFocused unit tests for all key functions:
TestProcessOrderedPromptImports+ fallbackprocessOrderedPromptImports— inline markdown, runtime-import macros, mixed entries, import inputs, missing-file fallbackTestProcessLegacyPromptImports+ inlined/fallback variantsprocessLegacyPromptImports— all import modes including disk-read and fallbackTestEnrichExpressionMappingsTestBuildMainWorkflowPromptChunks+ expression extractionTestProcessPromptImportEntriesDispatchprocessPromptImportEntriesTestMergeKnownNeedsExpressionsall-entries win overknownNeedsfor same env varTestResolveWorkspaceRoot.github/-relative and fallback path variantsTestExtractPromptChunksFromMarkdownsplitContentIntoChunksalready has dedicated coverage inxml_comments_test.go; tests here exercise it indirectly through the pipeline.run: https://github.com/github/gh-aw/actions/runs/30815680828