Use ephemeral ports in unit tests #3686
Conversation
|
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. Warning Rate limit exceeded@ReneWerner87 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughTests were refactored to use ephemeral ports and, where applicable, a supplied net.Listener. A new App.Listener(net.Listener, ListenConfig) method is introduced. Test assertions and startup messages were adjusted to reflect dynamic addresses. Proxy test expectations now target example.com. Prefork and hooks tests bind to :0. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant N as net.Listener (127.0.0.1:0)
participant A as App
participant C as Client (HTTP)
T->>N: net.Listen("tcp", "127.0.0.1:0")
N-->>T: ephemeral addr (e.g., 127.0.0.1:<port>)
T->>A: App.Listener(N, cfg)
Note over A: Server starts accepting on supplied listener
T->>C: Build request to http://127.0.0.1:<port>/...
C->>A: HTTP request over established connection
A-->>C: Response
C-->>T: Assert results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, 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 primarily focuses on enhancing the robustness and reliability of unit tests by transitioning from hardcoded network ports to dynamically assigned ephemeral ports. This change prevents potential test failures due to port conflicts, especially in concurrent test environments or on systems where specific ports might already be in use. It also updates assertions related to startup messages to correctly reflect these dynamic port assignments.
Highlights
- Dynamic Port Allocation for Tests: Modified numerous unit tests across app_test.go, hooks_test.go, listen_test.go, and prefork_test.go to utilize OS-assigned ephemeral ports (by listening on ":0") instead of fixed port numbers. This ensures tests are isolated and less prone to external port conflicts.
- Refined Startup Message Assertions: Updated tests that verify startup messages (in listen_test.go) to dynamically capture the assigned port and use it in assertions, ensuring the displayed messages accurately reflect the runtime environment.
- Client Test URL Update: Changed the hardcoded localhost:3000 URL to example.com in client/client_test.go for proxy URL tests, making them more generic and less dependent on a local listener.
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 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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR updates unit tests to use ephemeral ports (port 0) instead of hardcoded port numbers, allowing the OS to automatically assign available ports and preventing test conflicts.
- Replaces fixed port numbers with
:0for automatic port assignment - Implements dynamic port retrieval for startup message assertions
- Updates client tests to use external domains instead of localhost
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| prefork_test.go | Changes hardcoded ports to ephemeral ports for prefork tests |
| listen_test.go | Updates listen tests to use ephemeral ports and implements dynamic port retrieval for startup message assertions |
| hooks_test.go | Converts hook tests to use ephemeral ports |
| client/client_test.go | Updates proxy tests to use external domain instead of localhost |
| app_test.go | Modifies timeout and request tests to use ephemeral ports with proper listener management |
There was a problem hiding this comment.
Code Review
This pull request effectively replaces hardcoded ports in unit tests with ephemeral ports, which is a great improvement for test reliability. The changes are consistently applied across various test files. The use of net.Listen to acquire a dynamic port and then passing the listener to the application is a solid approach. I've identified a couple of opportunities to improve maintainability by refactoring duplicated code blocks into helper functions in app_test.go and listen_test.go.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
client/client_test.go (1)
1656-1656: Switch to example.com is correct and consistent with in-repo test DNS routing.This aligns with other tests using example.com and avoids hard-coding localhost:ports.
To make the proxy startup deterministic and avoid the 1s sleep, consider buffering the channel that carries the dynamic address and dropping the sleep:
- addrChan := make(chan string) + addrChan := make(chan string, 1)- time.Sleep(1 * time.Second) + // No sleep needed; receive from addrChan below blocks until address is ready.hooks_test.go (1)
285-285: Listen on :0 to avoid fixed-port conflicts — approved.This reduces flakiness in parallel test runs.
You can drop the 1s sleep and instead synchronize on readiness via a small channel signal or OnListen hook to make the test deterministic.
app_test.go (1)
1573-1576: Ephemeral listener + direct Dial — approved.Good use of 127.0.0.1:0 and passing the listener to app.Listener; avoids relying on global ports.
To avoid the fixed 500ms sleeps before dialing, consider synchronizing readiness. For example:
- Signal a “server starting” channel before calling app.Listener, then use require.Eventually to attempt net.Dial until it succeeds within a timeout.
- Alternatively, use ListenConfig.ListenerAddrFunc (for app.Listen cases) to expose readiness without sleeps.
Also applies to: 1584-1584, 1604-1605
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (5)
app_test.go(6 hunks)client/client_test.go(2 hunks)hooks_test.go(3 hunks)listen_test.go(10 hunks)prefork_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:
hooks_test.golisten_test.go
🧬 Code Graph Analysis (4)
hooks_test.go (1)
listen.go (1)
ListenConfig(42-125)
listen_test.go (2)
listen.go (1)
ListenConfig(42-125)constants.go (1)
NetworkTCP4(292-292)
client/client_test.go (1)
client/client.go (1)
Get(691-693)
app_test.go (2)
constants.go (2)
NetworkTCP4(292-292)MethodGet(5-5)listen.go (1)
ListenConfig(42-125)
🪛 GitHub Check: lint
listen_test.go
[failure] 522-522:
Error return value is not checked (errcheck)
[failure] 546-546:
Error return value is not checked (errcheck)
[failure] 567-567:
Error return value is not checked (errcheck)
[failure] 586-586:
Error return value is not checked (errcheck)
🪛 GitHub Actions: golangci-lint
listen_test.go
[error] 522-522: golangci-lint run: listen_test.go:522:10: Error return value is not checked (errcheck)
⏰ 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). (3)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: Analyse
🔇 Additional comments (9)
client/client_test.go (1)
1670-1670: Error-path assertion updated — looks good.Targeting example.com here keeps tests decoupled from fixed ports and matches the success-path change above.
prefork_test.go (2)
64-64: Ephemeral port for prefork master: Good change.Binding to :0 removes port contention and test flakiness on shared CI.
86-86: Using :0 in startupMessage test is safe and keeps the assertion semantics.Since the test verifies “no output,” the specific port value isn’t material, and :0 avoids conflicts.
hooks_test.go (2)
308-308: Prefork OnListen on :0 — approved.Prefork + ephemeral port is the right combo to keep tests isolated.
329-329: prefork(":0", ...) change looks good.Keeps the prefork hook test independent of fixed ports.
listen_test.go (2)
37-38: Core Listen test now uses :0 — approved.The change removes port collisions while preserving behavior with DisableStartupMessage.
172-173: Broad migration to :0 across Listen variants — approved.Good coverage: prefork, TLS, mTLS, ListenerAddrFunc, ListenerNetwork all switched to ephemeral binding. This should significantly reduce CI flakiness due to port reuse.
Also applies to: 183-195, 203-210, 227-231, 252-258, 276-281, 303-310, 335-336, 358-365, 380-388, 425-449
app_test.go (2)
1616-1619: BadRequest test migrated to dynamic listener — approved.The pattern mirrors ReadTimeout and keeps tests isolated.
Also applies to: 1622-1622, 1641-1642
1656-1659: SmallReadBuffer test updated to use :0 — approved.End-to-end behavior is preserved while eliminating fixed-port dependency.
Also applies to: 1661-1661, 1670-1671
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3686 +/- ##
=======================================
Coverage ? 92.01%
=======================================
Files ? 114
Lines ? 11494
Branches ? 0
=======================================
Hits ? 10576
Misses ? 659
Partials ? 259
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:
|
Summary
Testing
make audit(fails: lock value copy warnings in generated files)make generatemake betteralign(fails: analysis skipped due to errors)make modernize(fails: package requires Go 1.25)make formatmake testhttps://chatgpt.com/codex/tasks/task_e_689e3dcbed6c832698832ba4094c29a9