🧹 chore: Fix proxy middleware tests for offline environments #3467
Conversation
WalkthroughA new helper function was introduced to create a local HTTP redirect server for testing. Several proxy-related tests were updated to use this local server instead of external URLs, ensuring that tests are self-contained and do not depend on external network resources. Test assertions were adjusted to reflect the local server responses. Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
🪧 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 (
|
gaby
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3467 +/- ##
==========================================
+ Coverage 84.36% 84.45% +0.09%
==========================================
Files 120 120
Lines 12157 12157
==========================================
+ Hits 10256 10267 +11
+ Misses 1470 1462 -8
+ Partials 431 428 -3
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
This PR refactors proxy middleware tests to remove reliance on external network calls by spinning up local HTTP servers for redirect and real URL scenarios.
- Added
createRedirectServerhelper to simulate HTTP redirects in tests - Updated
Test_Proxy_Do_*andTest_Proxy_DoRedirects_*to use local servers and assert against controlled responses
Comments suppressed due to low confidence (1)
middleware/proxy/proxy_test.go:62
- [nitpick] The variable name 'addr' is quite generic; consider renaming to something like 'redirectServerAddr' to make its purpose clearer.
var addr string
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/proxy/proxy_test.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
middleware/proxy/proxy_test.go
[failure] 58-58:
createRedirectServer - result 0 (*github.com/gofiber/fiber/v3.App) is never used (unparam)
🪛 GitHub Actions: golangci-lint
middleware/proxy/proxy_test.go
[error] 58-58: golangci-lint: createRedirectServer - result 0 (*github.com/gofiber/fiber/v3.App) is never used (unparam)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (11)
middleware/proxy/proxy_test.go (11)
527-529: Good replacement of external URL dependency!The test now uses a local server created with
createProxyTestServerIPv4instead of an external URL, making the test more predictable and reliable.
533-533: Good update to use local test server URL!This change ensures the test uses the local server address rather than an external URL.
545-545: Appropriate assertion update!The expected response has been updated to match the local server's response ("real url").
552-552: Good implementation of local redirect server!The test now uses the new
createRedirectServerfunction instead of relying on external redirect behavior.
556-556: Proper use of local server address!This correctly uses the address returned by
createRedirectServer.
566-567: Correct assertions for local redirect server!The expected response body ("redirect") and status code (301) now match what the local redirect server returns.
574-574: Good reuse of redirect server in another test!Using the same redirect server helper function across multiple tests ensures consistency.
578-578: Appropriate use of DoRedirects with local server!The test now properly tests the redirect functionality with a controlled local environment.
586-588: Proper assertions for redirect follow-through!These assertions correctly verify that the
DoRedirectsfunction follows the redirect and receives the "final" response.
597-597: Consistent use of redirect server!Using the same helper function across all redirect-related tests ensures testing consistency.
601-601: Good test for too many redirects!Setting max redirects to 0 is a good way to test the too many redirects error handling.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
middleware/proxy/proxy_test.go (1)
58-79: Well-designed helper function for offline testingThe new
createRedirectServerfunction is well-implemented, creating a local HTTP server that handles redirects without requiring external network access. This approach makes the tests more reliable and consistent.There's a good use of
t.Helper()to mark this as a test helper function, and the cleanup function properly closes the listener when the test completes.One minor issue is that the error from
ln.Close()is not being checked, as flagged by the linter. Apply this fix:- t.Cleanup(func() { ln.Close() }) + t.Cleanup(func() { + err := ln.Close() + if err != nil { + t.Logf("failed to close listener: %v", err) + } + })🧰 Tools
🪛 GitHub Check: lint
[failure] 73-73:
Error return value ofln.Closeis not checked (errcheck)🪛 GitHub Actions: golangci-lint
[error] 73-73: golangci-lint: Error return value of
ln.Closeis not checked (errcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/proxy/proxy_test.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
middleware/proxy/proxy_test.go
[failure] 73-73:
Error return value of ln.Close is not checked (errcheck)
🪛 GitHub Actions: golangci-lint
middleware/proxy/proxy_test.go
[error] 73-73: golangci-lint: Error return value of ln.Close is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (11)
middleware/proxy/proxy_test.go (11)
527-531: Good replacement of external URL dependencyReplacing the external URL with a local test server is a great improvement. This change eliminates network dependencies and makes the test more reliable and faster.
534-535: Properly updated the test case with local server URLThe test logic is maintained while replacing the external dependency with a local server URL.
546-546: Appropriate assertion update for local server responseThe assertion has been correctly updated to match the response from the local server.
553-553: Good use of the new redirect server helperThe test now uses the newly created
createRedirectServerfunction, eliminating the dependency on external network resources.
556-557: Properly updated test logic for local redirect serverThe test logic has been maintained while using the local redirect server URL.
566-567: Appropriate assertions for redirect behaviorThe assertions correctly verify the redirect response and status code from the local server.
574-574: Good application of redirect server for testing complex scenariosUsing the local redirect server for testing multi-redirect scenarios is an excellent approach.
577-578: Well-structured test for redirect followingThe test properly sets up the redirect limit parameter (1) to test that the proxy follows redirects as expected.
585-587: Appropriate assertions for final redirect destinationThe assertions correctly verify that the final response after following redirects is as expected.
596-596: Good use of redirect server for error testingUsing the local redirect server to test error conditions is a smart approach.
599-600: Well-crafted test for redirect limitsSetting the redirect limit to 0 is an excellent way to test the "too many redirects" error condition.
![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: 60a122a | Previous: 81edaf0 | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels_Parallel/Brotli_LevelBestCompression - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
Summary
createRedirectServerused by updated testsThis change will help running the tests with agents like
codexwhich disable internet access after environment is setup.Testing
go test ./middleware/proxy -run Test_Proxy_Do_WithRealURL -vgo test ./middleware/proxy -run Test_Proxy_Do_WithRedirect -vgo test ./middleware/proxy -run Test_Proxy_DoRedirects_RestoreOriginalURL -vgo test ./middleware/proxy -run Test_Proxy_DoRedirects_TooManyRedirects -vgo test ./...