test(util/lifted): cover resource-name helpers#7467
Conversation
…resource helpers Adds unit tests for IsPrefixedNativeResource, IsHugePageResourceName, IsAttachableVolumeResourceName, IsExtendedResourceName and IsScalarResourceName. Signed-off-by: Aditya Pratap Singh Hada <apshada1@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, 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 improves the test suite by introducing unit tests for previously uncovered helper functions related to Kubernetes resource names. These additions help ensure the correctness and reliability of resource validation logic without modifying any existing production code. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. Footnotes
|
|
Welcome @apshada! It looks like this is your first PR to karmada-io/karmada 🎉 |
karmada-bot
added
the
size/M
There was a problem hiding this comment.
Pull request overview
This PR adds unit coverage for resource-name helper functions in the pkg/util/lifted package, extending tests around Kubernetes-lifted helper behavior without changing production logic.
Changes:
- Added table-driven tests for
IsPrefixedNativeResource,IsHugePageResourceName,IsAttachableVolumeResourceName, andIsExtendedResourceName. - Added coverage for
IsScalarResourceName, which lives inresourcename.gobut is exercised from the existing helper test file. - Expanded negative-case coverage for invalid, empty, and non-native resource names.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 adds unit tests for several resource-related helper functions, including IsPrefixedNativeResource, IsHugePageResourceName, IsAttachableVolumeResourceName, IsExtendedResourceName, and IsScalarResourceName. The review feedback suggests refactoring these tests to use table-driven subtests and parallel execution to improve consistency with existing tests and enhance observability. Additionally, there is a recommendation to move the scalar resource tests to a separate file for better code organization.
| func TestIsPrefixedNativeResource(t *testing.T) { | ||
| cases := []struct { | ||
| name corev1.ResourceName | ||
| want bool | ||
| }{ | ||
| {"kubernetes.io/foo", true}, | ||
| {"pod.alpha.kubernetes.io/opaque", true}, | ||
| {"example.com/gpu", false}, | ||
| {"foo", false}, | ||
| {"", false}, | ||
| } | ||
| for _, c := range cases { | ||
| if got := IsPrefixedNativeResource(c.name); got != c.want { | ||
| t.Errorf("IsPrefixedNativeResource(%q) = %v, want %v", c.name, got, c.want) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
To maintain consistency with the existing TestIsNativeResource in this file and follow Go best practices for table-driven tests, please use subtests (t.Run) and enable parallel execution (t.Parallel()). Additionally, using consistent field names like resourceName and expectVal improves maintainability.
func TestIsPrefixedNativeResource(t *testing.T) {
testCases := []struct {
resourceName corev1.ResourceName
expectVal bool
}{
{resourceName: "kubernetes.io/foo", expectVal: true},
{resourceName: "pod.alpha.kubernetes.io/opaque", expectVal: true},
{resourceName: "example.com/gpu", expectVal: false},
{resourceName: "foo", expectVal: false},
{resourceName: "", expectVal: false},
}
for _, tc := range testCases {
tc := tc
t.Run(string(tc.resourceName), func(t *testing.T) {
t.Parallel()
if got := IsPrefixedNativeResource(tc.resourceName); got != tc.expectVal {
t.Errorf("IsPrefixedNativeResource(%q) = %v, want %v", tc.resourceName, got, tc.expectVal)
}
})
}
}| func TestIsHugePageResourceName(t *testing.T) { | ||
| cases := []struct { | ||
| name corev1.ResourceName | ||
| want bool | ||
| }{ | ||
| {corev1.ResourceName(string(corev1.ResourceHugePagesPrefix) + "2Mi"), true}, | ||
| {corev1.ResourceName(string(corev1.ResourceHugePagesPrefix) + "1Gi"), true}, | ||
| {"huge-pages-2Mi", false}, // missing the hyphenated `hugepages-` prefix | ||
| {corev1.ResourceCPU, false}, | ||
| {"", false}, | ||
| } | ||
| for _, c := range cases { | ||
| if got := IsHugePageResourceName(c.name); got != c.want { | ||
| t.Errorf("IsHugePageResourceName(%q) = %v, want %v", c.name, got, c.want) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please refactor this test to use subtests and parallel execution for consistency and better observability, similar to the existing tests in this package.
func TestIsHugePageResourceName(t *testing.T) {
testCases := []struct {
resourceName corev1.ResourceName
expectVal bool
}{
{resourceName: corev1.ResourceName(string(corev1.ResourceHugePagesPrefix) + "2Mi"), expectVal: true},
{resourceName: corev1.ResourceName(string(corev1.ResourceHugePagesPrefix) + "1Gi"), expectVal: true},
{resourceName: "huge-pages-2Mi", expectVal: false}, // missing the hyphenated hugepages- prefix
{resourceName: corev1.ResourceCPU, expectVal: false},
{resourceName: "", expectVal: false},
}
for _, tc := range testCases {
tc := tc
t.Run(string(tc.resourceName), func(t *testing.T) {
t.Parallel()
if got := IsHugePageResourceName(tc.resourceName); got != tc.expectVal {
t.Errorf("IsHugePageResourceName(%q) = %v, want %v", tc.resourceName, got, tc.expectVal)
}
})
}
}| func TestIsAttachableVolumeResourceName(t *testing.T) { | ||
| cases := []struct { | ||
| name corev1.ResourceName | ||
| want bool | ||
| }{ | ||
| {corev1.ResourceName(string(corev1.ResourceAttachableVolumesPrefix) + "ebs"), true}, | ||
| {"attachable_volumes-foo", false}, // missing exact `attachable-volumes-` prefix | ||
| {corev1.ResourceMemory, false}, | ||
| {"", false}, | ||
| } | ||
| for _, c := range cases { | ||
| if got := IsAttachableVolumeResourceName(c.name); got != c.want { | ||
| t.Errorf("IsAttachableVolumeResourceName(%q) = %v, want %v", c.name, got, c.want) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please refactor this test to use subtests and parallel execution for consistency and better observability.
func TestIsAttachableVolumeResourceName(t *testing.T) {
testCases := []struct {
resourceName corev1.ResourceName
expectVal bool
}{
{resourceName: corev1.ResourceName(string(corev1.ResourceAttachableVolumesPrefix) + "ebs"), expectVal: true},
{resourceName: "attachable_volumes-foo", expectVal: false}, // missing exact attachable-volumes- prefix
{resourceName: corev1.ResourceMemory, expectVal: false},
{resourceName: "", expectVal: false},
}
for _, tc := range testCases {
tc := tc
t.Run(string(tc.resourceName), func(t *testing.T) {
t.Parallel()
if got := IsAttachableVolumeResourceName(tc.resourceName); got != tc.expectVal {
t.Errorf("IsAttachableVolumeResourceName(%q) = %v, want %v", tc.resourceName, got, tc.expectVal)
}
})
}
}| func TestIsExtendedResourceName(t *testing.T) { | ||
| cases := []struct { | ||
| name corev1.ResourceName | ||
| want bool | ||
| }{ | ||
| // Native resources are NOT extended. | ||
| {corev1.ResourceCPU, false}, | ||
| {"foo", false}, | ||
| // Names prefixed with requests. are NOT extended (would clash with quota convention). | ||
| {corev1.ResourceName(corev1.DefaultResourceRequestsPrefix + "example.com/gpu"), false}, | ||
| // Vendor-prefixed resource that satisfies IsQualifiedName once requests. is prepended IS extended. | ||
| {"example.com/gpu", true}, | ||
| // Invalid qualified name → not extended. | ||
| {"BAD!!", false}, | ||
| } | ||
| for _, c := range cases { | ||
| if got := IsExtendedResourceName(c.name); got != c.want { | ||
| t.Errorf("IsExtendedResourceName(%q) = %v, want %v", c.name, got, c.want) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please refactor this test to use subtests and parallel execution for consistency and better observability.
func TestIsExtendedResourceName(t *testing.T) {
testCases := []struct {
resourceName corev1.ResourceName
expectVal bool
}{
// Native resources are NOT extended.
{resourceName: corev1.ResourceCPU, expectVal: false},
{resourceName: "foo", expectVal: false},
// Names prefixed with requests. are NOT extended (would clash with quota convention).
{resourceName: corev1.ResourceName(corev1.DefaultResourceRequestsPrefix + "example.com/gpu"), expectVal: false},
// Vendor-prefixed resource that satisfies IsQualifiedName once requests. is prepended IS extended.
{resourceName: "example.com/gpu", expectVal: true},
// Invalid qualified name -> not extended.
{resourceName: "BAD!!", expectVal: false},
}
for _, tc := range testCases {
tc := tc
t.Run(string(tc.resourceName), func(t *testing.T) {
t.Parallel()
if got := IsExtendedResourceName(tc.resourceName); got != tc.expectVal {
t.Errorf("IsExtendedResourceName(%q) = %v, want %v", tc.resourceName, got, tc.expectVal)
}
})
}
}| func TestIsScalarResourceName(t *testing.T) { | ||
| cases := []struct { | ||
| name corev1.ResourceName | ||
| want bool | ||
| }{ | ||
| {"example.com/gpu", true}, // extended | ||
| {corev1.ResourceName(string(corev1.ResourceHugePagesPrefix) + "2Mi"), true}, | ||
| {corev1.ResourceName(string(corev1.ResourceAttachableVolumesPrefix) + "ebs"), true}, | ||
| {"kubernetes.io/foo", true}, // prefixed native | ||
| {corev1.ResourceCPU, false}, | ||
| {"foo", false}, | ||
| } | ||
| for _, c := range cases { | ||
| if got := IsScalarResourceName(c.name); got != c.want { | ||
| t.Errorf("IsScalarResourceName(%q) = %v, want %v", c.name, got, c.want) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If IsScalarResourceName is defined in resourcename.go (as implied by the PR description), this test should ideally be moved to a new file named resourcename_test.go to maintain proper code organization. Additionally, please apply the subtest and parallel execution pattern here as well.
func TestIsScalarResourceName(t *testing.T) {
testCases := []struct {
resourceName corev1.ResourceName
expectVal bool
}{
{resourceName: "example.com/gpu", expectVal: true}, // extended
{resourceName: corev1.ResourceName(string(corev1.ResourceHugePagesPrefix) + "2Mi"), expectVal: true},
{resourceName: corev1.ResourceName(string(corev1.ResourceAttachableVolumesPrefix) + "ebs"), expectVal: true},
{resourceName: "kubernetes.io/foo", expectVal: true}, // prefixed native
{resourceName: corev1.ResourceCPU, expectVal: false},
{resourceName: "foo", expectVal: false},
}
for _, tc := range testCases {
tc := tc
t.Run(string(tc.resourceName), func(t *testing.T) {
t.Parallel()
if got := IsScalarResourceName(tc.resourceName); got != tc.expectVal {
t.Errorf("IsScalarResourceName(%q) = %v, want %v", tc.resourceName, got, tc.expectVal)
}
})
}
}
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7467 +/- ##
==========================================
+ Coverage 41.90% 41.93% +0.03%
==========================================
Files 879 879
Lines 54326 54326
==========================================
+ Hits 22766 22784 +18
+ Misses 29833 29817 -16
+ Partials 1727 1725 -2
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:
|
|
Hi @apshada thanks for your contribution. |
Adds unit tests for the previously-uncovered helpers in
pkg/util/lifted/corev1helpers.goandresourcename.go. Pure test additions, no production-code changes.