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.
PR SummaryUpdated circuit-breaker tests to correctly observe open-circuit behavior and fix timestamp overflow handling. Reworked testEvaluateState to use long literals and a 60-second retry, and added testOpenCircuitReturnsCachedResponseWithoutCallingService to ensure an open circuit returns a cached failure without calling the service. Imports adjusted and assertion order made explicit (expected, actual). Changes
autogenerated by presubmit.ai |
![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"
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3619 +/- ##
============================================
- Coverage 84.06% 83.99% -0.08%
+ Complexity 4353 4347 -6
============================================
Files 1133 1133
Lines 15400 15400
Branches 739 739
============================================
- Hits 12946 12935 -11
- Misses 2158 2164 +6
- Partials 296 301 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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."
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.