Add unit tests for circuit-breaker module#269
Add unit tests for circuit-breaker module#269nikolasdehor 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. |
WalkthroughAdds a new test file with 22 unit tests for the CircuitBreaker module, covering state constants, constructor initialization, state transitions (CLOSED/OPEN/HALF_OPEN), success/failure recording, statistics retrieval, reset behavior, and complete lifecycle scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
There was a problem hiding this comment.
Pull request overview
Adds dedicated Jest unit coverage for the IDS CircuitBreaker implementation to validate state transitions (CLOSED/OPEN/HALF_OPEN), counters, and lifecycle behavior.
Changes:
- Introduces a new
circuit-breaker.test.jssuite with 22 unit tests. - Covers constants/defaults, constructor options,
isAllowed,recordSuccess,recordFailure,getStats,reset, and end-to-end lifecycle transitions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(breaker.getState()).toBe(STATE_OPEN); | ||
|
|
||
| // Simulate timeout passing | ||
| breaker._lastFailureTime = Date.now() - 61000; |
There was a problem hiding this comment.
61000 is a magic number tied to the default reset timeout (60000ms). To keep the test resilient if defaults change, compute this using DEFAULT_RESET_TIMEOUT_MS (e.g. Date.now() - (DEFAULT_RESET_TIMEOUT_MS + 1000)) rather than hard-coding 61000.
| breaker._lastFailureTime = Date.now() - 61000; | |
| breaker._lastFailureTime = Date.now() - (DEFAULT_RESET_TIMEOUT_MS + 1000); |
| // 4. Success probe closes circuit | ||
| breaker.recordSuccess(); | ||
| breaker._halfOpenProbeInFlight = true; | ||
| breaker.recordSuccess(); | ||
| breaker._halfOpenProbeInFlight = true; |
There was a problem hiding this comment.
In this lifecycle test, manually toggling _halfOpenProbeInFlight between recordSuccess() calls bypasses the actual probe-gating logic in isAllowed() and doesn’t exercise the intended HALF_OPEN flow. Consider calling isAllowed() before each probe attempt (or using fake timers) so each success corresponds to an allowed probe and the test validates the real state machine behavior end-to-end.
| // 4. Success probe closes circuit | |
| breaker.recordSuccess(); | |
| breaker._halfOpenProbeInFlight = true; | |
| breaker.recordSuccess(); | |
| breaker._halfOpenProbeInFlight = true; | |
| // 4. Success probes close circuit via allowed HALF_OPEN probes | |
| expect(breaker.isAllowed()).toBe(true); | |
| breaker.recordSuccess(); | |
| expect(breaker.isAllowed()).toBe(true); | |
| breaker.recordSuccess(); | |
| expect(breaker.isAllowed()).toBe(true); |
| breaker._state = STATE_HALF_OPEN; | ||
| breaker._halfOpenProbeInFlight = false; | ||
|
|
||
| expect(breaker.isAllowed()).toBe(true); |
There was a problem hiding this comment.
These assertions set _state / _halfOpenProbeInFlight directly to arrange HALF_OPEN behavior. To keep tests aligned with the public contract, prefer reaching HALF_OPEN via recordFailure() + elapsed timeout + isAllowed() (or fake timers) rather than writing internal fields.
| breaker._state = STATE_HALF_OPEN; | |
| breaker._halfOpenProbeInFlight = false; | |
| expect(breaker.isAllowed()).toBe(true); | |
| // Trip the breaker to OPEN | |
| for (let i = 0; i < 5; i++) breaker.recordFailure(); | |
| expect(breaker.getState()).toBe(STATE_OPEN); | |
| // Simulate timeout passing to allow transition to HALF_OPEN | |
| breaker._lastFailureTime = Date.now() - 61000; | |
| // First request after timeout should be allowed (probe) and move to HALF_OPEN | |
| expect(breaker.isAllowed()).toBe(true); | |
| expect(breaker.getState()).toBe(STATE_HALF_OPEN); | |
| // Second request while probe is in flight should be blocked |
| * Tests the CircuitBreaker pattern implementation with | ||
| * CLOSED, OPEN, and HALF_OPEN states. | ||
| */ | ||
|
|
There was a problem hiding this comment.
All other IDS test files in this repo start with 'use strict'; (e.g. tests/core/ids/framework-governor.test.js, entity-registry-schema.test.js). To match the established test-file convention and avoid subtle strict-mode differences, add 'use strict'; as the first statement in this file.
| 'use strict'; |
| for (let i = 0; i < 5; i++) breaker.recordFailure(); | ||
| expect(breaker.getState()).toBe(STATE_OPEN); | ||
|
|
||
| // Simulate timeout passing | ||
| breaker._lastFailureTime = Date.now() - 61000; | ||
|
|
||
| expect(breaker.isAllowed()).toBe(true); | ||
| expect(breaker.getState()).toBe(STATE_HALF_OPEN); |
There was a problem hiding this comment.
This test forces the OPEN→HALF_OPEN transition by directly mutating the internal _lastFailureTime. That makes the test brittle to implementation refactors. Prefer advancing time via Jest fake timers (jest.useFakeTimers() + jest.setSystemTime() / jest.advanceTimersByTime()) or using a shorter resetTimeoutMs option and real timers, as done in other IDS tests.
| for (let i = 0; i < 5; i++) breaker.recordFailure(); | |
| expect(breaker.getState()).toBe(STATE_OPEN); | |
| // Simulate timeout passing | |
| breaker._lastFailureTime = Date.now() - 61000; | |
| expect(breaker.isAllowed()).toBe(true); | |
| expect(breaker.getState()).toBe(STATE_HALF_OPEN); | |
| // Use fake timers to simulate the reset timeout elapsing | |
| jest.useFakeTimers().setSystemTime(new Date('2020-01-01T00:00:00Z')); | |
| for (let i = 0; i < 5; i++) breaker.recordFailure(); | |
| expect(breaker.getState()).toBe(STATE_OPEN); | |
| // Advance time past the default reset timeout (60s) | |
| jest.setSystemTime(new Date('2020-01-01T00:01:01Z')); | |
| expect(breaker.isAllowed()).toBe(true); | |
| expect(breaker.getState()).toBe(STATE_HALF_OPEN); | |
| jest.useRealTimers(); |
![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 (1)
tests/core/ids/circuit-breaker.test.js (1)
84-101: Considerjest.useFakeTimers()instead of mutating_lastFailureTime.Both the timeout transition test (line 89) and the probe-gating test (lines 96–97) directly mutate private implementation fields (
_lastFailureTime,_state,_halfOpenProbeInFlight) to set up state. This creates tight coupling to field names and skips the publicisAllowed()probe-entry path.Using fake timers would remove the need for
_lastFailureTimemutation, and the public trip → advance-time →isAllowed()sequence would correctly set up_halfOpenProbeInFlightwithout direct assignment.♻️ Suggested approach using fake timers
+ beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); test('transitions to HALF_OPEN after reset timeout', () => { for (let i = 0; i < 5; i++) breaker.recordFailure(); expect(breaker.getState()).toBe(STATE_OPEN); - // Simulate timeout passing - breaker._lastFailureTime = Date.now() - 61000; + jest.advanceTimersByTime(61000); expect(breaker.isAllowed()).toBe(true); expect(breaker.getState()).toBe(STATE_HALF_OPEN); }); test('allows one probe in HALF_OPEN', () => { - breaker._state = STATE_HALF_OPEN; - breaker._halfOpenProbeInFlight = false; + for (let i = 0; i < 5; i++) breaker.recordFailure(); + jest.advanceTimersByTime(61000); + breaker.isAllowed(); // transitions to HALF_OPEN, probe in flight + // Now advance time again so second isAllowed call doesn't re-enter via timeout + // (state is already HALF_OPEN; probe is in flight) expect(breaker.isAllowed()).toBe(true); expect(breaker.isAllowed()).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/ids/circuit-breaker.test.js` around lines 84 - 101, Replace direct private-field manipulation in the tests with public API + fake timers: call breaker.recordFailure() until it trips to STATE_OPEN, then use jest.useFakeTimers() and jest.advanceTimersByTime(61000) (or the circuit's reset timeout) before calling breaker.isAllowed() to trigger the HALF_OPEN transition instead of setting breaker._lastFailureTime; for the probe-gating test, set breaker._state to STATE_HALF_OPEN only if you can via public behavior (e.g., trip → advance timers → isAllowed()) and then call breaker.isAllowed() twice to assert the first probe is allowed and the second is blocked, removing direct mutations of _state and _halfOpenProbeInFlight and relying on breaker.recordFailure(), breaker.isAllowed(), STATE_OPEN and STATE_HALF_OPEN to drive state.
🤖 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/ids/circuit-breaker.test.js`:
- Around line 204-213: The test "resets to CLOSED state" omits assertions for
stats fields lastFailureTime and totalTrips after calling breaker.reset();
update the test to call breaker.getStats() and assert that stats.lastFailureTime
is null and stats.totalTrips matches the intended contract (either 0 if reset
should clear trips or preserved value if spec dictates) so the reset() behavior
for lastFailureTime and totalTrips is explicitly verified alongside getState()
and counts; reference the breaker.reset(), breaker.getStats(),
stats.lastFailureTime, and stats.totalTrips symbols when making the assertions.
- Around line 234-238: The test currently bypasses the public gating logic by
setting breaker._halfOpenProbeInFlight = true directly between
breaker.recordSuccess() calls; change the test to drive probe cycles via the
public isAllowed() method instead (i.e., call breaker.isAllowed() before each
simulated service call and only then call breaker.recordSuccess()), so the
HALF_OPEN gating logic is exercised rather than mutating the internal
_halfOpenProbeInFlight flag directly.
---
Nitpick comments:
In `@tests/core/ids/circuit-breaker.test.js`:
- Around line 84-101: Replace direct private-field manipulation in the tests
with public API + fake timers: call breaker.recordFailure() until it trips to
STATE_OPEN, then use jest.useFakeTimers() and jest.advanceTimersByTime(61000)
(or the circuit's reset timeout) before calling breaker.isAllowed() to trigger
the HALF_OPEN transition instead of setting breaker._lastFailureTime; for the
probe-gating test, set breaker._state to STATE_HALF_OPEN only if you can via
public behavior (e.g., trip → advance timers → isAllowed()) and then call
breaker.isAllowed() twice to assert the first probe is allowed and the second is
blocked, removing direct mutations of _state and _halfOpenProbeInFlight and
relying on breaker.recordFailure(), breaker.isAllowed(), STATE_OPEN and
STATE_HALF_OPEN to drive state.
| test('resets to CLOSED state', () => { | ||
| for (let i = 0; i < 5; i++) breaker.recordFailure(); | ||
| expect(breaker.getState()).toBe(STATE_OPEN); | ||
|
|
||
| breaker.reset(); | ||
|
|
||
| expect(breaker.getState()).toBe(STATE_CLOSED); | ||
| expect(breaker.getStats().failureCount).toBe(0); | ||
| expect(breaker.getStats().successCount).toBe(0); | ||
| }); |
There was a problem hiding this comment.
reset test is missing assertions for lastFailureTime and totalTrips.
After calling reset(), neither lastFailureTime (expected null) nor totalTrips (whatever the contract is — preserved or zeroed) is verified. Both are part of the getStats() surface and their post-reset values are an important behavioral contract.
🛡️ Proposed additional assertions
breaker.reset();
expect(breaker.getState()).toBe(STATE_CLOSED);
expect(breaker.getStats().failureCount).toBe(0);
expect(breaker.getStats().successCount).toBe(0);
+ expect(breaker.getStats().lastFailureTime).toBeNull();
+ // totalTrips: assert the expected value (0 if reset() clears it, 1 if it's preserved)
+ expect(breaker.getStats().totalTrips).toBe(/* 0 or 1 per implementation contract */);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/ids/circuit-breaker.test.js` around lines 204 - 213, The test
"resets to CLOSED state" omits assertions for stats fields lastFailureTime and
totalTrips after calling breaker.reset(); update the test to call
breaker.getStats() and assert that stats.lastFailureTime is null and
stats.totalTrips matches the intended contract (either 0 if reset should clear
trips or preserved value if spec dictates) so the reset() behavior for
lastFailureTime and totalTrips is explicitly verified alongside getState() and
counts; reference the breaker.reset(), breaker.getStats(),
stats.lastFailureTime, and stats.totalTrips symbols when making the assertions.
| breaker.recordSuccess(); | ||
| breaker._halfOpenProbeInFlight = true; | ||
| breaker.recordSuccess(); | ||
| breaker._halfOpenProbeInFlight = true; | ||
| breaker.recordSuccess(); |
There was a problem hiding this comment.
Lifecycle test bypasses isAllowed() for the 2nd and 3rd probe cycles.
After the first recordSuccess() at line 234 (which follows the legitimate isAllowed() at line 230), the test manually resets _halfOpenProbeInFlight = true at lines 235 and 237 instead of calling isAllowed() again. This skips exercising whether isAllowed() correctly re-gates probes after a success in HALF_OPEN. A bug where isAllowed() in HALF_OPEN mis-gates subsequent probes would not be caught.
The probe cycle should follow the real usage pattern: isAllowed() → service call → recordSuccess().
🛡️ Corrected probe sequence for the lifecycle test
// 4. Success probe closes circuit
- breaker.recordSuccess();
- breaker._halfOpenProbeInFlight = true;
- breaker.recordSuccess();
- breaker._halfOpenProbeInFlight = true;
- breaker.recordSuccess();
+ // Probe 1: isAllowed() was already called above (line 230), probe is in flight
+ breaker.recordSuccess(); // success 1
+ // Probe 2
+ expect(breaker.isAllowed()).toBe(true); // should allow next probe
+ breaker.recordSuccess(); // success 2
+ // Probe 3
+ expect(breaker.isAllowed()).toBe(true); // should allow next probe
+ breaker.recordSuccess(); // success 3 → closes circuit
expect(breaker.getState()).toBe(STATE_CLOSED);📝 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.
| breaker.recordSuccess(); | |
| breaker._halfOpenProbeInFlight = true; | |
| breaker.recordSuccess(); | |
| breaker._halfOpenProbeInFlight = true; | |
| breaker.recordSuccess(); | |
| // Probe 1: isAllowed() was already called above (line 230), probe is in flight | |
| breaker.recordSuccess(); // success 1 | |
| // Probe 2 | |
| expect(breaker.isAllowed()).toBe(true); // should allow next probe | |
| breaker.recordSuccess(); // success 2 | |
| // Probe 3 | |
| expect(breaker.isAllowed()).toBe(true); // should allow next probe | |
| breaker.recordSuccess(); // success 3 → closes circuit | |
| expect(breaker.getState()).toBe(STATE_CLOSED); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/ids/circuit-breaker.test.js` around lines 234 - 238, The test
currently bypasses the public gating logic by setting
breaker._halfOpenProbeInFlight = true directly between breaker.recordSuccess()
calls; change the test to drive probe cycles via the public isAllowed() method
instead (i.e., call breaker.isAllowed() before each simulated service call and
only then call breaker.recordSuccess()), so the HALF_OPEN gating logic is
exercised rather than mutating the internal _halfOpenProbeInFlight flag
directly.
|
Consolidated into #426 |
Closes #268
Summary
circuit-breakerIDS moduleTest Coverage
Test plan
Summary by CodeRabbit