Add unit tests for agent-invoker module#260
Add unit tests for agent-invoker module#260nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
|
@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. |
WalkthroughA comprehensive test suite is added for the AgentInvoker module, verifying constructor behavior, agent and task invocation workflows, internal methods for agent loading and metadata parsing, error handling, output validation, agent discovery, and audit trail functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
Adds a dedicated unit test suite for the .aios-core/core/orchestration/agent-invoker module to validate core orchestration behaviors (agent/task loading, metadata parsing, output validation, and invocation audit helpers).
Changes:
- Add
AgentInvokerunit tests covering constants, constructor defaults,invokeAgenthappy/failure paths, and internal helpers. - Mock
fs-extraandjs-yamlto avoid filesystem/YAML parsing dependencies during unit tests. - Add tests for audit/history query helpers (
getInvocations*,getInvocationSummary,getLogs,clearInvocations).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Unit tests for agent-invoker module | ||
| * | ||
| * Tests the AgentInvoker class that provides an interface to invoke | ||
| * agents for tasks during orchestration, with retry logic and audit logging. | ||
| */ |
There was a problem hiding this comment.
The file header says these tests cover “retry logic”, but this test suite doesn’t currently exercise retry behavior (e.g., _executeWithRetry retry count/backoff or invocation.retries updates) or timeout behavior (_executeWithTimeout). Either add targeted tests for these behaviors or adjust the header comment/PR description to match what’s actually covered.
|
|
||
| test('strips @ prefix from agent name', async () => { | ||
| const result = await invoker.invokeAgent('@dev', 'my-task'); | ||
| expect(result.success).toBe(true); |
There was a problem hiding this comment.
This test name says it “strips @ prefix”, but the assertion only checks success. Either rename it to reflect the behavior being tested (e.g., that @dev is accepted) or assert on a concrete normalized outcome (such as the returned/recorded agent name) if that’s the intended contract.
| expect(result.success).toBe(true); | |
| expect(result.success).toBe(true); | |
| expect(result.agentName).toBe('dev'); |
| fs.pathExists.mockResolvedValue(false); | ||
|
|
||
| const result = await invoker.invokeAgent('dev', 'nonexistent-task'); | ||
|
|
||
| // Agent also not found since pathExists returns false | ||
| expect(result.success).toBe(false); |
There was a problem hiding this comment.
The setup here makes fs.pathExists return false for both the agent file and the task file, so this test doesn’t isolate the “task not found” case. Also, the inline comment “Agent also not found…” is inaccurate because _loadAgent('dev') returns a supported agent config even when the file is missing. Consider using mockResolvedValueOnce(...) to make the agent path exist and the task path not exist, and assert that the error contains “Task not found”.
| fs.pathExists.mockResolvedValue(false); | |
| const result = await invoker.invokeAgent('dev', 'nonexistent-task'); | |
| // Agent also not found since pathExists returns false | |
| expect(result.success).toBe(false); | |
| fs.pathExists | |
| .mockResolvedValueOnce(true) // agent path exists | |
| .mockResolvedValueOnce(false); // task path does not exist | |
| const result = await invoker.invokeAgent('dev', 'nonexistent-task'); | |
| expect(result.success).toBe(false); | |
| expect(result.error).toContain('Task not found'); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/core/orchestration/agent-invoker.test.js (2)
157-165: Consider asserting on the event payload for consistency.The
invocationCompletetest (line 151) verifies the payload shape (agentName,status), butinvocationFailedonly checkstoHaveBeenCalled(). Adding a similarobjectContainingassertion here would strengthen the test and keep the event-test pattern consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/agent-invoker.test.js` around lines 157 - 165, The test for 'emits invocationFailed on failure' only checks that the handler was called; update it to assert the event payload shape like the 'invocationComplete' test by verifying the handler was called with an object containing at least agentName and status (e.g. agentName === 'unknown-agent' and a failure status), using Jest's objectContaining matcher against the mock handler used in the test (handler) after invoking invoker.invokeAgent('unknown-agent', 'task').
433-440: Minor: indirect assertion ingetLogs returns copytest.Line 439 asserts
invoker.logs.length < logs.length, which only proves the pushed fake entry didn't propagate. A more explicit assertion would capture the original length before mutation and compare against it, making the test intent clearer at a glance:Suggested improvement
test('getLogs returns copy', async () => { await invoker.invokeAgent('dev', 'task'); const logs = invoker.getLogs(); + const originalLength = logs.length; logs.push({ fake: true }); - expect(invoker.logs.length).toBeLessThan(logs.length); + expect(invoker.logs).toHaveLength(originalLength); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/agent-invoker.test.js` around lines 433 - 440, In the "getLogs returns copy" test, make the assertion explicit by storing the original length from invoker.getLogs() (or invoker.logs) before mutating the returned array and then asserting that invoker.logs.length equals that original length after pushing the fake entry; i.e., call invoker.invokeAgent('dev','task'), const originalLength = invoker.logs.length (or const logs = invoker.getLogs(); const originalLength = logs.length), mutate the returned logs, and assert invoker.logs.length === originalLength to prove getLogs returned a copy rather than using an indirect length comparison.
🤖 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/agent-invoker.test.js`:
- Around line 128-135: The test "returns failure when task not found" currently
mocks fs.pathExists globally to false which triggers the Unknown agent path in
_loadAgent; change the mock to isolate the case by using
fs.pathExists.mockResolvedValueOnce(true) for the agent file existence check and
then fs.pathExists.mockResolvedValueOnce(false) (or mockResolvedValue(false)
after the first call) for the task file check so that invoker.invokeAgent('dev',
'nonexistent-task') exercises the valid-agent + missing-task path and asserts
result.success is false.
---
Nitpick comments:
In `@tests/core/orchestration/agent-invoker.test.js`:
- Around line 157-165: The test for 'emits invocationFailed on failure' only
checks that the handler was called; update it to assert the event payload shape
like the 'invocationComplete' test by verifying the handler was called with an
object containing at least agentName and status (e.g. agentName ===
'unknown-agent' and a failure status), using Jest's objectContaining matcher
against the mock handler used in the test (handler) after invoking
invoker.invokeAgent('unknown-agent', 'task').
- Around line 433-440: In the "getLogs returns copy" test, make the assertion
explicit by storing the original length from invoker.getLogs() (or invoker.logs)
before mutating the returned array and then asserting that invoker.logs.length
equals that original length after pushing the fake entry; i.e., call
invoker.invokeAgent('dev','task'), const originalLength = invoker.logs.length
(or const logs = invoker.getLogs(); const originalLength = logs.length), mutate
the returned logs, and assert invoker.logs.length === originalLength to prove
getLogs returned a copy rather than using an indirect length comparison.
| test('returns failure when task not found', async () => { | ||
| fs.pathExists.mockResolvedValue(false); | ||
|
|
||
| const result = await invoker.invokeAgent('dev', 'nonexistent-task'); | ||
|
|
||
| // Agent also not found since pathExists returns false | ||
| expect(result.success).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "agent-invoker.js" --type fRepository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
git ls-files | grep -E "agent-invoker|orchestration" | head -20Repository: SynkraAI/aios-core
Length of output: 1139
🏁 Script executed:
cat -n ".aios-core/core/orchestration/agent-invoker.js"Repository: SynkraAI/aios-core
Length of output: 19915
🏁 Script executed:
sed -n '120,140p' tests/core/orchestration/agent-invoker.test.jsRepository: SynkraAI/aios-core
Length of output: 747
Test doesn't properly isolate the "task not found" scenario.
With fs.pathExists.mockResolvedValue(false) applied globally, the agent file check fails first (at _loadAgent line 230), causing the method to throw "Unknown agent" before ever reaching the task file lookup. This test actually exercises the "unknown agent" path rather than testing the specific case where a valid agent exists but the task file is missing.
Consider mocking pathExists to return true for the agent file check and false for the task file check using mockResolvedValueOnce:
Suggested fix
test('returns failure when task not found', async () => {
- fs.pathExists.mockResolvedValue(false);
+ fs.pathExists
+ .mockResolvedValueOnce(true) // agent file check passes
+ .mockResolvedValueOnce(false); // task file check fails
const result = await invoker.invokeAgent('dev', 'nonexistent-task');
- // Agent also not found since pathExists returns false
expect(result.success).toBe(false);
+ expect(result.error).toContain('Task not found');
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/orchestration/agent-invoker.test.js` around lines 128 - 135, The
test "returns failure when task not found" currently mocks fs.pathExists
globally to false which triggers the Unknown agent path in _loadAgent; change
the mock to isolate the case by using fs.pathExists.mockResolvedValueOnce(true)
for the agent file existence check and then
fs.pathExists.mockResolvedValueOnce(false) (or mockResolvedValue(false) after
the first call) for the task file check so that invoker.invokeAgent('dev',
'nonexistent-task') exercises the valid-agent + missing-task path and asserts
result.success is false.
|
Consolidated into #426 |
Summary
agent-invokerorchestration moduleTest Coverage
Test plan
Summary by CodeRabbit
Closes #301