test: add unit tests for SetLabelsAndAnnotationsForWorkload and GetWorkSuspendDispatching#7675
Conversation
…rkSuspendDispatching Add unit tests for two functions in pkg/util/work.go that lacked coverage: - GetWorkSuspendDispatching: covers true, false, and nil SuspendDispatching pointer (defaults to false) - SetLabelsAndAnnotationsForWorkload: covers the managed-annotations and managed-labels annotation population, and propagation of the WorkPermanentIDLabel from the Work object to the workload Fixes karmada-io#7673 Signed-off-by: Goutham Annem <annem.usedu@gmail.com>
karmada-bot
added
kind/feature
|
Invalid commit message issues detected Invalid commit messagesKeywords which can automatically close issues and hashtag(#) mentions are not allowed.
Instructions 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. I understand the commands that are listed here. |
|
[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 reliability of the codebase by introducing unit tests for two previously untested utility functions. The changes ensure that workload suspension logic and label/annotation propagation behave as expected under various conditions, addressing a known gap in test coverage. 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
|
karmada-bot
added
the
size/L
![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 in pkg/util/work_test.go to verify GetWorkSuspendDispatching and SetLabelsAndAnnotationsForWorkload. The review feedback highlights that TestSetLabelsAndAnnotationsForWorkload contains unused struct fields and duplicate test cases that do not actually set the permanent ID label as named. It is recommended to simplify this test to a single basic test case, as permanent ID propagation is already thoroughly tested in a separate dedicated test.
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.
| tests := []struct { | ||
| name string | ||
| workload *unstructured.Unstructured | ||
| work *workv1alpha1.Work | ||
| wantLabelKey string | ||
| wantLabelValue string | ||
| wantAnnotationKey string | ||
| }{ | ||
| { | ||
| name: "work has permanent ID label — propagated to workload", | ||
| workload: &unstructured.Unstructured{ | ||
| Object: map[string]any{ | ||
| "apiVersion": "apps/v1", | ||
| "kind": "Deployment", | ||
| "metadata": map[string]any{ | ||
| "name": "demo", | ||
| "namespace": "default", | ||
| }, | ||
| }, | ||
| }, | ||
| work: &workv1alpha1.Work{ | ||
| Spec: workv1alpha1.WorkSpec{}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "work has permanent ID label set", | ||
| workload: &unstructured.Unstructured{ | ||
| Object: map[string]any{ | ||
| "apiVersion": "apps/v1", | ||
| "kind": "Deployment", | ||
| "metadata": map[string]any{ | ||
| "name": "demo", | ||
| "namespace": "default", | ||
| }, | ||
| }, | ||
| }, | ||
| work: &workv1alpha1.Work{ | ||
| Spec: workv1alpha1.WorkSpec{}, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The table-driven test TestSetLabelsAndAnnotationsForWorkload contains several issues:
- The struct fields
wantLabelKey,wantLabelValue, andwantAnnotationKeyare defined but never used in the test cases or assertions. - The two test cases are completely identical and neither of them actually sets the permanent ID label on the
workobject, despite their names indicating they do. - Since permanent ID propagation is already thoroughly tested in
TestSetLabelsAndAnnotationsForWorkload_WithPermanentID, we can simplify this test to a single basic test case and remove the unused fields.
tests := []struct {
name string
workload *unstructured.Unstructured
work *workv1alpha1.Work
}{
{
name: "basic workload and work",
workload: &unstructured.Unstructured{
Object: map[string]any{
"apiVersion": "apps/v1",
"kind": "Deployment",
"metadata": map[string]any{
"name": "demo",
"namespace": "default",
},
},
},
work: &workv1alpha1.Work{
Spec: workv1alpha1.WorkSpec{},
},
},
}There was a problem hiding this comment.
Pull request overview
Adds missing unit test coverage in pkg/util/work_test.go for two helper functions in pkg/util/work.go, improving regression protection around suspend-dispatching indexing and workload metadata management.
Changes:
- Add table-driven tests for
GetWorkSuspendDispatchingcoveringtrue,false, and nilSuspendDispatching(defaulting tofalse). - Add tests for
SetLabelsAndAnnotationsForWorkloadensuring managed-annotations/managed-labels are set, plus a dedicated test forWorkPermanentIDLabelpropagation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wantLabelKey string | ||
| wantLabelValue string | ||
| wantAnnotationKey string | ||
| }{ |
| wantAnnotationKey string | ||
| }{ | ||
| { | ||
| name: "work has permanent ID label — propagated to workload", |
| { | ||
| name: "work has permanent ID label set", | ||
| workload: &unstructured.Unstructured{ | ||
| Object: map[string]any{ | ||
| "apiVersion": "apps/v1", | ||
| "kind": "Deployment", | ||
| "metadata": map[string]any{ | ||
| "name": "demo", | ||
| "namespace": "default", | ||
| }, | ||
| }, | ||
| }, | ||
| work: &workv1alpha1.Work{ | ||
| Spec: workv1alpha1.WorkSpec{}, | ||
| }, | ||
| }, |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7675 +/- ##
=======================================
Coverage 42.06% 42.06%
=======================================
Files 879 879
Lines 54831 54831
=======================================
+ Hits 23063 23067 +4
+ Misses 30023 30017 -6
- Partials 1745 1747 +2
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:
|
|
Hi, just bumping this for visibility. Happy to address any feedback or questions! |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Two functions in
pkg/util/work.golacked unit test coverage. This PR adds tests for:GetWorkSuspendDispatching— coverstrue,false, andnilSuspendDispatchingpointer (nil defaults tofalse)SetLabelsAndAnnotationsForWorkload— verifies that themanaged-annotationsandmanaged-labelsannotations are always set on the workload, and that theWorkPermanentIDLabelis propagated from the Work object to the workload when presentWhich issue(s) this PR fixes:
Fixes #7673
Special notes for your reviewer:
All new tests follow the table-driven pattern and use
github.com/stretchr/testify/assertconsistent with the existingwork_test.go.Does this PR introduce a user-facing change?: