From f79bccf4d99b16c855f70af44f1f2e7fdc817bc5 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 27 Feb 2025 09:32:52 -0500 Subject: [PATCH] validation unit test cleanups Fix some incorrect test case names. Use t.Run() in a few more places (to facilitate using SetFeatureGateDuringTest later). Clarify TestPodIPsValidation/TestHostIPsValidation (and fix weird indentation). --- pkg/apis/core/validation/validation_test.go | 151 ++++++++++-------- .../networking/validation/validation_test.go | 62 ++++--- 2 files changed, 119 insertions(+), 94 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 10b1795267c..eca9b3c5e08 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -9951,16 +9951,18 @@ func TestValidatePodDNSConfig(t *testing.T) { } for _, tc := range testCases { - if tc.dnsPolicy == nil { - tc.dnsPolicy = &testDNSClusterFirst - } + t.Run("", func(t *testing.T) { + if tc.dnsPolicy == nil { + tc.dnsPolicy = &testDNSClusterFirst + } - errs := validatePodDNSConfig(tc.dnsConfig, tc.dnsPolicy, field.NewPath("dnsConfig"), tc.opts) - if len(errs) != 0 && !tc.expectedError { - t.Errorf("%v: validatePodDNSConfig(%v) = %v, want nil", tc.desc, tc.dnsConfig, errs) - } else if len(errs) == 0 && tc.expectedError { - t.Errorf("%v: validatePodDNSConfig(%v) = nil, want error", tc.desc, tc.dnsConfig) - } + errs := validatePodDNSConfig(tc.dnsConfig, tc.dnsPolicy, field.NewPath("dnsConfig"), tc.opts) + if len(errs) != 0 && !tc.expectedError { + t.Errorf("%v: validatePodDNSConfig(%v) = %v, want nil", tc.desc, tc.dnsConfig, errs) + } else if len(errs) == 0 && tc.expectedError { + t.Errorf("%v: validatePodDNSConfig(%v) = nil, want error", tc.desc, tc.dnsConfig) + } + }) } } @@ -10304,12 +10306,14 @@ func TestValidatePodSpec(t *testing.T) { ), } for k, v := range failureCases { - opts := PodValidationOptions{ - ResourceIsPod: true, - } - if errs := ValidatePodSpec(&v.Spec, nil, field.NewPath("field"), opts); len(errs) == 0 { - t.Errorf("expected failure for %q", k) - } + t.Run(k, func(t *testing.T) { + opts := PodValidationOptions{ + ResourceIsPod: true, + } + if errs := ValidatePodSpec(&v.Spec, nil, field.NewPath("field"), opts); len(errs) == 0 { + t.Errorf("expected failure") + } + }) } } @@ -15302,35 +15306,35 @@ func TestValidateServiceCreate(t *testing.T) { // numErrs: 1, numErrs: 0, }, { - name: "invalid publicIPs localhost", + name: "invalid externalIPs localhost", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"127.0.0.1"} }, numErrs: 1, }, { - name: "invalid publicIPs unspecified", + name: "invalid externalIPs unspecified", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"0.0.0.0"} }, numErrs: 1, }, { - name: "invalid publicIPs loopback", + name: "invalid externalIPs loopback", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"127.0.0.1"} }, numErrs: 1, }, { - name: "invalid publicIPs host", + name: "invalid externalIPs host", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"myhost.mydomain"} }, numErrs: 1, }, { - name: "valid publicIPs", + name: "valid externalIPs", tweakSvc: func(s *core.Service) { s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"1.2.3.4"} @@ -16989,9 +16993,11 @@ func TestValidateNode(t *testing.T) { }, } for _, successCase := range successCases { - if errs := ValidateNode(&successCase); len(errs) != 0 { - t.Errorf("expected success: %v", errs) - } + t.Run("", func(t *testing.T) { + if errs := ValidateNode(&successCase); len(errs) != 0 { + t.Errorf("expected success: %v", errs) + } + }) } errorCases := map[string]core.Node{ @@ -17195,30 +17201,32 @@ func TestValidateNode(t *testing.T) { }, } for k, v := range errorCases { - errs := ValidateNode(&v) - if len(errs) == 0 { - t.Errorf("expected failure for %s", k) - } - for i := range errs { - field := errs[i].Field - expectedFields := map[string]bool{ - "metadata.name": true, - "metadata.labels": true, - "metadata.annotations": true, - "metadata.namespace": true, - "spec.externalID": true, - "spec.taints[0].key": true, - "spec.taints[0].value": true, - "spec.taints[0].effect": true, - "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature": true, - "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature.PodController.Controller": true, + t.Run(k, func(t *testing.T) { + errs := ValidateNode(&v) + if len(errs) == 0 { + t.Errorf("expected failure") } - if val, ok := expectedFields[field]; ok { - if !val { - t.Errorf("%s: missing prefix for: %v", k, errs[i]) + for i := range errs { + field := errs[i].Field + expectedFields := map[string]bool{ + "metadata.name": true, + "metadata.labels": true, + "metadata.annotations": true, + "metadata.namespace": true, + "spec.externalID": true, + "spec.taints[0].key": true, + "spec.taints[0].value": true, + "spec.taints[0].effect": true, + "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature": true, + "metadata.annotations.scheduler.alpha.kubernetes.io/preferAvoidPods[0].PodSignature.PodController.Controller": true, + } + if val, ok := expectedFields[field]; ok { + if !val { + t.Errorf("missing prefix for: %v", errs[i]) + } } } - } + }) } } @@ -22825,28 +22833,32 @@ func makePod(podName string, podNamespace string, podIPs []core.PodIP) core.Pod } } func TestPodIPsValidation(t *testing.T) { + // We test updating every pod in testCases to every other pod in testCases. + // expectError is true if we expect an error when updating *to* that pod. + testCases := []struct { pod core.Pod expectError bool - }{{ - expectError: false, - pod: makePod("nil-ips", "ns", nil), - }, { - expectError: false, - pod: makePod("empty-podips-list", "ns", []core.PodIP{}), - }, { - expectError: false, - pod: makePod("single-ip-family-6", "ns", []core.PodIP{{IP: "::1"}}), - }, { - expectError: false, - pod: makePod("single-ip-family-4", "ns", []core.PodIP{{IP: "1.1.1.1"}}), - }, { - expectError: false, - pod: makePod("dual-stack-4-6", "ns", []core.PodIP{{IP: "1.1.1.1"}, {IP: "::1"}}), - }, { - expectError: false, - pod: makePod("dual-stack-6-4", "ns", []core.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), - }, + }{ + { + expectError: false, + pod: makePod("nil-ips", "ns", nil), + }, { + expectError: false, + pod: makePod("empty-podips-list", "ns", []core.PodIP{}), + }, { + expectError: false, + pod: makePod("single-ip-family-6", "ns", []core.PodIP{{IP: "::1"}}), + }, { + expectError: false, + pod: makePod("single-ip-family-4", "ns", []core.PodIP{{IP: "1.1.1.1"}}), + }, { + expectError: false, + pod: makePod("dual-stack-4-6", "ns", []core.PodIP{{IP: "1.1.1.1"}, {IP: "::1"}}), + }, { + expectError: false, + pod: makePod("dual-stack-6-4", "ns", []core.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + }, /* failure cases start here */ { expectError: true, @@ -22887,10 +22899,10 @@ func TestPodIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) if len(errs) == 0 && testCase.expectError { - t.Fatalf("expected failure for %s, but there were none", testCase.pod.Name) + t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { - t.Fatalf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs) + t.Fatalf("expected success updating from %s, but there were errors: %v", oldTestCase.pod.Name, errs) } } }) @@ -22921,6 +22933,9 @@ func makePodWithHostIPs(podName string, podNamespace string, hostIPs []core.Host } func TestHostIPsValidation(t *testing.T) { + // We test updating every pod in testCases to every other pod in testCases. + // expectError is true if we expect an error when updating *to* that pod. + testCases := []struct { pod core.Pod expectError bool @@ -22952,7 +22967,7 @@ func TestHostIPsValidation(t *testing.T) { /* failure cases start here */ { expectError: true, - pod: makePodWithHostIPs("invalid-pod-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), + pod: makePodWithHostIPs("invalid-host-ip", "ns", []core.HostIP{{IP: "this-is-not-an-ip"}}), }, { expectError: true, @@ -22994,10 +23009,10 @@ func TestHostIPsValidation(t *testing.T) { errs := ValidatePodStatusUpdate(newPod, oldPod, PodValidationOptions{}) if len(errs) == 0 && testCase.expectError { - t.Fatalf("expected failure for %s, but there were none", testCase.pod.Name) + t.Fatalf("expected failure updating from %s, but there were none", oldTestCase.pod.Name) } if len(errs) != 0 && !testCase.expectError { - t.Fatalf("expected success for %s, but there were errors: %v", testCase.pod.Name, errs) + t.Fatalf("expected success updating from %s, but there were errors: %v", oldTestCase.pod.Name, errs) } } }) diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index 7b2ad6f0515..daf22967bf4 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -249,10 +249,12 @@ func TestValidateNetworkPolicy(t *testing.T) { // Success cases are expected to pass validation. - for k, v := range successCases { - if errs := ValidateNetworkPolicy(v, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) != 0 { - t.Errorf("Expected success for the success validation test number %d, got %v", k, errs) - } + for _, v := range successCases { + t.Run("", func(t *testing.T) { + if errs := ValidateNetworkPolicy(v, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) != 0 { + t.Errorf("Expected success, got %v", errs) + } + }) } invalidSelector := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"} @@ -367,9 +369,11 @@ func TestValidateNetworkPolicy(t *testing.T) { // Error cases are not expected to pass validation. for testName, networkPolicy := range errorCases { - if errs := ValidateNetworkPolicy(networkPolicy, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) == 0 { - t.Errorf("Expected failure for test: %s", testName) - } + t.Run(testName, func(t *testing.T) { + if errs := ValidateNetworkPolicy(networkPolicy, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) == 0 { + t.Errorf("Expected failure") + } + }) } } @@ -420,11 +424,13 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { } for testName, successCase := range successCases { - successCase.old.ObjectMeta.ResourceVersion = "1" - successCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{false}); len(errs) != 0 { - t.Errorf("expected success (%s): %v", testName, errs) - } + t.Run(testName, func(t *testing.T) { + successCase.old.ObjectMeta.ResourceVersion = "1" + successCase.update.ObjectMeta.ResourceVersion = "1" + if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{false}); len(errs) != 0 { + t.Errorf("expected success, but got %v", errs) + } + }) } errorCases := map[string]npUpdateTest{ @@ -447,11 +453,13 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { } for testName, errorCase := range errorCases { - errorCase.old.ObjectMeta.ResourceVersion = "1" - errorCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{false}); len(errs) == 0 { - t.Errorf("expected failure: %s", testName) - } + t.Run(testName, func(t *testing.T) { + errorCase.old.ObjectMeta.ResourceVersion = "1" + errorCase.update.ObjectMeta.ResourceVersion = "1" + if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{false}); len(errs) == 0 { + t.Errorf("expected failure") + } + }) } } @@ -1824,16 +1832,18 @@ func TestValidateIngressStatusUpdate(t *testing.T) { "status.loadBalancer.ingress[0].hostname: Invalid value": invalidHostname, } for k, v := range errorCases { - errs := ValidateIngressStatusUpdate(&v, &oldValue) - if len(errs) == 0 { - t.Errorf("expected failure for %s", k) - } else { - s := strings.Split(k, ":") - err := errs[0] - if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) { - t.Errorf("unexpected error: %q, expected: %q", err, k) + t.Run(k, func(t *testing.T) { + errs := ValidateIngressStatusUpdate(&v, &oldValue) + if len(errs) == 0 { + t.Errorf("expected failure") + } else { + s := strings.Split(k, ":") + err := errs[0] + if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) { + t.Errorf("unexpected error: %q, expected: %q", err, k) + } } - } + }) } }