test: move GetFreePort into test/framework/net#30
Conversation
GetFreePort is a network helper, not a general test util. Moving it into the test/framework tree gives it an obvious home and keeps test/utils from growing into a grab bag. The function body is unchanged; the only edit is the package clause plus a license header and package comment on the new file. Part of llm-d#1188 Signed-off-by: ChethanUK <chethanuk@outlook.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the test network utilities by renaming the package from utils to net under test/framework/net and updating its usages across the codebase. Feedback points out that calling GetFreePort() sequentially to allocate ports for multiple services can lead to port collision flakes in tests, and suggests introducing a helper function to allocate multiple distinct ports simultaneously.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| metricsPort, err := fwknet.GetFreePort() | ||
| require.NoError(t, err) | ||
| eppOptions.MetricsPort = metricsPort | ||
|
|
||
| grpcPort, err := testutils.GetFreePort() | ||
| grpcPort, err := fwknet.GetFreePort() | ||
| require.NoError(t, err) | ||
| eppOptions.GRPCPort = grpcPort | ||
|
|
||
| healthPort, err := testutils.GetFreePort() | ||
| healthPort, err := fwknet.GetFreePort() | ||
| require.NoError(t, err) | ||
| eppOptions.GRPCHealthPort = healthPort |
There was a problem hiding this comment.
Calling GetFreePort() multiple times sequentially to allocate ports for different services (MetricsPort, GRPCPort, GRPCHealthPort) can lead to port collision flakes. Since GetFreePort() immediately closes the listener, the OS may return the same ephemeral port for subsequent calls if they occur in rapid succession before any server binds to them. Consider adding a helper like GetFreePorts(n int) ([]int, error) in the fwknet package that keeps all listeners open until all requested ports are allocated, ensuring they are distinct.
1 participant
What type of PR is this?
/kind cleanup
/kind test
What this PR does / why we need it:
Second of eight in the llm-d#1188 split. It follows the Makefile wiring PR that creates
test/frameworkand puts it undermake test-unit.GetFreePortis a network helper. It sat intest/utils, which is how a packagebecomes a grab bag: nothing in the name says what belongs there, so everything does.
test/framework/netis where a port helper belongs.The move is verbatim. The function body is unchanged; the deltas are the package
clause, the llm-d license header and a package comment, none of which
test/utils/network.gocarried. Callers switch to thefwknetalias that.golangci.ymlalready declares.test/framework/netimports nothing frompkg/, so it stays an R1 leaf under thelayering rules in
test/framework/doc.go. The chain moves one leaf package per PR soeach is revertable on its own, with the harness last because it is the heaviest.
Test plan:
go buildandgo vet ./test/...clean at this commitgo test ./test/framework/...passes, including the moved testWhich issue(s) this PR fixes:
Part of llm-d#1188
Release note (write
NONEif no user-facing change):