test: add unit tests for cli-commands module#246
test: add unit tests for cli-commands module#246nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
Add 24 tests covering orchestrator CLI commands: - orchestrate: full pipeline, epic flag, dry run, blocked/failed results - orchestrate-status: state display, missing state, read errors - orchestrate-stop: state update to stopped, missing state, write errors - orchestrate-resume: resume from current/next epic, completed check - All commands validate storyId requirement
|
@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. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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
This PR adds comprehensive unit test coverage for the CLI commands module that handles orchestrator control operations. The module was previously untested (0% coverage for 580 lines).
Changes:
- Adds 24 unit tests covering all four CLI command handlers (orchestrate, orchestrate-status, orchestrate-stop, orchestrate-resume)
- Tests validation, success/error scenarios, state transitions, and exit codes
- Implements proper mocking of fs-extra and master-orchestrator dependencies
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test('exports all command functions', () => { | ||
| expect(typeof orchestrate).toBe('function'); | ||
| expect(typeof orchestrateStatus).toBe('function'); | ||
| expect(typeof orchestrateStop).toBe('function'); | ||
| expect(typeof orchestrateResume).toBe('function'); | ||
| }); | ||
|
|
||
| test('exports commands map', () => { |
There was a problem hiding this comment.
The codebase consistently uses it() instead of test() for test case definitions. All other test files in tests/core and tests/core/orchestration use the it() pattern (e.g., tests/core/master-orchestrator.test.js, tests/core/orchestration/lock-manager.test.js). Please update all test() calls to it() to maintain consistency with the established convention.
| test('exports all command functions', () => { | |
| expect(typeof orchestrate).toBe('function'); | |
| expect(typeof orchestrateStatus).toBe('function'); | |
| expect(typeof orchestrateStop).toBe('function'); | |
| expect(typeof orchestrateResume).toBe('function'); | |
| }); | |
| test('exports commands map', () => { | |
| it('exports all command functions', () => { | |
| expect(typeof orchestrate).toBe('function'); | |
| expect(typeof orchestrateStatus).toBe('function'); | |
| expect(typeof orchestrateStop).toBe('function'); | |
| expect(typeof orchestrateResume).toBe('function'); | |
| }); | |
| it('exports commands map', () => { |
| MasterOrchestrator.mockImplementation(() => mockOrchestrator); | ||
|
|
||
| const result = await orchestrate('story-1', { projectRoot: '/p' }); | ||
|
|
There was a problem hiding this comment.
For consistency with other test assertions, consider also checking that result.success is false when testing the failed result scenario. Other similar tests check both exitCode and success.
| expect(result.success).toBe(false); |
| test('returns error for empty storyId', async () => { | ||
| const result = await orchestrate(''); | ||
| expect(result.success).toBe(false); | ||
| expect(result.exitCode).toBe(3); |
There was a problem hiding this comment.
For consistency with the test on line 59-64, consider also checking result.error to ensure it contains an appropriate message about Story ID being required.
| expect(result.exitCode).toBe(3); | |
| expect(result.exitCode).toBe(3); | |
| expect(result.error).toContain('Story ID'); |
|
Consolidated into #426 |
Summary
Closes #245
Add 24 unit tests for the CLI command handlers in
.aios-core/core/orchestration/cli-commands.js(580 lines, previously 0% coverage).Test Coverage
Testing Approach
fs-extraandmaster-orchestratorfor full isolationjest.spyOn(console, 'log')All 24 tests passing.