[test-improver] Improve tests for server circuit breaker#4181
Merged
Conversation
Add missing test cases that cover previously uncovered branches in circuit_breaker.go: - RecordSuccess from OPEN state: exercises the 'else if prev != circuitClosed' branch, covering the case where an in-flight request completes successfully after the circuit was opened - Allow in HALF-OPEN with no probe in flight: exercises the defensive fallback path that permits a request through when probeInFlight is false - extractRateLimitErrorText with non-map content items: covers the continue branch when a content array element is not a map - isRateLimitToolResult with non-map content items: same as above for the rate limit tool result detector - parseRateLimitResetFromText without a terminator character: covers the 'end < 0' branch when the seconds value has no trailing s/]/) delimiter Coverage improvements in circuit_breaker.go: Allow: 90.0% -> 95.0% RecordSuccess: 91.7% -> 100.0% extractRateLimitErrorText: 90.9% -> 100.0% isRateLimitToolResult: 93.8% -> 100.0% parseRateLimitResetFromText: 91.7% -> 100.0% Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves branch coverage for the server rate-limit circuit breaker by adding targeted tests for defensive/edge-case paths in the circuit breaker and rate-limit parsing helpers.
Changes:
- Add test coverage for skipping non-map
contententries in rate-limit tool result parsing/extraction. - Add test coverage for rate-limit reset parsing when the pattern has no terminator.
- Add tests for circuit breaker edge states:
RecordSuccess()when OPEN andAllow()when HALF-OPEN withprobeInFlight=false.
Show a summary per file
| File | Description |
|---|---|
internal/server/circuit_breaker_test.go |
Adds new table/subtests to exercise previously uncovered defensive branches in circuit breaker logic and rate-limit text parsing. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This was referenced Apr 20, 2026
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.
File Analyzed
internal/server/circuit_breaker_test.gointernal/serverImprovements Made
1. Increased Coverage
Five previously uncovered branches in
circuit_breaker.goare now exercised:RecordSuccessfrom OPEN state — covers theelse if prev != circuitClosedbranch, which fires when an in-flight request completes successfully after the circuit was already opened by a concurrent requestAllowin HALF-OPEN withprobeInFlight=false— covers the defensive fallback that permits a request through when the probe flag was reset without a state transition (the comment in code says "This shouldn't normally happen")extractRateLimitErrorTextwith non-map content items — covers thecontinuebranch when a content array element is not amap[string]interface{}(e.g. a raw string)isRateLimitToolResultwith non-map content items — same branch, parallel functionparseRateLimitResetFromTextwithout a terminator — covers theend < 0branch when the seconds value has no trailings,], or)delimiter (e.g."rate reset in 42")2. Coverage Improvements (circuit_breaker.go)
AllowRecordSuccessextractRateLimitErrorTextisRateLimitToolResultparseRateLimitResetFromTextOverall
internal/serverpackage: 88.8% → 89.2%3. Cleaner & More Stable Tests
t.Parallel()consistent with the existing styleTestCircuitBreaker_HalfOpenAllowsWhenNoProbeInFlightdirectly manipulates internal state via the exported mutex to set up the edge-case scenario without timing dependenciesTest Execution
All tests pass:
Note:
TestFetchAndFixSchema_NetworkErrorininternal/configis a pre-existing failure (confirmed by checking main before this change) and is unrelated to these modifications.Why These Changes?
The
circuit_breaker.gofunctionsRecordSuccess,extractRateLimitErrorText,isRateLimitToolResult, andparseRateLimitResetFromTexteach had minor coverage gaps caused by defensive/error branches that are difficult to trigger through normal code flow. These branches protect against edge cases (in-flight requests completing after circuit open, malformed content arrays, truncated error strings) and are important to verify correct behavior. The new tests exercise each gap directly and confirm the expected fallback behavior.Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests
Warning
The following domain was blocked by the firewall during workflow execution:
invalidhostthatdoesnotexist12345.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.