test: add unit tests for controller util package#2395
test: add unit tests for controller util package#2395archy-rock3t-cloud wants to merge 1 commit intoopenkruise:masterfrom
Conversation
|
[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 |
|
Welcome @archy-rock3t-cloud! It looks like this is your first PR to openkruise/kruise 🎉 |
There was a problem hiding this comment.
Pull request overview
Adds unit tests for helper functions in pkg/controller/util, improving confidence in controller utility behaviors (workload object creation-by-key, StatefulSet pod ordinal parsing, and pod-condition timeout/message helpers).
Changes:
- Add tests for
GetEmptyObjectWithKey, including typed objects andunstructured.UnstructuredGVK preservation. - Add tests for StatefulSet-style pod name parsing (
getParentNameAndOrdinal,GetOrdinal). - Add tests for pod-condition message KV parsing/round-trip and pending/update timeout calculations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/controller/util/workload_utils_test.go | Adds coverage for GetEmptyObjectWithKey across several supported workload types + unstructured objects. |
| pkg/controller/util/statefulset_utils_test.go | Adds table-driven tests for StatefulSet pod name/ordinal parsing helpers. |
| pkg/controller/util/pod_condition_utils_test.go | Adds tests for condition message KV parsing/marshaling and timeout helper logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.True(t, ok) | ||
| assert.Equal(t, gvk, u.GroupVersionKind()) | ||
| assert.Equal(t, "my-foo", u.GetName()) | ||
| assert.Equal(t, "bar", u.GetNamespace()) | ||
| // Labels should not carry over | ||
| assert.Empty(t, u.GetLabels()) |
There was a problem hiding this comment.
assert.True(t, ok) doesn’t stop the test, so if the type assertion ever fails this test will panic on the next lines when u is nil. Consider using assert.IsType or guarding the rest of the assertions with if assert.True(t, ok) { ... } so failures are reported cleanly without a nil dereference.
| assert.True(t, ok) | |
| assert.Equal(t, gvk, u.GroupVersionKind()) | |
| assert.Equal(t, "my-foo", u.GetName()) | |
| assert.Equal(t, "bar", u.GetNamespace()) | |
| // Labels should not carry over | |
| assert.Empty(t, u.GetLabels()) | |
| if assert.True(t, ok) { | |
| assert.Equal(t, gvk, u.GroupVersionKind()) | |
| assert.Equal(t, "my-foo", u.GetName()) | |
| assert.Equal(t, "bar", u.GetNamespace()) | |
| // Labels should not carry over | |
| assert.Empty(t, u.GetLabels()) | |
| } |
| func TestGetEmptyObjectWithKey(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| object client.Object | ||
| wantType client.Object | ||
| }{ | ||
| { | ||
| name: "Pod", | ||
| object: &v1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "my-pod", Namespace: "default", Labels: map[string]string{"app": "test"}}, | ||
| Spec: v1.PodSpec{NodeName: "node-1"}, | ||
| }, | ||
| wantType: &v1.Pod{}, | ||
| }, | ||
| { | ||
| name: "Service", | ||
| object: &v1.Service{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "my-svc", Namespace: "kube-system"}, | ||
| }, | ||
| wantType: &v1.Service{}, | ||
| }, | ||
| { | ||
| name: "Ingress", | ||
| object: &networkingv1.Ingress{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "my-ingress", Namespace: "web"}, | ||
| }, | ||
| wantType: &networkingv1.Ingress{}, | ||
| }, | ||
| { | ||
| name: "Deployment", | ||
| object: &apps.Deployment{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "my-deploy", Namespace: "prod"}, | ||
| }, | ||
| wantType: &apps.Deployment{}, | ||
| }, | ||
| { | ||
| name: "ReplicaSet", | ||
| object: &apps.ReplicaSet{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "my-rs", Namespace: "default"}, | ||
| }, | ||
| wantType: &apps.ReplicaSet{}, |
There was a problem hiding this comment.
These cases only cover the currently enumerated types. GetEmptyObjectWithKey will panic for any unsupported client.Object because empty stays nil and empty.SetName(...) is called. Consider adding a test case for an unsupported object type (and then either assert the panic explicitly, or—preferably—update the helper to return a safe default / error) so this behavior is exercised and doesn’t regress silently.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2395 +/- ##
==========================================
+ Coverage 48.77% 48.87% +0.09%
==========================================
Files 324 324
Lines 27928 27928
==========================================
+ Hits 13623 13650 +27
+ Misses 12775 12751 -24
+ Partials 1530 1527 -3
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:
|
archy-rock3t-cloud
force-pushed
the
test/controller-util-coverage
branch
from
e45e6f6 to
bc4c0fe
Compare
Signed-off-by: Artem Muterko <artem@sopho.tech>
archy-rock3t-cloud
force-pushed
the
test/controller-util-coverage
branch
from
bc4c0fe to
4dbdc29
Compare
Ⅰ. Describe what this PR does
Add unit tests for the previously untested
pkg/controller/utilpackage.Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. Describe how to verify it
go test ./pkg/controller/util/ -v -count=1Ⅳ. Special notes for reviews