test: add deep coverage for orchestration modules (P2 priority)#468
Open
nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Open
test: add deep coverage for orchestration modules (P2 priority)#468nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
Extends gate-evaluator.test.js with 45 new tests covering: - _loadConfig() filesystem loading with YAML parsing and fallback - _getGateKey() and _getDefaultChecks() for all epic transitions - _determineVerdict() edge cases (allowMinorIssues, severity levels) - Config-based checks (min_score, require_tests, min_coverage) - Check-specific edge cases (artifacts, codeChanges, testResults) - Error handling, logging, and score calculation Extends bob-orchestrator.test.js with 30 new tests covering: - _resolveStoryPath() path resolution (active dir, root, normalization) - handleBrownfieldDecision() delegation - handleBrownfieldPhaseFailure() delegation - handlePostDiscoveryChoice() delegation - handleGreenfieldSurfaceDecision() delegation - handleGreenfieldPhaseFailure() delegation - _routeByState() unknown state handling - _checkExistingSession() elapsed time formatting edge cases - Constructor initialization of all Story 12.x dependencies Refs SynkraAI#52
nikolasdehor
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
@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 comprehensive test coverage for GateEvaluator and BobOrchestrator modules, covering edge cases, error handling, configuration behavior, verdict determination logic, and integration scenarios across 942 new test lines. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/core/gate-evaluator.test.js (1)
770-784: The monkey-patch restoration is safe here, but atry/finallywould be more defensive.Since
evaluatoris recreated inbeforeEach, a failed assertion before line 782 won't leak into other tests. That said, wrapping the assertion + restore intry/finallyis a low-cost safety net if the test structure ever changes.✏️ Optional: wrap in try/finally
it('should return BLOCKED when _runGateChecks throws', async () => { const original = evaluator._runGateChecks; evaluator._runGateChecks = () => { throw new Error('check explosion'); }; - const result = await evaluator.evaluate(3, 4, {}); - - expect(result.verdict).toBe(GateVerdict.BLOCKED); - expect(result.issues).toHaveLength(1); - expect(result.issues[0].severity).toBe('critical'); - expect(result.issues[0].message).toContain('check explosion'); - - evaluator._runGateChecks = original; + try { + const result = await evaluator.evaluate(3, 4, {}); + + expect(result.verdict).toBe(GateVerdict.BLOCKED); + expect(result.issues).toHaveLength(1); + expect(result.issues[0].severity).toBe('critical'); + expect(result.issues[0].message).toContain('check explosion'); + } finally { + evaluator._runGateChecks = original; + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/gate-evaluator.test.js` around lines 770 - 784, The test temporarily replaces evaluator._runGateChecks and restores it manually; make this defensive by saving the original as done and wrapping the invocation/assertions and the restore in a try/finally so the original _runGateChecks is always restored even if an assertion throws. Locate the test case that assigns evaluator._runGateChecks (the 'should return BLOCKED when _runGateChecks throws' it block) and change the structure to use try { ... assertions ... } finally { evaluator._runGateChecks = original; } to ensure restoration.tests/core/orchestration/bob-orchestrator.test.js (2)
1236-1353: Consider extracting the repeated mock session factory.Each
_checkExistingSessionedge-case test builds a near-identicalmockSessionStateobject and re-stubs the same four mock methods. A small helper likesetupMockSession(overrides)would cut the boilerplate substantially while keeping each test's intent clear. Not blocking—test isolation is valuable—but worth considering as this section grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/bob-orchestrator.test.js` around lines 1236 - 1353, Create a small helper (e.g., setupMockSession) to centralize constructing mockSessionState and stubbing orchestrator.sessionState.exists, loadSessionState, detectCrash, and getResumeSummary for the _checkExistingSession tests; the helper should accept an overrides object to modify session_state fields (last_updated, epic, progress, workflow) and return or apply the stubs so each test only calls setupMockSession({ last_updated: ..., workflow: ... }) then invokes orchestrator._checkExistingSession(), replacing the repeated mock construction and jest.fn() calls scattered across the tests.
1291-1316: Misleading variable name:oneHourAgois actually 90 minutes ago.The variable is named
oneHourAgobutsetMinutes(getMinutes() - 90)shifts the clock by 90 minutes, not 60. The test asserts'1 hora'(singular hour), which likely works because the production code floors toMath.floor(1.5) === 1. The test logic is correct, but the name is confusing. Consider renaming toninetyMinutesAgoor adding a brief comment.✏️ Suggested rename
- it('should use singular form for 1 hour', async () => { - // Given - const oneHourAgo = new Date(); - oneHourAgo.setMinutes(oneHourAgo.getMinutes() - 90); + it('should use singular form for 1 hour', async () => { + // Given — 90 minutes ago floors to 1 hour + const ninetyMinutesAgo = new Date(); + ninetyMinutesAgo.setMinutes(ninetyMinutesAgo.getMinutes() - 90);And update the reference on line 1299:
- last_updated: oneHourAgo.toISOString(), + last_updated: ninetyMinutesAgo.toISOString(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/bob-orchestrator.test.js` around lines 1291 - 1316, Rename the misleading test variable oneHourAgo to ninetyMinutesAgo (or add a clarifying comment) in the test that calls orchestrator._checkExistingSession; update the declaration and any subsequent references (e.g., where oneHourAgo.setMinutes(...) is used and where mockSessionState.last_updated is set) so the variable name matches the 90-minute offset and clarifies intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/gate-evaluator.test.js`:
- Around line 770-784: The test temporarily replaces evaluator._runGateChecks
and restores it manually; make this defensive by saving the original as done and
wrapping the invocation/assertions and the restore in a try/finally so the
original _runGateChecks is always restored even if an assertion throws. Locate
the test case that assigns evaluator._runGateChecks (the 'should return BLOCKED
when _runGateChecks throws' it block) and change the structure to use try { ...
assertions ... } finally { evaluator._runGateChecks = original; } to ensure
restoration.
In `@tests/core/orchestration/bob-orchestrator.test.js`:
- Around line 1236-1353: Create a small helper (e.g., setupMockSession) to
centralize constructing mockSessionState and stubbing
orchestrator.sessionState.exists, loadSessionState, detectCrash, and
getResumeSummary for the _checkExistingSession tests; the helper should accept
an overrides object to modify session_state fields (last_updated, epic,
progress, workflow) and return or apply the stubs so each test only calls
setupMockSession({ last_updated: ..., workflow: ... }) then invokes
orchestrator._checkExistingSession(), replacing the repeated mock construction
and jest.fn() calls scattered across the tests.
- Around line 1291-1316: Rename the misleading test variable oneHourAgo to
ninetyMinutesAgo (or add a clarifying comment) in the test that calls
orchestrator._checkExistingSession; update the declaration and any subsequent
references (e.g., where oneHourAgo.setMinutes(...) is used and where
mockSessionState.last_updated is set) so the variable name matches the 90-minute
offset and clarifies intent.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for two critical orchestration modules as part of addressing the coverage gap identified in issue #52 (ADE implementation added 108k lines, dropping coverage below thresholds).
Changes:
- Added 45 tests for gate-evaluator.js covering all private methods, edge cases, error handling, and scoring logic
- Added 30 tests for bob-orchestrator.js covering delegation patterns, path resolution, elapsed time formatting, and constructor initialization
- Tests follow existing Given/When/Then structure and mocking patterns
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/core/gate-evaluator.test.js | Adds deep coverage for _loadConfig, _getGateKey, _getDefaultChecks, _determineVerdict, config-based checks, check-specific edge cases, error handling, logging, score calculation, and summary aggregation |
| tests/core/orchestration/bob-orchestrator.test.js | Adds deep coverage for _resolveStoryPath, brownfield/greenfield delegation methods, _routeByState, _checkExistingSession elapsed time formatting, and constructor initialization of Story 12.x dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1162
to
+1215
| describe('handleGreenfieldSurfaceDecision', () => { | ||
| it('should delegate GO decision to greenfieldHandler', async () => { | ||
| // Given | ||
| const mockResult = { action: 'phase_started', phase: 2 }; | ||
| orchestrator.greenfieldHandler.handleSurfaceDecision = jest.fn().mockResolvedValue(mockResult); | ||
|
|
||
| // When | ||
| const result = await orchestrator.handleGreenfieldSurfaceDecision('GO', 2); | ||
|
|
||
| // Then | ||
| expect(orchestrator.greenfieldHandler.handleSurfaceDecision).toHaveBeenCalledWith('GO', 2, {}); | ||
| expect(result).toEqual(mockResult); | ||
| }); | ||
|
|
||
| it('should pass context to greenfieldHandler', async () => { | ||
| // Given | ||
| const ctx = { feedback: 'looks good' }; | ||
| orchestrator.greenfieldHandler.handleSurfaceDecision = jest.fn().mockResolvedValue({}); | ||
|
|
||
| // When | ||
| await orchestrator.handleGreenfieldSurfaceDecision('PAUSE', 3, ctx); | ||
|
|
||
| // Then | ||
| expect(orchestrator.greenfieldHandler.handleSurfaceDecision).toHaveBeenCalledWith('PAUSE', 3, ctx); | ||
| }); | ||
| }); | ||
|
|
||
| describe('handleGreenfieldPhaseFailure', () => { | ||
| it('should delegate retry action to greenfieldHandler', async () => { | ||
| // Given | ||
| const mockResult = { action: 'phase_retried' }; | ||
| orchestrator.greenfieldHandler.handlePhaseFailureAction = jest.fn().mockResolvedValue(mockResult); | ||
|
|
||
| // When | ||
| const result = await orchestrator.handleGreenfieldPhaseFailure('init', 'retry'); | ||
|
|
||
| // Then | ||
| expect(orchestrator.greenfieldHandler.handlePhaseFailureAction).toHaveBeenCalledWith('init', 'retry', {}); | ||
| expect(result).toEqual(mockResult); | ||
| }); | ||
|
|
||
| it('should delegate abort action with context', async () => { | ||
| // Given | ||
| const ctx = { reason: 'user cancelled' }; | ||
| orchestrator.greenfieldHandler.handlePhaseFailureAction = jest.fn().mockResolvedValue({ action: 'aborted' }); | ||
|
|
||
| // When | ||
| const result = await orchestrator.handleGreenfieldPhaseFailure('scaffold', 'abort', ctx); | ||
|
|
||
| // Then | ||
| expect(orchestrator.greenfieldHandler.handlePhaseFailureAction).toHaveBeenCalledWith('scaffold', 'abort', ctx); | ||
| expect(result.action).toBe('aborted'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The GreenfieldHandler module is not mocked in the test setup. While the tests override individual methods on the handler instance (which works), it would be more consistent with the existing test patterns to add a jest.mock for greenfield-handler at the module level, similar to how BrownfieldHandler is mocked on lines 42-58. This would prevent potential issues if the GreenfieldHandler constructor has side effects or dependencies that could affect test reliability.
This was referenced Mar 3, 2026
This was referenced Mar 11, 2026
Contributor
Author
|
@Pedrovaleriolopez PR aprovado pelo CodeRabbit, pronto para merge! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resumo
Plano de teste