🚨 Test: Add flushing-related unit tests for net/http adaptor#3807
🚨 Test: Add flushing-related unit tests for net/http adaptor#3807ReneWerner87 merged 6 commits intogofiber:mainfrom
Conversation
Summary of ChangesHello @grivera64, 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 introduces a suite of new unit tests for the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds integration tests for the adaptor middleware that validate net/http → adaptor → fasthttp/fiber interactions, including propagation of request fields/headers/body, mid-response Flush() streaming, delayed chunk writes, and timeout-interrupted partial responses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test
participant Adaptor as Fiber Adaptor
participant NetHTTP as net/http Handler
participant FastHTTP as fasthttp Response
Test->>Adaptor: invoke adapted handler with RequestCtx
Adaptor->>NetHTTP: construct ResponseWriter + *http.Request, call handler
NetHTTP->>NetHTTP: set headers, write body chunk(s)
NetHTTP->>Adaptor: call Flush (if Flusher present)
Adaptor->>FastHTTP: stream headers/chunk immediately
alt repeated flushes
NetHTTP->>Adaptor: additional Flush calls
Adaptor->>FastHTTP: stream more chunks
end
alt timeout / interruption
Note right of Adaptor #ffe6a7: test/app timeout triggers
Adaptor->>NetHTTP: handler interrupted
NetHTTP-->>Adaptor: returns partial body / reader error
Adaptor->>FastHTTP: finalize with partial body
else normal completion
NetHTTP->>Adaptor: handler completes
Adaptor->>FastHTTP: finalize response
end
Test->>Test: assert status, headers, body chunks, and error conditions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Comment |
There was a problem hiding this comment.
Code Review
This pull request adds valuable unit tests for flushing behavior in the net/http adaptor. The tests cover several important scenarios, including a direct handler test, a full application test, and a test for interrupted connections. My review includes suggestions to improve maintainability by reducing code duplication, enhance test suite performance by enabling parallel execution, and maintain a consistent coding style within the tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3807 +/- ##
==========================================
- Coverage 92.07% 92.02% -0.05%
==========================================
Files 115 115
Lines 12249 12249
==========================================
- Hits 11278 11272 -6
- Misses 707 712 +5
- Partials 264 265 +1
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:
|
There was a problem hiding this comment.
Pull Request Overview
Adds unit tests to verify flushing behavior in the net/http adaptor, ensuring http.Flusher support and behavior under interrupted reads.
- Adds Test_HTTPHandler_Flush to validate request/response mapping and flush behavior.
- Adds Test_HTTPHandler_Flush_App_Test to verify streaming with flush via app.Test.
- Adds Test_HTTPHandler_App_Test_Interrupted to verify behavior when the client times out mid-stream.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/adaptor/adaptor_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (Make target:make format).
Run golangci-lint for linting (Make target:make lint).
Rungo vetas part of audit to catch suspicious constructs (Make target:make audit).
Optimize struct field alignment with betteralign (Make target:make betteralign).
Applygopls modernizeto update code patterns (Make target:make modernize).
Files:
middleware/adaptor/adaptor_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Run the test suite with gotestsum (Make target:make test).
Run benchmarks withgo test(Make target:make benchmark).
Files:
middleware/adaptor/adaptor_test.go
🧠 Learnings (1)
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/adaptor/adaptor_test.go
🧬 Code graph analysis (1)
middleware/adaptor/adaptor_test.go (3)
constants.go (4)
MethodPost(7-7)StatusBadRequest(73-73)StatusOK(52-52)MethodGet(5-5)middleware/adaptor/adaptor.go (1)
HTTPHandlerFunc(33-35)app.go (1)
TestConfig(1084-1095)
🪛 GitHub Actions: golangci-lint
middleware/adaptor/adaptor_test.go
[error] 33-33: golangci-lint: string /foo/bar?baz=123 has 3 occurrences, make it a constant (goconst)
🪛 GitHub Check: lint
middleware/adaptor/adaptor_test.go
[failure] 246-246:
File is not properly formatted (gofmt)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-13)
|
@grivera64 can you check the ai hints and linting |
@ReneWerner87 Done (edit: for the most part, a few hints I missed, but will address asap)! The linter workflow should pass and I've separated the middleware/adaptor vs adapter code unit tests to cover both the middleware adaptor and built-in adapter as per the AI hints. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
middleware/adaptor/adaptor_test.go (2)
209-233: Add t.Parallel; use require.True instead of t.Fatalf for Flusher check; consider shorter sleep
- Add t.Parallel() at the start.
- Replace t.Fatalf with require.True for consistency.
- Optionally reduce time.Sleep(500ms) to speed up tests.
235-263: Same: add t.Parallel; use require.True; consider shorter sleep
- Add t.Parallel() at the start.
- Replace t.Fatalf with require.True.
- Optionally reduce time.Sleep(500ms) while keeping Timeout < sleep.
🧹 Nitpick comments (1)
middleware/adaptor/adaptor_test.go (1)
117-207: Avoid re-declaring expected locals; reuse package-level constants*This test redefines expectedRequestURI/body/host/remoteAddr already declared at the top. Reuse the hoisted constants to prevent shadowing and keep linters (goconst/shadow) happy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
adapter_test.go(2 hunks)middleware/adaptor/adaptor_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (Make target:make format).
Run golangci-lint for linting (Make target:make lint).
Rungo vetas part of audit to catch suspicious constructs (Make target:make audit).
Optimize struct field alignment with betteralign (Make target:make betteralign).
Applygopls modernizeto update code patterns (Make target:make modernize).
Files:
adapter_test.gomiddleware/adaptor/adaptor_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Run the test suite with gotestsum (Make target:make test).
Run benchmarks withgo test(Make target:make benchmark).
Files:
adapter_test.gomiddleware/adaptor/adaptor_test.go
🧠 Learnings (4)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
adapter_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
adapter_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
adapter_test.gomiddleware/adaptor/adaptor_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/adaptor/adaptor_test.go
🧬 Code graph analysis (2)
adapter_test.go (3)
app.go (3)
Handler(40-40)New(515-628)TestConfig(1084-1095)constants.go (2)
StatusOK(52-52)MethodGet(5-5)middleware/adaptor/adaptor_test.go (1)
Test_HTTPHandler_App_Test_Interrupted(235-262)
middleware/adaptor/adaptor_test.go (4)
constants.go (4)
MethodPost(7-7)StatusBadRequest(73-73)StatusOK(52-52)MethodGet(5-5)middleware/adaptor/adaptor.go (1)
HTTPHandlerFunc(33-35)app.go (2)
New(515-628)TestConfig(1084-1095)adapter_test.go (1)
Test_HTTPHandler_App_Test_Interrupted(144-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
🔇 Additional comments (3)
adapter_test.go (1)
90-116: Flush test logic looks goodCovers write+flush path; assertions are consistent with the rest of the file.
middleware/adaptor/adaptor_test.go (2)
26-31: Good: shared test constants reduce duplicationNice centralization; helps with goconst and readability.
168-173: Flush point inserted correctlySplitting the write and flushing in between matches the intended behavior under test.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
middleware/adaptor/adaptor_test.go (1)
214-224: DRY the streaming handler used in both testsExtract the repeated net/http handler into a helper to reduce duplication and keep the tests lean.
Example:
// outside tests func streamingFlusherHandler(t *testing.T) fiber.Handler { return HTTPHandlerFunc(func(w http.ResponseWriter, _ *http.Request) { flusher, ok := w.(http.Flusher) require.True(t, ok, "w does not implement http.Flusher") w.WriteHeader(fiber.StatusOK) fmt.Fprintf(w, "Hello ") flusher.Flush() time.Sleep(500 * time.Millisecond) fmt.Fprintf(w, "World!") }) }Then:
- app.Get("/", HTTPHandlerFunc(func(w http.ResponseWriter, r *http.Request) { ... })) + app.Get("/", streamingFlusherHandler(t))Also applies to: 242-253
🧹 Nitpick comments (3)
middleware/adaptor/adaptor_test.go (3)
53-81: Prefer require in handler assertionsUsing require.* inside net/http handler stops on first failure, keeping failures local and clearer.
Based on learnings
124-134: Reuse package-level constants here to avoid duplicationRemove local re-definitions and rely on the top-level expectedRequestURI/expectedBody/expectedHost/expectedRemoteAddr.
- expectedRequestURI := "/foo/bar?baz=123" - expectedBody := "body 123 foo bar baz" - expectedHost := "foobar.com" - expectedRemoteAddr := "1.2.3.4:6789"
168-173: Make flush mandatory in this testAssert ResponseWriter implements http.Flusher to catch regressions, then flush.
- fmt.Fprintf(w, "request body is ") - if flusher, ok := w.(http.Flusher); ok { - flusher.Flush() - } - fmt.Fprintf(w, "%q", body) + fmt.Fprintf(w, "request body is ") + flusher, ok := w.(http.Flusher) + require.True(t, ok, "w does not implement http.Flusher") + flusher.Flush() + fmt.Fprintf(w, "%q", body)As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
adapter_test.go(2 hunks)middleware/adaptor/adaptor_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- adapter_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (Make target:make format).
Run golangci-lint for linting (Make target:make lint).
Rungo vetas part of audit to catch suspicious constructs (Make target:make audit).
Optimize struct field alignment with betteralign (Make target:make betteralign).
Applygopls modernizeto update code patterns (Make target:make modernize).
Files:
middleware/adaptor/adaptor_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Run the test suite with gotestsum (Make target:make test).
Run benchmarks withgo test(Make target:make benchmark).
Files:
middleware/adaptor/adaptor_test.go
🧠 Learnings (3)
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/adaptor/adaptor_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/adaptor/adaptor_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
middleware/adaptor/adaptor_test.go
🧬 Code graph analysis (1)
middleware/adaptor/adaptor_test.go (4)
constants.go (4)
MethodPost(7-7)StatusBadRequest(73-73)StatusOK(52-52)MethodGet(5-5)middleware/adaptor/adaptor.go (1)
HTTPHandlerFunc(33-35)app.go (2)
New(515-628)TestConfig(1084-1095)adapter_test.go (1)
Test_HTTPHandler_App_Test_Interrupted(146-175)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
🔇 Additional comments (2)
middleware/adaptor/adaptor_test.go (2)
26-32: Good: hoisted shared test constantsReduces duplication and resolves goconst noise. Nice.
212-225: LGTM: end-to-end flush path via app.TestSolid sanity check for normal streaming completion.
|
@ReneWerner87 Now actually done 😂 We can wait for the workflows to complete before merging and everything should be good. |
Description
This PR adds flushing-related unit tests for the net/http adaptor:
Fixes #3421
Changes introduced/Type of change
fasthttpadaptorchanges invalyala/fasthttp@v1.67.0Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.