[test-improver] Improve tests for server HMAC middleware#6601
Merged
Conversation
Add two tests covering previously uncovered branches in hmacMiddleware: - TestHMACMiddleware_BodyReadError: exercises the body-read failure path (io.ReadAll error → HTTP 400) using an errReader that always returns an error. - TestHMACMiddleware_ConcurrentReplay_PostCheckRejected: exercises the post-check replay detection branch using synchronized barrierReader goroutines that both pass the seenNonce pre-check before racing on checkAndSet, ensuring exactly one request succeeds. Also adds errReader and barrierReader helper types to support these tests. hmacMiddleware coverage: 88.1% → 100% Overall server package coverage: 91.7% → 92.1% Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds two new tests to internal/server/hmac_test.go to cover previously unreachable branches in hmacMiddleware: the body-read error path and the post-check concurrent replay rejection path. Coverage of hmacMiddleware increases from 88.1% to 100%.
Changes:
- Add
errReaderandbarrierReadertest helpers for deterministic error and concurrency simulation. - Add
TestHMACMiddleware_BodyReadErrorto exercise theio.ReadAllfailure branch (HTTP 400). - Add
TestHMACMiddleware_ConcurrentReplay_PostCheckRejectedusing a barrier to deterministically race two requests atcheckAndSet.
Show a summary per file
| File | Description |
|---|---|
| internal/server/hmac_test.go | Adds reader helpers and two new tests covering body-read error and post-check replay branches. |
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: 0
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.
Test Improvements:
hmac_test.goFile Analyzed
internal/server/hmac_test.gointernal/serverImprovements Made
1. Increased Coverage
✅ Added
TestHMACMiddleware_BodyReadError— covers theio.ReadAllerror branch inhmacMiddlewarethat was previously unreachable by existing tests✅ Added
TestHMACMiddleware_ConcurrentReplay_PostCheckRejected— covers the post-check replay detection branch where two concurrent requests with identical nonces both pass theseenNoncepre-check but only one winscheckAndSetPrevious Coverage (
hmacMiddleware): 88.1%New Coverage (
hmacMiddleware): 100%Server package improvement: 91.7% → 92.1%
2. New Test Helper Types
errReader— minimalio.Readerthat always returns an error, used to trigger the body-read failure path (HTTP 400)barrierReader— synchronized reader that stalls both goroutines insideio.ReadAll(afterseenNoncebut beforecheckAndSet), enabling deterministic testing of the concurrent post-check replay race3. Better Testing Patterns
atomic.Int32for goroutine-safe result counting in the concurrent testsync.WaitGroupbarrier to guarantee deterministic race coverage without timing-dependent sleepsrequirefor critical invariantsTest Execution
All tests pass:
Why These Changes?
hmacMiddlewarehad two uncovered branches:Body read error (
io.ReadAllfailure): This path returns HTTP 400 with"failed to read request body". It was unreachable without a reader that returns errors, which normal test requests never do.Post-check replay detection (
checkAndSetreturning false): This is a concurrency guard — the pre-check (seenNonce) is a read-only fast-reject, while the authoritative write happens incheckAndSet. When two concurrent requests carry the same nonce and both pass the pre-check simultaneously, exactly one should wincheckAndSetand the other should be rejected. Testing this required abarrierReaderto synchronise both goroutines at the body-read stall point, ensuring both are pastseenNoncebefore either callscheckAndSet.Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
index.crates.ioSee Network Configuration for more information.