From 49ec35fa6a22a73f2236b730dc878f011e199f9e Mon Sep 17 00:00:00 2001 From: tariq-hasan Date: Fri, 12 Apr 2024 03:45:00 -0400 Subject: [PATCH] Introduced cmp.Diff for error comparison Signed-off-by: tariq-hasan --- pkg/webhook/v1beta1/pod/inject_webhook.go | 10 +-- .../v1beta1/pod/inject_webhook_test.go | 62 ++++++++++++------- 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/pkg/webhook/v1beta1/pod/inject_webhook.go b/pkg/webhook/v1beta1/pod/inject_webhook.go index fc344eac104..f5270528848 100644 --- a/pkg/webhook/v1beta1/pod/inject_webhook.go +++ b/pkg/webhook/v1beta1/pod/inject_webhook.go @@ -49,10 +49,10 @@ import ( var log = logf.Log.WithName("injector-webhook") var ( - errInvalidOwnerAPIVersion = errors.New("invalid owner API version") - errInvalidSuggestionName = errors.New("invalid suggestion name") - errPodNotBelongToKatibJob = errors.New("pod does not belong to Katib Job") - errNestedObjectNotFound = errors.New("nested object not found in the cluster") + errInvalidOwnerAPIVersion = errors.New("invalid owner API version") + errInvalidSuggestionName = errors.New("invalid suggestion name") + errPodNotBelongToKatibJob = errors.New("pod does not belong to Katib Job") + errFailedToGetTrialTemplateJob = errors.New("unable to get Job in the trialTemplate") ) // SidecarInjector injects metrics collect sidecar to the primary pod. @@ -280,7 +280,7 @@ func (s *SidecarInjector) getKatibJob(object *unstructured.Unstructured, namespa // Nested object namespace must be equal to object namespace err = s.client.Get(context.TODO(), apitypes.NamespacedName{Name: owners[i].Name, Namespace: namespace}, nestedJob) if err != nil { - return "", "", fmt.Errorf("%w: %w", errNestedObjectNotFound, err) + return "", "", fmt.Errorf("%w: %w", errFailedToGetTrialTemplateJob, err) } // Recursively search for Trial ownership in nested object jobKind, jobName, err = s.getKatibJob(nestedJob, namespace) diff --git a/pkg/webhook/v1beta1/pod/inject_webhook_test.go b/pkg/webhook/v1beta1/pod/inject_webhook_test.go index dd00ef49024..db8baa65274 100644 --- a/pkg/webhook/v1beta1/pod/inject_webhook_test.go +++ b/pkg/webhook/v1beta1/pod/inject_webhook_test.go @@ -31,7 +31,6 @@ import ( batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -161,7 +160,16 @@ func TestWrapWorkerContainer(t *testing.T) { }, }, }, - PathKind: common.FileKind, + PathKind: common.FileKind, + WantPod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "not-primary-container", + }, + }, + }, + }, WantError: errPrimaryContainerNotFound, }, "Container with early stopping command": { @@ -218,8 +226,8 @@ func TestWrapWorkerContainer(t *testing.T) { if diff := cmp.Diff(tc.WantError, err, cmpopts.EquateErrors()); len(diff) != 0 { t.Errorf("Unexpected error from wrapWorkerContainer (-want,+got):\n%s", diff) } - if err == nil && !equality.Semantic.DeepEqual(tc.Pod.Spec.Containers, tc.WantPod.Spec.Containers) { - t.Errorf("Unexpected error from wrapWorkerContainer, expected pod: %v, got: %v", tc.WantPod.Spec.Containers, tc.Pod.Spec.Containers) + if diff := cmp.Diff(tc.WantPod.Spec.Containers, tc.Pod.Spec.Containers); len(diff) != 0 { + t.Errorf("Unexpected pod from wrapWorkerContainer (-want,+got):\n%s", diff) } }) } @@ -636,31 +644,33 @@ func TestMutateMetricsCollectorVolume(t *testing.T) { if diff := cmp.Diff(tc.WantError, err, cmpopts.EquateErrors()); len(diff) != 0 { t.Errorf("Unexpected error from mutateMetricsCollectorVolume (-want,+got):\n%s", diff) } - if err == nil && !equality.Semantic.DeepEqual(tc.Pod, tc.WantPod) { - t.Errorf("Unexpected error from mutateMetricsCollectorVolume, expected pod: %v, got: %v", tc.WantPod, tc.Pod) + if diff := cmp.Diff(tc.WantPod, tc.Pod); len(diff) != 0 { + t.Errorf("Unexpected pod from mutateMetricsCollectorVolume (-want,+got):\n%s", diff) } } func TestGetSidecarContainerName(t *testing.T) { - cases := []struct { + cases := map[string]struct { CollectorKind common.CollectorKind WantCollectorKind string }{ - { + "Valid run with StdOutCollector": { CollectorKind: common.StdOutCollector, WantCollectorKind: mccommon.MetricLoggerCollectorContainerName, }, - { + "Valid run with TfEventCollector": { CollectorKind: common.TfEventCollector, WantCollectorKind: mccommon.MetricCollectorContainerName, }, } - for _, tc := range cases { - collectorKind := getSidecarContainerName(tc.CollectorKind) - if collectorKind != tc.WantCollectorKind { - t.Errorf("Expected Collector Kind: %v, got %v", tc.WantCollectorKind, collectorKind) - } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + collectorKind := getSidecarContainerName(tc.CollectorKind) + if collectorKind != tc.WantCollectorKind { + t.Errorf("Expected Collector Kind: %v, got %v", tc.WantCollectorKind, collectorKind) + } + }) } } @@ -880,7 +890,7 @@ func TestGetKatibJob(t *testing.T) { }, }, }, - WantError: errNestedObjectNotFound, + WantError: errFailedToGetTrialTemplateJob, }, "Run when Pod owns Job with invalid API version": { Pod: &v1.Pod{ @@ -986,10 +996,12 @@ func TestIsPrimaryPod(t *testing.T) { } for name, tc := range cases { - isPrimary := isPrimaryPod(tc.podLabels, tc.primaryPodLabels) - if isPrimary != tc.isPrimary { - t.Errorf("Case %v. Expected isPrimary %v, got %v", name, tc.isPrimary, isPrimary) - } + t.Run(name, func(t *testing.T) { + isPrimary := isPrimaryPod(tc.podLabels, tc.primaryPodLabels) + if isPrimary != tc.isPrimary { + t.Errorf("Case %v. Expected isPrimary %v, got %v", name, tc.isPrimary, isPrimary) + } + }) } } @@ -1029,10 +1041,12 @@ func TestMutatePodMetadata(t *testing.T) { }, } - for _, tc := range cases { - mutatePodMetadata(tc.pod, tc.trial) - if diff := cmp.Diff(tc.mutatedPod, tc.pod); len(diff) != 0 { - t.Errorf("Unexpected pod from mutatePodMetadata (-want,+got):\n%s", diff) - } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + mutatePodMetadata(tc.pod, tc.trial) + if diff := cmp.Diff(tc.mutatedPod, tc.pod); len(diff) != 0 { + t.Errorf("Unexpected pod from mutatePodMetadata (-want,+got):\n%s", diff) + } + }) } }