test: move generic Pod builder into test/framework/k8s#32
Conversation
The Pod builders in pkg/epp/util/testing are plain Kubernetes object helpers with nothing EPP-specific about them, and they are used from outside the EPP tree. Moving them to test/framework/k8s leaves wrappers.go holding only the GAIE wrappers it is named for. The moved code is unchanged apart from the package clause, a license header, and a package comment; the corev1 import drops from wrappers.go because the remainder never used it. 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 utilities by moving the PodWrapper helper from pkg/epp/util/testing to a new shared framework package test/framework/k8s, updating all references across test files and integration harnesses, and adding comprehensive unit tests for the builder. The review feedback points out a potential issue in FromBase where a shallow copy of the Kubernetes Pod object is used, which can lead to shared state mutation bugs in tests; it is recommended to use DeepCopy() 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.
| func FromBase(pod *corev1.Pod) *PodWrapper { | ||
| return &PodWrapper{ | ||
| Pod: *pod, | ||
| } | ||
| } |
There was a problem hiding this comment.
Using a shallow copy (*pod) for a Kubernetes Pod object can lead to shared state mutation bugs in tests. Since corev1.Pod contains reference types like maps (Labels, Annotations) and slices (Containers, Conditions), modifying these fields on the wrapper will inadvertently mutate the original base pod's fields if they are not reassigned.
To ensure complete isolation and prevent accidental mutations of the base pod, use DeepCopy() which is built into Kubernetes API types. Additionally, we should add a nil check to prevent a nil-pointer dereference panic.
func FromBase(pod *corev1.Pod) *PodWrapper {
if pod == nil {
return nil
}
return &PodWrapper{
Pod: *pod.DeepCopy(),
}
}1 participant
What type of PR is this?
/kind cleanup
/kind test
What this PR does / why we need it:
Fourth of eight in the llm-d#1188 split. It follows the
NewTestContextmove. One block ofbuilders moves and four call sites follow it.
pkg/epp/util/testing/wrappers.goholds two unrelated things: GAIE wrappers and aPodWrapperfluent builder overcorev1.Pod. The Pod builders are plain Kubernetesobject construction, and two of their callers live outside the EPP tree under
test/integration/epp. They move totest/framework/k8s, leavingwrappers.gowithonly the GAIE wrappers it is named for. The
corev1import has to drop with them orthe remaining file does not compile.
The table-driven test is here for the coverage gate, not the move. CI computes one
number across
./test/framework/...and allows a 2.0pp regression, so a packagearriving without tests drags down everything already there. It also pins
Complete'sdefault-namespace branch, which nothing in the repo executed before.
Test plan:
go buildandgo vet ./test/...clean at this commitgo test ./test/framework/...passes, builders at 100% coverageWhich issue(s) this PR fixes:
Part of llm-d#1188
Release note (write
NONEif no user-facing change):