test: add unit tests for subagent-prompt-builder module#236
test: add unit tests for subagent-prompt-builder module#236nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
Add 30 tests covering SubagentPromptBuilder class: - Constructor initialization and path setup - Agent/task definition loading with fallback patterns - Checklist extraction from frontmatter and phase config - Template loading with multi-extension support - Context section formatting for previous phases - Prompt assembly with all components - Integration test for full buildPrompt flow
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a comprehensive unit test suite for the SubagentPromptBuilder class with 30 test cases covering constructor initialization, agent/task definition loading, checklist and template extraction, context formatting, and full prompt assembly, using mocks for fs-extra and js-yaml. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/core/orchestration/subagent-prompt-builder.test.js (3)
51-61: Misleading inline comment on line 54.The test name and the
data-engineerinput both confirm this test exercises the dash→underscore fallback, but the comment// architect.md (no dash to underscore here)says the opposite and references a different agent name entirely.📝 Proposed fix
- fs.pathExists - .mockResolvedValueOnce(false) // architect.md not found - .mockResolvedValueOnce(true); // architect.md (no dash to underscore here) + fs.pathExists + .mockResolvedValueOnce(false) // data-engineer.md not found + .mockResolvedValueOnce(true); // data_engineer.md (dash-to-underscore variant) found🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/subagent-prompt-builder.test.js` around lines 51 - 61, The inline comment in the test for the dash→underscore fallback is incorrect and references "architect.md"; update the comment near the second fs.pathExists mock (in the test 'tries alternative naming pattern with underscores') to correctly state that the second mockResolvedValueOnce(true) simulates the fallback file being found (e.g., "data_engineer.md" found) for builder.loadAgentDefinition('data-engineer'), so the comment accurately describes the dash→underscore fallback behavior.
260-267: "Strips existing extension" assertion doesn't verify the intended behavior.
expect(fs.pathExists).toHaveBeenCalledWith(expect.stringContaining('report.yaml'))is trivially satisfied even if the implementation passes the raw'report.yaml'input straight through without stripping anything. The assertion cannot distinguish between correct stripping + re-appending and no stripping at all.A more meaningful assertion verifies that only one
pathExistscall was made (i.e.,.yamlwas tried first after stripping, not.yaml.yaml), and that no double-extension path was attempted:🔍 Proposed stronger assertion
- await builder.loadTemplate('report.yaml'); - - expect(fs.pathExists).toHaveBeenCalledWith(expect.stringContaining('report.yaml')); + await builder.loadTemplate('report.yaml'); + + expect(fs.pathExists).toHaveBeenCalledTimes(1); + expect(fs.pathExists).toHaveBeenCalledWith(expect.stringContaining('report.yaml')); + expect(fs.pathExists).not.toHaveBeenCalledWith(expect.stringContaining('report.yaml.yaml'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/subagent-prompt-builder.test.js` around lines 260 - 267, The test's assertion is too weak; update the assertions for builder.loadTemplate to ensure the extension stripping logic is exercised by asserting fs.pathExists was invoked exactly once and that it was not called with a double-extension (e.g., ensure fs.pathExists was not called with a string containing '.yaml.yaml') while still asserting the final attempted path ends with 'report.yaml' (use expect.stringContaining or a regex match against the call argument); reference builder.loadTemplate and fs.pathExists to locate and modify the assertions accordingly.
149-163: Deduplication test doesn't assert checklist content — partial bug coverage.The test verifies
resulthas length 2 and checks.namevalues, but doesn't assert.content. If the deduplication logic were buggy and consumed thereadFilemocks in the wrong order (e.g., returning'Override checklist'fortestingandundefinedforquality-gate), the name assertions would still pass while the content would be wrong.✅ Proposed improvement
expect(result).toHaveLength(2); expect(result[0].name).toBe('quality-gate'); + expect(result[0].content).toBe('Override checklist'); expect(result[1].name).toBe('testing'); + expect(result[1].content).toBe('Testing checklist');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/subagent-prompt-builder.test.js` around lines 149 - 163, The test for extractAndLoadChecklists should also assert the checklist .content values to ensure deduplication uses the override for 'quality-gate' and the frontmatter file for 'testing'; update the test in tests/core/orchestration/subagent-prompt-builder.test.js to add expectations on result[0].content === 'Override checklist' and result[1].content === 'Testing checklist', and confirm the fs.readFile mockResolvedValueOnce order matches those expectations so the readFile stubs are consumed in the intended sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/orchestration/subagent-prompt-builder.test.js`:
- Around line 19-22: Add a global restore of jest spies in this test file by
adding jest.restoreAllMocks() in an afterEach block (paired with the existing
beforeEach that resets mocks and constructs SubagentPromptBuilder), and remove
the inline consoleSpy.mockRestore() calls from the individual tests (including
the two tests that exercise loadTaskDefinition and the other
console.warn-wrapping tests) so that spies are always restored even if an
assertion throws; keep the consoleSpy variable usage and assertions unchanged,
only eliminate per-test mockRestore() and rely on afterEach
jest.restoreAllMocks().
- Around line 390-405: The test "builds complete prompt from files" assumes
yaml.load behavior but doesn't mock it; explicitly mock yaml.load (e.g., set
yaml.load to a jest mock that returns undefined or a parsed object as
appropriate) before calling builder.buildPrompt in that test so the test doesn't
rely on internal frontmatter guards—add the mock setup alongside
fs.pathExists/fs.readFile stubs referenced in the test to ensure deterministic
behavior when invoking builder.buildPrompt.
---
Nitpick comments:
In `@tests/core/orchestration/subagent-prompt-builder.test.js`:
- Around line 51-61: The inline comment in the test for the dash→underscore
fallback is incorrect and references "architect.md"; update the comment near the
second fs.pathExists mock (in the test 'tries alternative naming pattern with
underscores') to correctly state that the second mockResolvedValueOnce(true)
simulates the fallback file being found (e.g., "data_engineer.md" found) for
builder.loadAgentDefinition('data-engineer'), so the comment accurately
describes the dash→underscore fallback behavior.
- Around line 260-267: The test's assertion is too weak; update the assertions
for builder.loadTemplate to ensure the extension stripping logic is exercised by
asserting fs.pathExists was invoked exactly once and that it was not called with
a double-extension (e.g., ensure fs.pathExists was not called with a string
containing '.yaml.yaml') while still asserting the final attempted path ends
with 'report.yaml' (use expect.stringContaining or a regex match against the
call argument); reference builder.loadTemplate and fs.pathExists to locate and
modify the assertions accordingly.
- Around line 149-163: The test for extractAndLoadChecklists should also assert
the checklist .content values to ensure deduplication uses the override for
'quality-gate' and the frontmatter file for 'testing'; update the test in
tests/core/orchestration/subagent-prompt-builder.test.js to add expectations on
result[0].content === 'Override checklist' and result[1].content === 'Testing
checklist', and confirm the fs.readFile mockResolvedValueOnce order matches
those expectations so the readFile stubs are consumed in the intended sequence.
| beforeEach(() => { | ||
| jest.resetAllMocks(); | ||
| builder = new SubagentPromptBuilder('/project'); | ||
| }); |
There was a problem hiding this comment.
Console spy may leak across tests on assertion failure.
consoleSpy.mockRestore() is placed at the end of two test bodies (lines 71, 115). If any earlier assertion in that test throws, mockRestore() is never called. jest.resetAllMocks() in beforeEach calls mockReset() (clears implementations/return values) but not mockRestore(), so console.warn remains wrapped as a jest spy for all subsequent tests.
The simplest fix is to add jest.restoreAllMocks() to an afterEach, which makes the inline mockRestore() calls redundant:
🛡️ Proposed fix
beforeEach(() => {
jest.resetAllMocks();
builder = new SubagentPromptBuilder('/project');
});
+ afterEach(() => {
+ jest.restoreAllMocks();
+ });- const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
-
- const result = await builder.loadAgentDefinition('unknown-agent');
-
- expect(result).toContain('# Agent: unknown-agent');
- expect(result).toContain('No definition file found');
- consoleSpy.mockRestore();
+ jest.spyOn(console, 'warn').mockImplementation();
+
+ const result = await builder.loadAgentDefinition('unknown-agent');
+
+ expect(result).toContain('# Agent: unknown-agent');
+ expect(result).toContain('No definition file found');Apply the same removal to the loadTaskDefinition variant (lines 110–115).
Also applies to: 63-72, 108-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/orchestration/subagent-prompt-builder.test.js` around lines 19 -
22, Add a global restore of jest spies in this test file by adding
jest.restoreAllMocks() in an afterEach block (paired with the existing
beforeEach that resets mocks and constructs SubagentPromptBuilder), and remove
the inline consoleSpy.mockRestore() calls from the individual tests (including
the two tests that exercise loadTaskDefinition and the other
console.warn-wrapping tests) so that spies are always restored even if an
assertion throws; keep the consoleSpy variable usage and assertions unchanged,
only eliminate per-test mockRestore() and rely on afterEach
jest.restoreAllMocks().
| test('builds complete prompt from files', async () => { | ||
| fs.pathExists.mockResolvedValue(true); | ||
| fs.readFile.mockImplementation((path) => { | ||
| if (path.includes('agents')) return Promise.resolve('# Agent Definition'); | ||
| if (path.includes('tasks')) return Promise.resolve('# Task Definition'); | ||
| return Promise.resolve('content'); | ||
| }); | ||
|
|
||
| const result = await builder.buildPrompt('architect', 'design.md', { | ||
| creates: 'output.md', | ||
| }); | ||
|
|
||
| expect(result).toContain('AGENT TRANSFORMATION'); | ||
| expect(result).toContain('# Agent Definition'); | ||
| expect(result).toContain('# Task Definition'); | ||
| }); |
There was a problem hiding this comment.
yaml.load is not mocked in buildPrompt test 1 — implicit implementation assumption.
After jest.resetAllMocks(), yaml.load is a no-op jest function that returns undefined. The test relies on the fact that '# Task Definition' contains no YAML frontmatter delimiters (---), so the implementation presumably skips calling yaml.load. If the implementation is ever refactored to call yaml.load unconditionally (or the guard condition changes), this test will silently break.
Making the mock explicit removes the coupling to the implementation's internal frontmatter guard:
🛡️ Proposed fix
test('builds complete prompt from files', async () => {
fs.pathExists.mockResolvedValue(true);
fs.readFile.mockImplementation((path) => {
if (path.includes('agents')) return Promise.resolve('# Agent Definition');
if (path.includes('tasks')) return Promise.resolve('# Task Definition');
return Promise.resolve('content');
});
+ // Task definition has no frontmatter, so yaml.load should not be called;
+ // guard explicitly to avoid relying on an internal implementation detail.
+ yaml.load.mockReturnValue({});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('builds complete prompt from files', async () => { | |
| fs.pathExists.mockResolvedValue(true); | |
| fs.readFile.mockImplementation((path) => { | |
| if (path.includes('agents')) return Promise.resolve('# Agent Definition'); | |
| if (path.includes('tasks')) return Promise.resolve('# Task Definition'); | |
| return Promise.resolve('content'); | |
| }); | |
| const result = await builder.buildPrompt('architect', 'design.md', { | |
| creates: 'output.md', | |
| }); | |
| expect(result).toContain('AGENT TRANSFORMATION'); | |
| expect(result).toContain('# Agent Definition'); | |
| expect(result).toContain('# Task Definition'); | |
| }); | |
| test('builds complete prompt from files', async () => { | |
| fs.pathExists.mockResolvedValue(true); | |
| fs.readFile.mockImplementation((path) => { | |
| if (path.includes('agents')) return Promise.resolve('# Agent Definition'); | |
| if (path.includes('tasks')) return Promise.resolve('# Task Definition'); | |
| return Promise.resolve('content'); | |
| }); | |
| // Task definition has no frontmatter, so yaml.load should not be called; | |
| // guard explicitly to avoid relying on an internal implementation detail. | |
| yaml.load.mockReturnValue({}); | |
| const result = await builder.buildPrompt('architect', 'design.md', { | |
| creates: 'output.md', | |
| }); | |
| expect(result).toContain('AGENT TRANSFORMATION'); | |
| expect(result).toContain('# Agent Definition'); | |
| expect(result).toContain('# Task Definition'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/orchestration/subagent-prompt-builder.test.js` around lines 390 -
405, The test "builds complete prompt from files" assumes yaml.load behavior but
doesn't mock it; explicitly mock yaml.load (e.g., set yaml.load to a jest mock
that returns undefined or a parsed object as appropriate) before calling
builder.buildPrompt in that test so the test doesn't rely on internal
frontmatter guards—add the mock setup alongside fs.pathExists/fs.readFile stubs
referenced in the test to ensure deterministic behavior when invoking
builder.buildPrompt.
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive unit tests for the SubagentPromptBuilder class, bringing a previously untested 368-line module up to 30 tests covering all major methods and edge cases. The SubagentPromptBuilder is a critical orchestration component responsible for assembling prompts from agent definitions, task files, checklists, and templates during sub-agent execution.
Changes:
- Added 30 unit tests covering constructor, file loading methods, checklist/template extraction, context formatting, and prompt assembly
- Implemented mocking for fs-extra and js-yaml to isolate tests from filesystem
- Covered both happy paths and edge cases (missing files, invalid YAML, fallback patterns)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fs.readFile.mockResolvedValueOnce('# Alt Task'); | ||
|
|
||
| const result = await builder.loadTaskDefinition('*create-story'); | ||
|
|
There was a problem hiding this comment.
This test doesn't properly verify the alternative path behavior for action names. According to the source code (lines 116-120), when a task file starts with '*' (like '*create-story'), the code strips the asterisk and tries that path. However, this test doesn't verify that the correct paths are being checked. The test should verify:
- First attempt checks the path with the asterisk included
- Second attempt checks the path with the asterisk removed
Consider adding assertions to verify the exact paths being checked with fs.pathExists.
| expect(fs.pathExists).toHaveBeenCalledTimes(2); | |
| expect(fs.pathExists).toHaveBeenNthCalledWith( | |
| 1, | |
| expect.stringContaining('*create-story') | |
| ); | |
| expect(fs.pathExists).toHaveBeenNthCalledWith( | |
| 2, | |
| expect.stringContaining('create-story') | |
| ); | |
| expect(fs.pathExists.mock.calls[1][0]).not.toContain('*'); |
| describe('extractAndLoadTemplates', () => { | ||
| test('loads override template from phase config', async () => { | ||
| fs.pathExists.mockResolvedValueOnce(true); | ||
| fs.readFile.mockResolvedValueOnce('template: content'); | ||
|
|
||
| const result = await builder.extractAndLoadTemplates('No frontmatter', 'report.yaml'); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(result[0].name).toBe('report.yaml'); | ||
| }); | ||
|
|
||
| test('loads templates from task frontmatter', async () => { | ||
| const taskDef = '---\ntemplates:\n - report\n - schema\n---\n# Task'; | ||
| yaml.load.mockReturnValue({ templates: ['report', 'schema'] }); | ||
| fs.pathExists.mockResolvedValue(true); | ||
| fs.readFile | ||
| .mockResolvedValueOnce('report template') | ||
| .mockResolvedValueOnce('schema template'); | ||
|
|
||
| const result = await builder.extractAndLoadTemplates(taskDef); | ||
|
|
||
| expect(result).toHaveLength(2); | ||
| }); | ||
|
|
||
| test('handles invalid YAML frontmatter', async () => { | ||
| const taskDef = '---\n{bad\n---\n# Task'; | ||
| yaml.load.mockImplementation(() => { throw new Error('parse'); }); | ||
|
|
||
| const result = await builder.extractAndLoadTemplates(taskDef); | ||
| expect(result).toHaveLength(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test case for extractAndLoadTemplates when the task definition has no frontmatter. The extractAndLoadChecklists tests include a "handles missing frontmatter gracefully" test (lines 165-168), but extractAndLoadTemplates lacks a similar test. Since both methods have identical frontmatter parsing logic, this case should also be tested for templates to ensure symmetric coverage.
| describe('extractAndLoadChecklists', () => { | ||
| test('loads override checklist from phase config', async () => { | ||
| fs.pathExists.mockResolvedValueOnce(true); | ||
| fs.readFile.mockResolvedValueOnce('- [ ] Check 1\n- [ ] Check 2'); | ||
|
|
||
| const result = await builder.extractAndLoadChecklists('No frontmatter', 'quality-gate'); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(result[0].name).toBe('quality-gate'); | ||
| expect(result[0].content).toContain('Check 1'); | ||
| }); | ||
|
|
||
| test('loads checklists from task frontmatter', async () => { | ||
| const taskDef = '---\nchecklists:\n - code-review\n - testing\n---\n# Task'; | ||
| yaml.load.mockReturnValue({ checklists: ['code-review', 'testing'] }); | ||
| fs.pathExists.mockResolvedValue(true); | ||
| fs.readFile | ||
| .mockResolvedValueOnce('Code review checklist') | ||
| .mockResolvedValueOnce('Testing checklist'); | ||
|
|
||
| const result = await builder.extractAndLoadChecklists(taskDef); | ||
|
|
||
| expect(result).toHaveLength(2); | ||
| expect(result[0].name).toBe('code-review'); | ||
| expect(result[1].name).toBe('testing'); | ||
| }); | ||
|
|
||
| test('skips duplicate checklist from override', async () => { | ||
| const taskDef = '---\nchecklists:\n - quality-gate\n - testing\n---\n# Task'; | ||
| yaml.load.mockReturnValue({ checklists: ['quality-gate', 'testing'] }); | ||
| fs.pathExists.mockResolvedValue(true); | ||
| fs.readFile | ||
| .mockResolvedValueOnce('Override checklist') | ||
| .mockResolvedValueOnce('Testing checklist'); | ||
|
|
||
| const result = await builder.extractAndLoadChecklists(taskDef, 'quality-gate'); | ||
|
|
||
| // quality-gate loaded once (override), testing loaded from frontmatter | ||
| expect(result).toHaveLength(2); | ||
| expect(result[0].name).toBe('quality-gate'); | ||
| expect(result[1].name).toBe('testing'); | ||
| }); | ||
|
|
||
| test('handles missing frontmatter gracefully', async () => { | ||
| const result = await builder.extractAndLoadChecklists('# No frontmatter here'); | ||
| expect(result).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('handles invalid YAML in frontmatter', async () => { | ||
| const taskDef = '---\n{invalid yaml\n---\n# Task'; | ||
| yaml.load.mockImplementation(() => { throw new Error('parse error'); }); | ||
|
|
||
| const result = await builder.extractAndLoadChecklists(taskDef); | ||
| expect(result).toHaveLength(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests don't cover the case where a checklist referenced in frontmatter doesn't exist (loadChecklist returns null). According to the source code (lines 151-154), when loadChecklist returns null, that checklist is simply skipped. This behavior should be tested to ensure that: (1) the method doesn't fail when a referenced checklist is missing, and (2) other valid checklists are still loaded correctly.
| describe('extractAndLoadTemplates', () => { | ||
| test('loads override template from phase config', async () => { | ||
| fs.pathExists.mockResolvedValueOnce(true); | ||
| fs.readFile.mockResolvedValueOnce('template: content'); | ||
|
|
||
| const result = await builder.extractAndLoadTemplates('No frontmatter', 'report.yaml'); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(result[0].name).toBe('report.yaml'); | ||
| }); | ||
|
|
||
| test('loads templates from task frontmatter', async () => { | ||
| const taskDef = '---\ntemplates:\n - report\n - schema\n---\n# Task'; | ||
| yaml.load.mockReturnValue({ templates: ['report', 'schema'] }); | ||
| fs.pathExists.mockResolvedValue(true); | ||
| fs.readFile | ||
| .mockResolvedValueOnce('report template') | ||
| .mockResolvedValueOnce('schema template'); | ||
|
|
||
| const result = await builder.extractAndLoadTemplates(taskDef); | ||
|
|
||
| expect(result).toHaveLength(2); | ||
| }); | ||
|
|
||
| test('handles invalid YAML frontmatter', async () => { | ||
| const taskDef = '---\n{bad\n---\n# Task'; | ||
| yaml.load.mockImplementation(() => { throw new Error('parse'); }); | ||
|
|
||
| const result = await builder.extractAndLoadTemplates(taskDef); | ||
| expect(result).toHaveLength(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests don't cover the case where a template referenced in frontmatter doesn't exist (loadTemplate returns null). According to the source code (lines 207-210), when loadTemplate returns null, that template is simply skipped. This behavior should be tested to ensure that: (1) the method doesn't fail when a referenced template is missing, and (2) other valid templates are still loaded correctly.
| if (path.includes('tasks')) return Promise.resolve('# Task Definition'); | ||
| return Promise.resolve('content'); | ||
| }); | ||
|
|
||
| const result = await builder.buildPrompt('architect', 'design.md', { | ||
| creates: 'output.md', | ||
| }); | ||
|
|
||
| expect(result).toContain('AGENT TRANSFORMATION'); | ||
| expect(result).toContain('# Agent Definition'); | ||
| expect(result).toContain('# Task Definition'); |
There was a problem hiding this comment.
The buildPrompt integration test doesn't verify that the context's checklist and template overrides are properly passed through. The buildPrompt method accepts context.checklist and context.template parameters (lines 57, 60 in source), but the test on lines 390-405 doesn't include these in the context object or verify that they work correctly in an end-to-end scenario.
| if (path.includes('tasks')) return Promise.resolve('# Task Definition'); | |
| return Promise.resolve('content'); | |
| }); | |
| const result = await builder.buildPrompt('architect', 'design.md', { | |
| creates: 'output.md', | |
| }); | |
| expect(result).toContain('AGENT TRANSFORMATION'); | |
| expect(result).toContain('# Agent Definition'); | |
| expect(result).toContain('# Task Definition'); | |
| if (path.includes('tasks')) return Promise.resolve('# Task Definition'); | |
| if (path.includes('checklists')) return Promise.resolve('OVERRIDE CHECKLIST CONTENT'); | |
| if (path.includes('templates')) return Promise.resolve('OVERRIDE TEMPLATE CONTENT'); | |
| return Promise.resolve('content'); | |
| }); | |
| const result = await builder.buildPrompt('architect', 'design.md', { | |
| creates: 'output.md', | |
| checklist: 'override-checklist.md', | |
| template: 'override-template.md', | |
| }); | |
| expect(result).toContain('AGENT TRANSFORMATION'); | |
| expect(result).toContain('# Agent Definition'); | |
| expect(result).toContain('# Task Definition'); | |
| expect(result).toContain('OVERRIDE CHECKLIST CONTENT'); | |
| expect(result).toContain('OVERRIDE TEMPLATE CONTENT'); |
|
|
||
| const result = await builder.loadTaskDefinition('missing-task'); | ||
|
|
||
| expect(result).toContain('# Task: missing-task'); |
There was a problem hiding this comment.
The console.warn spy is created but only the restore is verified, not whether console.warn was actually called. Consider adding an assertion like "expect(consoleSpy).toHaveBeenCalledTimes(1)" or "expect(consoleSpy).toHaveBeenCalledWith(expect.stringMatching(/not found/))" to ensure the warning behavior is working correctly.
| expect(result).toContain('# Task: missing-task'); | |
| expect(result).toContain('# Task: missing-task'); | |
| expect(consoleSpy).toHaveBeenCalledTimes(1); | |
| expect(consoleSpy).toHaveBeenCalledWith(expect.stringMatching(/missing-task/i)); |
| fs.readFile.mockImplementation((path) => { | ||
| if (path.includes('agents')) return Promise.resolve('# Agent Definition'); | ||
| if (path.includes('tasks')) return Promise.resolve('# Task Definition'); | ||
| return Promise.resolve('content'); |
There was a problem hiding this comment.
The mock implementation for fs.readFile in the first buildPrompt test (lines 392-396) doesn't handle all paths that buildPrompt might try to read. The method calls extractAndLoadChecklists and extractAndLoadTemplates, which may attempt to read checklist and template files. If these paths are accessed, the mock will return 'content' which may not be appropriate. While the test currently passes because no checklists/templates are loaded from frontmatter, this mock implementation is fragile and could break if the test is modified. Consider making the mock more explicit about what files exist and what they return.
| return Promise.resolve('content'); | |
| if (path.includes('checklists')) return Promise.resolve('- [ ] Checklist item'); | |
| if (path.includes('templates')) return Promise.resolve('Template content'); | |
| // For any other unexpected paths, return empty content to avoid masking errors | |
| return Promise.resolve(''); |
| fs.pathExists.mockResolvedValueOnce(true); | ||
| fs.readFile.mockResolvedValueOnce('content'); | ||
|
|
||
| await builder.loadTemplate('report.yaml'); | ||
|
|
||
| expect(fs.pathExists).toHaveBeenCalledWith(expect.stringContaining('report.yaml')); |
There was a problem hiding this comment.
This test doesn't properly verify the extension stripping behavior. According to the source code (line 230), when 'report.yaml' is passed, it should strip the extension to get 'report', then try all three extensions (.yaml, .yml, .md). However, the test only mocks one successful pathExists call, which means it succeeds on the first attempt. This doesn't test that the extension was actually stripped and re-added. To properly test this behavior, the test should either: (1) pass a template name with .yml and verify it tries .yaml first, or (2) verify all the paths that pathExists was called with to confirm the extension was stripped.
| fs.pathExists.mockResolvedValueOnce(true); | |
| fs.readFile.mockResolvedValueOnce('content'); | |
| await builder.loadTemplate('report.yaml'); | |
| expect(fs.pathExists).toHaveBeenCalledWith(expect.stringContaining('report.yaml')); | |
| fs.pathExists | |
| .mockResolvedValueOnce(false) // report.yaml not found | |
| .mockResolvedValueOnce(false) // report.yml not found | |
| .mockResolvedValueOnce(true); // report.md found | |
| fs.readFile.mockResolvedValueOnce('content'); | |
| await builder.loadTemplate('report.yaml'); | |
| expect(fs.pathExists).toHaveBeenCalledTimes(3); | |
| const firstCallPath = fs.pathExists.mock.calls[0][0]; | |
| const secondCallPath = fs.pathExists.mock.calls[1][0]; | |
| const thirdCallPath = fs.pathExists.mock.calls[2][0]; | |
| expect(firstCallPath).toEqual(expect.stringContaining('report.yaml')); | |
| expect(secondCallPath).toEqual(expect.stringContaining('report.yml')); | |
| expect(thirdCallPath).toEqual(expect.stringContaining('report.md')); |
|
|
||
| expect(result).toContain('See task definition'); | ||
| expect(result).toContain('Interactive'); | ||
| expect(result).toContain('balanced'); |
There was a problem hiding this comment.
The test for default context values is incomplete. While it verifies defaults for creates, yoloMode, and executionProfile, it doesn't verify the defaults for context.elicit (line 303 in source shows "No" when undefined) and context.executionPolicy (line 304 shows "{}" when undefined). Consider adding assertions to verify these default values as well for complete coverage.
| expect(result).toContain('balanced'); | |
| expect(result).toContain('balanced'); | |
| expect(result).toContain('No'); | |
| expect(result).toContain('{}'); |
| test('tries alternative naming pattern with underscores', async () => { | ||
| fs.pathExists | ||
| .mockResolvedValueOnce(false) // architect.md not found | ||
| .mockResolvedValueOnce(true); // architect.md (no dash to underscore here) |
There was a problem hiding this comment.
The inline comment on line 54 is misleading. The test is checking for an alternative naming pattern where dashes are replaced with underscores (e.g., "data-engineer" -> "data_engineer"), but the comment says "no dash to underscore here". This contradicts the test's purpose described in the test name "tries alternative naming pattern with underscores".
| .mockResolvedValueOnce(true); // architect.md (no dash to underscore here) | |
| .mockResolvedValueOnce(true); // data_engineer.md (dash replaced with underscore) |
|
Consolidated into #426 |
Summary
Closes #235
Add 30 unit tests for the
SubagentPromptBuilderclass in.aios-core/core/orchestration/subagent-prompt-builder.js(368 lines, previously 0% coverage).Test Coverage
Testing Approach
fs-extraandjs-yamlfor isolationjest.resetAllMocks()between testsAll 30 tests passing.
Summary by CodeRabbit