test: add unit tests for GenFieldMergePatch and binding helper functions#7614
test: add unit tests for GenFieldMergePatch and binding helper functions#7614tushar-pandhare wants to merge 2 commits into
Conversation
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 coverage for utility functions within the project. By adding unit tests for key helper functions responsible for patch generation and resource binding retrieval, the changes ensure more robust verification of logic and prevent potential regressions in future updates. 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 the 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 counterproductive. 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
|
|
[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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds unit tests for additional helper functions to validate merge-patch generation and binding retrieval behavior.
Changes:
- Added table-driven tests for
GenFieldMergePatch. - Added fake-client based tests for
GetClusterResourceBindings,GetResourceBindings, andGetResourceBindingsByNamespace. - Introduced a new dependency on
k8s.io/apimachinery/pkg/labelsin binding helper tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/util/helper/patch_test.go | Adds coverage for field-scoped merge patch generation via GenFieldMergePatch. |
| pkg/util/helper/binding_test.go | Adds coverage for fetching bindings using label selectors and namespace scoping with a fake controller-runtime client. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: "no change returns nil", | ||
| fieldName: "spec", | ||
| originField: map[string]string{"key": "value"}, | ||
| modifiedField: map[string]string{"key": "value"}, | ||
| expectedPatch: "", | ||
| expectErr: false, |
| if string(patch) != tt.expectedPatch { | ||
| t.Fatalf("want patch: %s, but got: %s", tt.expectedPatch, string(patch)) | ||
| } |
| fieldName: "spec", | ||
| originField: map[string]string{"key": "old"}, | ||
| modifiedField: map[string]string{"key": "new"}, | ||
| expectedPatch: `{"spec":{"key":"new"}}`, |
| fieldName: "metadata", | ||
| originField: map[string]any{"name": "foo", "labels": map[string]string{"env": "dev"}}, | ||
| modifiedField: map[string]any{"name": "foo", "labels": map[string]string{"env": "prod"}}, | ||
| expectedPatch: `{"metadata":{"labels":{"env":"prod"}}}`, |
| if string(patch) != tt.expectedPatch { | ||
| t.Fatalf("want patch: %s, but got: %s", tt.expectedPatch, string(patch)) | ||
| } |
| import ( | ||
| "context" | ||
| "k8s.io/apimachinery/pkg/labels" | ||
| "reflect" | ||
| "testing" |
There was a problem hiding this comment.
Code Review
This pull request introduces unit tests for several helper functions, including GetClusterResourceBindings, GetResourceBindings, and GetResourceBindingsByNamespace in binding_test.go, and GenFieldMergePatch in patch_test.go. The review feedback recommends separating standard library imports from third-party imports in binding_test.go to follow standard Go practices, and adding an error-path test case to TestGenFieldMergePatch to ensure complete test coverage for error handling.
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.
| "context" | ||
| "k8s.io/apimachinery/pkg/labels" | ||
| "reflect" | ||
| "testing" |
There was a problem hiding this comment.
According to standard Go best practices (enforced by goimports), standard library imports should be grouped separately from third-party/external imports.
Please move "k8s.io/apimachinery/pkg/labels" to the second import group containing the other k8s.io packages.
| "context" | |
| "k8s.io/apimachinery/pkg/labels" | |
| "reflect" | |
| "testing" | |
| "context" | |
| "reflect" | |
| "testing" |
References
- These rules supplement the official Go style guide. Where this guide is silent, standard Go best practices apply. (link)
| { | ||
| name: "nested field change", | ||
| fieldName: "metadata", | ||
| originField: map[string]any{"name": "foo", "labels": map[string]string{"env": "dev"}}, | ||
| modifiedField: map[string]any{"name": "foo", "labels": map[string]string{"env": "prod"}}, | ||
| expectedPatch: `{"metadata":{"labels":{"env":"prod"}}}`, | ||
| expectErr: false, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The test suite for GenFieldMergePatch defines an expectErr field in the test cases struct, but none of the current test cases actually test the error path (expectErr: true).
Adding a test case with an invalid input (such as a channel, which cannot be marshaled to JSON) will ensure that the error handling of GenFieldMergePatch is fully covered.
{
name: "nested field change",
fieldName: "metadata",
originField: map[string]any{"name": "foo", "labels": map[string]string{"env": "dev"}},
modifiedField: map[string]any{"name": "foo", "labels": map[string]string{"env": "prod"}},
expectedPatch: "{\"metadata\":{\"labels\":{\"env\":\"prod\"}}}",
expectErr: false,
},
{
name: "invalid input returns error",
fieldName: "spec",
originField: make(chan int),
modifiedField: map[string]string{"key": "value"},
expectedPatch: "",
expectErr: true,
},
}9ae28c0 to
37d0cd2
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7614 +/- ##
==========================================
+ Coverage 42.16% 42.17% +0.01%
==========================================
Files 879 879
Lines 54669 54669
==========================================
+ Hits 23053 23059 +6
+ Misses 29870 29861 -9
- Partials 1746 1749 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Tushar Pandhare <tusharpandharetp@gmail.com>
37d0cd2 to
bbc020c
Compare
|
/cc @RainbowMango Hi! Could you please review this PR when you get a chance? |
|
Adding label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/kind cleanup
What this PR does / why we need it:
Add unit tests for functions in pkg/util/helper that lacked test coverage:
GenFieldMergePatch(patch.go) - tests no change, field changed, nested field changeGetClusterResourceBindings(binding.go) - tests no bindings, matching binding, non-matching bindingGetResourceBindings(binding.go) - tests no bindings, matching binding, non-matching bindingGetResourceBindingsByNamespace(binding.go) - tests no bindings, binding in namespace, binding in different namespaceWhich issue(s) this PR fixes:
Fixes : #7607
Special notes for your reviewer:
All existing tests pass with the new additions:
ok github.com/karmada-io/karmada/pkg/util/helper 1.441s
Does this PR introduce a user-facing change?: