Repository navigation
test(circuit-breaker): verify open circuit does not call the service and fix overflowed timestamp - #3619
test(circuit-breaker): verify open circuit does not call the service and fix overflowed timestamp#3619renanmpimentel wants to merge 2 commits into
Conversation
- Add a test with a call-counting service: once the circuit is open, attemptRequest() must return the cached failure without calling the service again. Previously a fallback and a real failing call returned the same string, so ignoring the OPEN state went undetected. - testEvaluateState computed "a failure long ago" with int arithmetic (1000 * 1000 * 1000 * 1000 overflows to -727379968), which placed the failure in the future. Use long literals and assert OPEN within the retry period and HALF_OPEN after it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
-
5085a97: test(circuit-breaker): verify open circuit does not call the service
-
Add a test with a call-counting service: once the circuit is open,
attemptRequest() must return the cached failure without calling the
service again. Previously a fallback and a real failing call returned
the same string, so ignoring the OPEN state went undetected. -
testEvaluateState computed "a failure long ago" with int arithmetic
(1000 * 1000 * 1000 * 1000 overflows to -727379968), which placed the
failure in the future. Use long literals and assert OPEN within the
retry period and HALF_OPEN after it.
Files Processed (1)
- circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java (3 hunks)
Actionable Comments (3)
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [41-41]
best_practice: "AssertEquals argument order should be (expected, actual)."
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [47-47]
best_practice: "AssertEquals argument order misused"
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [52-52]
best_practice: "AssertEquals argument order misused"
Skipped Comments (1)
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [85-105]
enhancement: "New test: verify cached response after OPEN does not call service again"
| var circuitBreaker = new DefaultCircuitBreaker(null, 1, 1, 100); | ||
| var circuitBreaker = new DefaultCircuitBreaker(null, 1, 1, 60_000_000_000L); | ||
| // Right now, failureCount<failureThreshold, so state should be closed | ||
| assertEquals(circuitBreaker.getState(), "CLOSED"); |
There was a problem hiding this comment.
JUnit's assertEquals expects (expected, actual). The arguments are reversed here, which leads to confusing failure messages. Change to assertEquals("CLOSED", circuitBreaker.getState()) and similarly for the other two state assertions.
There was a problem hiding this comment.
Fixed in 6f47a62: all four state assertions in testEvaluateState now use assertEquals(expected, actual).
| circuitBreaker.evaluateState(); | ||
| // Since failureCount>failureThreshold, and lastFailureTime is nearly equal to current time, | ||
| // the retry period has not elapsed yet, so state should be open | ||
| assertEquals(circuitBreaker.getState(), "OPEN"); |
There was a problem hiding this comment.
Similarly reverse the order for OPEN.
There was a problem hiding this comment.
Fixed in 6f47a62: all four state assertions in testEvaluateState now use assertEquals(expected, actual).
| circuitBreaker.lastFailureTime = System.nanoTime() - 120_000_000_000L; | ||
| circuitBreaker.evaluateState(); | ||
| assertEquals(circuitBreaker.getState(), "OPEN"); | ||
| assertEquals(circuitBreaker.getState(), "HALF_OPEN"); |
There was a problem hiding this comment.
Similarly reverse the order for HALF_OPEN.
There was a problem hiding this comment.
Fixed in 6f47a62: all four state assertions in testEvaluateState now use assertEquals(expected, actual).
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 6f47a62: test(circuit-breaker): use (expected, actual) order in testEvaluateState
Files Processed (1)
- circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java (3 hunks)
Actionable Comments (0)
Skipped Comments (9)
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [39-39]
readability: "Explicit initialization intent for long retry period."
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [41-41]
readability: "Assert initial CLOSED state after construction."
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [47-47]
readability: "Open state before retry period expiration."
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [51-51]
readability: "HALF_OPEN after retry period elapsed."
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [55-55]
maintainability: "Idempotency of evaluateState to CLOSED."
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [86-86]
enhancement: "Add test for cached response during OPEN state."
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [98-98]
behavior: "First failure triggers exception and opens circuit."
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [101-101]
behavior: "Cached failure response on OPEN state."
-
circuit-breaker/src/test/java/com/iluwatar/circuitbreaker/DefaultCircuitBreakerTest.java [103-103]
readability: "OPEN state preserved on cache-hit."
This branch has not been deployed
1 participant
What
Test-only changes to
DefaultCircuitBreakerTest.1. The open-circuit contract was not observed
Making
attemptRequest()ignore theOPENstate (if (state == State.OPEN)→if (false)) keeps all 12 module tests green.AppTestexpects the fallback"Delayed service is down", but that is also the message of the exception thrown by the real call, so a fallback and a real call cannot be told apart.New test
testOpenCircuitReturnsCachedResponseWithoutCallingServiceuses a service that counts calls and returns a distinct message per call (Failure #1,#2, ...). It asserts that, after the circuit opens,attemptRequest()returns the cachedFailure #1and the service was called once. It also catches removingevaluateState()fromattemptRequest().2.
testEvaluateStaterelied on anintoverflow1000 * 1000 * 1000 * 1000is evaluated asintand equals-727379968, so the "failure much earlier than now" actually lands ~0.73 s in the future, and the test assertsOPENfor it (the comment says the opposite of what the circuit should do). As a result, the test passes even ifevaluateStateignoresretryTimePeriod(> retryTimePeriod→> 0).The test now uses
longliterals and a 60 s retry period: a recent failure must beOPEN, a failure older than the retry period must beHALF_OPEN.Verification
mvn -pl circuit-breaker verify: 13 tests pass, spotless check passes.Found with supertest: mutation testing plus statement-removal analysis, with every finding confirmed by running the regression against the unmodified test.