test: move ext_proc builders into test/framework/epp#34
Conversation
test/integration held a 642-line grab bag of ext_proc request builders and live executors that the integration suite imported as a package, which is the cyclic-dependency risk llm-d#1188 calls out: helpers living inside the tree that consumes them. The builders and executors are framework-generic, so they move to test/framework/epp and test/integration stops being importable. Five symbols are deleted rather than moved. ReqLLMWithStream, ReqLLMUnary, SendRequest and StartExtProcServer have no callers, and GenerateRequestWithStream is reachable only from ReqLLMWithStream, so moving it would ship a zero-caller export into the new package. 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 framework by moving ext_proc request/response builders and gRPC stream executors from the integration package to a dedicated test/framework/epp package. It extracts client execution helpers into client_extproc.go and introduces comprehensive unit tests for both builders and client utilities, updating all integration tests to use the new framework package. The feedback points out a concurrency issue in TestClientWaitsForALateServer where ExtProcServerClient is called inside a background goroutine; since it internally uses require assertions that invoke t.FailNow(), this can cause undefined behavior or deadlocks in Go tests, and should be refactored to return errors or run the server setup in the background instead.
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.
| go func() { | ||
| client, conn := ExtProcServerClient(t.Context(), t, port, logr.Discard()) | ||
| done <- dialed{client: client, conn: conn} | ||
| }() |
There was a problem hiding this comment.
Calling ExtProcServerClient inside a background goroutine is unsafe because it internally uses require.Eventually and require.NoError. In Go's testing package, calling methods that invoke t.FailNow() (such as require assertions) from any goroutine other than the main test goroutine is unsafe and can lead to deadlocks, silent failures, or panics.
To fix this, you can refactor ExtProcServerClient in client_extproc.go to return an error instead of asserting internally, or run the late server setup in the background with a delayed start and call ExtProcServerClient on the main test goroutine.
1 participant
What type of PR is this?
/kind cleanup
/kind test
What this PR does / why we need it:
Sixth of eight in the llm-d#1188 split. It follows the GAIE builders move.
test/integration/util.gowas a 642-line grab bag of ext_proc request builders andlive stream executors, and the suite under
test/integration/eppimported thedirectory above it as a package. That is the cyclic shape llm-d#1188 calls out: helpers
living inside the tree that consumes them. It moves to
test/framework/epp, splitinto
builders_extproc.goandclient_extproc.go. Afterwards the only Go files leftin
test/integrationsit behind theintegration_testsbuild tag, so the directoryis no longer importable.
Five symbols are deleted rather than moved.
ReqLLMWithStream,ReqLLMUnary,SendRequestandStartExtProcServerhave no callers anywhere in the tree, andGenerateRequestWithStreamis reachable only fromReqLLMWithStream.The new tests are here for the coverage gate: several hundred untested lines would
drag down the shared
./test/framework/...number. They assert message shape, notEPP behavior.
extPorcConnSetupPollIntervalis misspelled. It moves verbatim so the diff staysreadable as a move. Worth a separate cleanup.
Test plan:
go buildandgo vet ./test/...clean at this commitgo test ./test/framework/...passes, covering the builders and the streamclient against an in-process echo server
Which issue(s) this PR fixes:
Part of llm-d#1188
Release note (write
NONEif no user-facing change):