🧹 chore: Improve Idempotency middleware tests coverage #3521
Conversation
WalkthroughThe changes expand and refactor the idempotency middleware test suite. The test package is renamed for direct access to internal symbols, imports are adjusted, and a wide range of new tests are added to cover configuration, error handling, and edge cases. Additionally, stub implementations for storage and locking are introduced for controlled testing. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant App as Fiber App
participant Middleware as Idempotency Middleware
participant StubStorage as stubStorage
participant StubLock as stubLock
Test->>App: Initialize with Middleware (using stubs)
App->>Middleware: Incoming HTTP request
Middleware->>StubLock: Lock(id)
StubLock-->>Middleware: Success or error
Middleware->>StubStorage: Get(id)
StubStorage-->>Middleware: Data or error
Middleware->>App: Call handler or return cached response
App-->>Middleware: Handler response or error
Middleware->>StubStorage: Set(id, response)
StubStorage-->>Middleware: Success or error
Middleware->>StubLock: Unlock(id)
StubLock-->>Middleware: Success or error
Middleware-->>Test: Return response for assertions
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This pull request increases test coverage for the idempotency middleware by adding stub implementations and extra test cases to cover error paths, configuration defaults, and header filtering scenarios. Key changes include:
- Introducing stub types (stubLock and stubStorage) for simulating storage and locking behaviors.
- Updating tests to directly use middleware functions from the idempotency package.
- Adding new tests to cover edge cases in config defaults, error paths, and response header filtering.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/idempotency/stub_test.go | New stubs for testing basic lock and storage behavior. |
| middleware/idempotency/idempotency_test.go | Expanded test suite for middleware configuration and error handling. |
Comments suppressed due to low confidence (2)
middleware/idempotency/idempotency_test.go:329
- Consider adding a comment that clarifies the intended behavior of header filtering: the 'Foo' header should be maintained on both requests while non-kept headers (like 'Bar') are only available on the first response.
require.Equal(t, "foo", resp.Header.Get("Foo"))
middleware/idempotency/idempotency_test.go:270
- [nitpick] Consider externalizing error message strings generated by KeyHeaderValidate to reduce tight coupling between the test assertions and specific error text, which may improve maintainability in future changes.
req.Header.Set(ConfigDefault.KeyHeader, "bad")
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the test suite for the idempotency middleware. It introduces stub implementations for dependencies to enable comprehensive testing of various error conditions and edge cases, particularly around storage, locking, and configuration. The goal is to ensure the middleware is robust and handles failures gracefully.
Highlights
- Increased Test Coverage: Added numerous new test cases to improve coverage of the idempotency middleware, focusing on configuration edge cases and various error handling paths.
- Stub Implementations for Testing: Introduced
stubLockandstubStoragetypes to simulate dependencies (Locker and fiber.Storage) and facilitate testing of error conditions and specific middleware interactions. - Error Path Testing: Added specific tests to verify the middleware's behavior when encountering errors during storage operations (Get, Set), locking (Lock, Unlock), unmarshalling cached data, and when the main handler returns an error.
- Configuration Testing: Included tests to cover the default configuration (
configDefault) and how overrides are handled, ensuring expected behavior forNext,KeyHeaderValidate, and other options. - Response Header Filtering Test: Added a test case to specifically verify that the
KeepResponseHeadersconfiguration correctly filters headers when serving a cached response.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
gaby
changed the title
There was a problem hiding this comment.
Code Review
This pull request significantly improves the test coverage for the idempotency middleware by adding stub implementations for Locker and fiber.Storage, comprehensive tests for configDefault, and extensive tests for error paths and header filtering in idempotency.New. The consolidation of tests into a single file also improves maintainability.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3521 +/- ##
==========================================
+ Coverage 83.82% 84.34% +0.52%
==========================================
Files 120 120
Lines 12286 12286
==========================================
+ Hits 10299 10363 +64
+ Misses 1561 1517 -44
+ Partials 426 406 -20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
middleware/idempotency/idempotency_test.go (1)
118-126: 🛠️ Refactor suggestion*Avoid using testing.T inside spawned goroutines
assert.Equal(t, …)is called from 100 goroutines.testing.T’s methods are not guaranteed to be race-free; future Go versions may panic. Capture the value in a closure and report outside the goroutine, or useassert.Eventually.Example pattern:
results := make(chan bool, 100) for i := 0; i < 100; i++ { wg.Add(1) go func() { defer wg.Done() results <- doReq(... ) == "11" }() } wg.Wait() close(results) for ok := range results { assert.True(t, ok) }
🧹 Nitpick comments (4)
middleware/idempotency/stub_test.go (2)
12-18: Don’t invokeafterLockwhen the lock failsCalling
afterLockunconditionally means the callback fires even whenLockreturns an error, which is counter-intuitive and can mask issues in tests that expect “locked” state only after a successful lock.func (s *stubLock) Lock(string) error { - if s.afterLock != nil { - s.afterLock() - } - return s.lockErr + if s.lockErr == nil && s.afterLock != nil { + s.afterLock() + } + return s.lockErr }
22-26: Stub storage is not goroutine-safe and silently ignores TTL
stubStorageis accessed without synchronisation andsetCountis mutated from tests that can run in parallel (e.g.t.Parallel()and goroutine fan-out inTest_Idempotency). A data race is unlikely to break CI, yet it compromises determinism.In addition, the
_ time.Durationparameter is discarded, so tests cannot simulate expiry. Consider:
- Guarding the map and counters with a small
sync.Mutex(orsync/atomicforsetCount).- Recording the expiry and returning
nilfromGetafter it passes to mimic real storage.These tweaks improve fidelity with production behaviour and avoid racy false positives.
Also applies to: 38-48
middleware/idempotency/idempotency_test.go (2)
81-95: Close response bodies to avoid descriptor leaks
doReqreads the body but never closes it. Although tests exit quickly, keeping fd’s open is poor hygiene and can bite when the suite scales.- resp, err := app.Test(req, fiber.TestConfig{ + resp, err := app.Test(req, fiber.TestConfig{ Timeout: 15 * time.Second, FailOnTimeout: true, }) require.NoError(t, err) body, err := io.ReadAll(resp.Body) require.NoError(t, err) + _ = resp.Body.Close()Repeat for any other helper that returns a
*http.Response.
229-237: Duplicate helper leaks body—apply the same fixThe
dohelper has the same missingresp.Body.Close()issue. Patch it alongsidedoReq.body, _ := io.ReadAll(resp.Body) +_ = resp.Body.Close() return resp, string(body)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/idempotency/idempotency_test.go(5 hunks)middleware/idempotency/stub_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
middleware/idempotency/idempotency_test.go (3)
middleware/idempotency/idempotency.go (3)
IsFromCache(24-26)WasPutToCache(28-30)New(32-159)middleware/idempotency/config.go (2)
Config(15-49)ConfigDefault(52-74)constants.go (4)
MethodGet(5-5)MethodPost(7-7)StatusInternalServerError(101-101)StatusOK(50-50)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, ubuntu-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: lint
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: Analyse
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 561dc54 | Previous: 670fbd5 | Ratio |
|---|---|---|---|
Benchmark_GenericParseTypeString/benchmark_genericParseTypeString#01 |
13.54 ns/op 0 B/op 0 allocs/op |
8.867 ns/op 0 B/op 0 allocs/op |
1.53 |
Benchmark_GenericParseTypeString/benchmark_genericParseTypeString#01 - ns/op |
13.54 ns/op |
8.867 ns/op |
1.53 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
middleware/idempotency/idempotency_test.go (3)
83-97: Always close the response body in tests
resp.Bodyis never closed afterio.ReadAll. Although the object will be
GC-collected eventually, closing it immediately avoids file-descriptor leaks
and is a good habit when the same test opens many connections (e.g. the
100-goroutine race test below).resp, err := app.Test(req, fiber.TestConfig{ Timeout: 15 * time.Second, FailOnTimeout: true, }) require.NoError(t, err) body, err := io.ReadAll(resp.Body) require.NoError(t, err) +_ = resp.Body.Close() require.Equal(t, fiber.StatusOK, resp.StatusCode, string(body))
228-238: Apply the same body-closing pattern to helperdoThe helper re-introduces the same resource-leak risk. Close the body and mark
the helper witht.Helper()to improve failure reports.-func do(app *fiber.App, req *http.Request) (*http.Response, string) { - resp, err := app.Test(req, fiber.TestConfig{Timeout: 5 * time.Second}) - if err != nil { - panic(err) - } - body, err := io.ReadAll(resp.Body) - if err != nil { - panic(err) - } - return resp, string(body) +func do(app *fiber.App, req *http.Request) (*http.Response, string) { + t.Helper() // assuming the helper is moved into *_test.go that has a *testing.T in scope + resp, err := app.Test(req, fiber.TestConfig{Timeout: 5 * time.Second}) + if err != nil { + panic(err) + } + body, err := io.ReadAll(resp.Body) + if err != nil { + panic(err) + } + _ = resp.Body.Close() + return resp, string(body) }
152-173: Reuse of the samefasthttp.RequestCtxskews benchmark results
Benchmark_Idempotencyreuses a singlefasthttp.RequestCtxfor all
iterations. After the first call, both request and response objects contain
left-over state that the handler may rely on (headers, body, etc.). This can
both invalidate the benchmark and hide bugs.Create a fresh context each loop (or reset it):
for b.Loop() { - h(c) + ctx := &fasthttp.RequestCtx{} + ctx.Request.Header.SetMethod(fiber.MethodPost) + ctx.Request.SetRequestURI("/") + ctx.Request.Header.Set("X-Idempotency-Key", "00000000-0000-0000-0000-000000000000") + h(ctx) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/idempotency/idempotency_test.go(6 hunks)middleware/idempotency/stub_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/idempotency/stub_test.go
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: govulncheck-check
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: lint
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, ubuntu-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: Analyse
- GitHub Check: repeated
🔇 Additional comments (1)
middleware/idempotency/idempotency_test.go (1)
120-129: Potential data race onapp.Test
app.Testis invoked concurrently from 100 goroutines. Fiber v3 currently
does not document(*fiber.App).Testas goroutine-safe. If the helper happens
to mutate shared state (e.g. the app’s internal pool) this may introduce flaky
tests under-race.Consider serialising calls per application instance or switching the race
section toapp.Handler()with an isolatedfasthttp.RequestCtxper goroutine
to avoid touching global app state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
middleware/idempotency/idempotency_test.go (3)
83-97: Close theresp.Bodyto avoid leaking the connection pool
app.Testreturns an*http.Responsewith an open body. The body is read but never closed, which keeps the underlyingfasthttpconnection in the pool until it is garbage-collected and can skew concurrent-test behaviour.resp, err := app.Test(req, fiber.TestConfig{ Timeout: 15 * time.Second, FailOnTimeout: true, }) require.NoError(t, err) body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, fiber.StatusOK, resp.StatusCode, string(body)) +_ = resp.Body.Close() // best-effort; ignore error return string(body)
228-238: Helper leaks response bodies and hides test-line information
The helper repeats the body-closing issue and panics instead of failing the test at the correct call-site.-func do(app *fiber.App, req *http.Request) (*http.Response, string) { - resp, err := app.Test(req, fiber.TestConfig{Timeout: 5 * time.Second}) - if err != nil { - panic(err) - } - body, err := io.ReadAll(resp.Body) - if err != nil { - panic(err) - } - return resp, string(body) +func do(app *fiber.App, req *http.Request) (*http.Response, string) { + t.Helper() // report failure on caller line + resp, err := app.Test(req, fiber.TestConfig{Timeout: 5 * time.Second}) + require.NoError(t, err) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + _ = resp.Body.Close() + return resp, string(body) }
152-173: Benchmark allocates a freshfasthttp.RequestCtxevery iteration
Creating a new ctx inside the benchmark loop inflates allocation counts and dilutes the middleware overhead you’re trying to measure. Reuse a single ctx or an object-pool outsideb.Loop()to get cleaner numbers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/idempotency/idempotency_test.go(6 hunks)middleware/idempotency/stub_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/idempotency/stub_test.go
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, ubuntu-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: Analyse
- GitHub Check: lint
🔇 Additional comments (1)
middleware/idempotency/idempotency_test.go (1)
118-131: Potential data race:app.Testis invoked from 100 goroutines
fiber.App.Testis not documented as goroutine-safe; concurrent use may trigger false positives under the-raceflag. Consider switching to the low-level handler (app.Handler()) with independentfasthttp.RequestCtxinstances or serialising calls via a worker pool.
gaby
changed the title
Summary