address comments

Signed-off-by: Jian Zeng <[email protected]>

apis/kueue/v1alpha1/workload_webhook.go

@@ -60,20 +60,20 @@ var _ webhook.Validator = &Workload{}
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (r *Workload) ValidateCreate() error {
-	return validateWorkload(r).ToAggregate()
+	return ValidateWorkload(r).ToAggregate()
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
 func (r *Workload) ValidateUpdate(old runtime.Object) error {
-	return validateWorkload(r).ToAggregate()
+	return ValidateWorkload(r).ToAggregate()
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (r *Workload) ValidateDelete() error {
 	return nil
 }
 
-func validateWorkload(obj *Workload) field.ErrorList {
+func ValidateWorkload(obj *Workload) field.ErrorList {
 	var allErrs field.ErrorList
 	specField := field.NewPath("spec")
 	podSetsField := specField.Child("podSets")

apis/kueue/v1alpha1/workload_webhook_test.go

@@ -1,79 +1,56 @@
-package v1alpha1
+// Rename the package to avoid circular dependencies which is caused by "sigs.k8s.io/kueue/pkg/util/testing".
+// See also: https://github.com/golang/go/wiki/CodeReviewComments#import-dot
+package v1alpha1_test
 
 import (
-	"reflect"
 	"testing"
 
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"k8s.io/apimachinery/pkg/util/validation/field"
-)
 
-// makeWorkload is copied from "sigs.k8s.io/kueue/pkg/util/testing"
-// NOTE: we could not import "sigs.k8s.io/kueue/pkg/util/testing" because of cyclic import.
-func makeWorkload(override func(*Workload)) *Workload {
-	obj := &Workload{
-		ObjectMeta: metav1.ObjectMeta{Name: "name", Namespace: "ns"},
-		Spec: WorkloadSpec{
-			PodSets: []PodSet{
-				{
-					Name:  "main",
-					Count: 1,
-					Spec: corev1.PodSpec{
-						Containers: []corev1.Container{
-							{
-								Name: "c",
-								Resources: corev1.ResourceRequirements{
-									Requests: make(corev1.ResourceList),
-								},
-							},
-						},
-					},
-				},
-			},
-		},
-	}
-	override(obj)
-	return obj
-}
+	. "sigs.k8s.io/kueue/apis/kueue/v1alpha1"
+	testingutil "sigs.k8s.io/kueue/pkg/util/testing"
+)
 
 func TestValidateWorkload(t *testing.T) {
+	const (
+		objName = "name"
+		objNs   = "ns"
+	)
 	specField := field.NewPath("spec")
 	podSetsField := specField.Child("podSets")
 	testCases := map[string]struct {
-		workload    *Workload
-		expectedErr field.ErrorList
+		workload *Workload
+		wantErr  field.ErrorList
 	}{
 		"should have at least one podSet": {
-			workload: makeWorkload(func(w *Workload) {
-				w.Spec.PodSets = nil
-			}),
-			expectedErr: field.ErrorList{
-				field.Required(podSetsField, "at least one podSet is required"),
+			workload: testingutil.MakeWorkload(objName, objNs).PodSets(nil).Obj(),
+			wantErr: field.ErrorList{
+				field.Required(podSetsField, ""),
 			},
 		},
 		"count should be greater than 0": {
-			workload: makeWorkload(func(w *Workload) {
-				w.Spec.PodSets[0].Count = -1
-			}),
-			expectedErr: field.ErrorList{
-				field.Invalid(podSetsField.Index(0).Child("count"), int32(-1), "count must be greater than 0"),
+			workload: testingutil.MakeWorkload(objName, objNs).PodSetCount(-1).Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(podSetsField.Index(0).Child("count"), int32(-1), ""),
 			},
 		},
 		"should have valid priorityClassName": {
-			workload: makeWorkload(func(w *Workload) {
-				w.Spec.PriorityClassName = "invalid_class"
-			}),
-			expectedErr: field.ErrorList{
-				field.Invalid(specField.Child("priorityClassName"), "invalid_class", "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"),
+			workload: testingutil.MakeWorkload(objName, objNs).PriorityClass("invalid_class").Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(specField.Child("priorityClassName"), "invalid_class", ""),
 			},
 		},
 	}
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {
-			errList := validateWorkload(tc.workload)
-			if !reflect.DeepEqual(errList, tc.expectedErr) {
-				t.Errorf("expected: %v\nbut got: %v", tc.expectedErr, errList)
+			errList := ValidateWorkload(tc.workload)
+			if len(errList) != 1 {
+				t.Errorf("Unexpected error: %v, want %v", errList, tc.wantErr)
+			}
+			if diff := cmp.Diff(tc.wantErr[0], errList[0], cmpopts.IgnoreFields(field.Error{}, "Detail")); diff != "" {
+				t.Errorf("ValidateWorkload() mismatch (-want +got):\n%s", diff)
 			}
 		})
 	}

pkg/util/testing/wrappers.go

@@ -199,6 +199,16 @@ func (w *WorkloadWrapper) RuntimeClass(name string) *WorkloadWrapper {
 	return w
 }
 
+func (w *WorkloadWrapper) PodSets(podSets []kueue.PodSet) *WorkloadWrapper {
+	w.Spec.PodSets = podSets
+	return w
+}
+
+func (w *WorkloadWrapper) PodSetCount(count int32) *WorkloadWrapper {
+	w.Spec.PodSets[0].Count = count
+	return w
+}
+
 // AdmissionWrapper wraps an Admission
 type AdmissionWrapper struct{ kueue.Admission }
 

test/integration/webhook/v1alpha1/workload_test.go

@@ -14,6 +14,9 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"net/http"
+
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/onsi/ginkgo/v2"
 	"github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
@@ -65,15 +68,17 @@ var _ = ginkgo.Describe("Workload validating webhook", func() {
 	ginkgo.Context("When creating a Workload", func() {
 		ginkgo.It("Should validate Workload", func() {
 			ginkgo.By("Creating a new Workload")
-			workload := testing.MakeWorkload(workloadName, ns.Name).PriorityClass("invalid_class").Obj()
-			workload.Spec.PodSets[0].Count = -1
+			workload := testing.MakeWorkload(workloadName, ns.Name).
+				PodSetCount(-1).
+				PriorityClass("invalid_class").
+				Obj()
 			err := k8sClient.Create(ctx, workload)
 			gomega.Expect(err).Should(gomega.HaveOccurred())
 			status := err.(*errors.StatusError).Status()
-			gomega.Expect(status.Reason).Should(gomega.BeEquivalentTo(
-				"[spec.podSets[0].count: Invalid value: -1: count must be greater than 0, " +
-					"spec.priorityClassName: Invalid value: \"invalid_class\": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')]",
-			))
+			gomega.Expect(status).Should(testing.Equal(metav1.Status{
+				Status: metav1.StatusFailure,
+				Code:   http.StatusForbidden,
+			}, cmpopts.IgnoreFields(metav1.Status{}, "TypeMeta", "ListMeta", "Message", "Reason", "Details")))
 		})
 	})
 
@@ -88,7 +93,10 @@ var _ = ginkgo.Describe("Workload validating webhook", func() {
 			err := k8sClient.Update(ctx, workload)
 			gomega.Expect(err).Should(gomega.HaveOccurred())
 			status := err.(*errors.StatusError).Status()
-			gomega.Expect(status.Reason).Should(gomega.BeEquivalentTo("spec.podSets[0].count: Invalid value: -1: count must be greater than 0"))
+			gomega.Expect(status).Should(testing.Equal(metav1.Status{
+				Status: metav1.StatusFailure,
+				Code:   http.StatusForbidden,
+			}, cmpopts.IgnoreFields(metav1.Status{}, "TypeMeta", "ListMeta", "Message", "Reason", "Details")))
 		})
 	})
 })