Address review comments

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>

cmd/kueue/main.go

@@ -260,7 +260,7 @@ func setupControllers(mgr ctrl.Manager, cCache *cache.Cache, queues *queue.Manag
 
 	opts := []jobframework.Option{
 		jobframework.WithManageJobsWithoutQueueName(cfg.ManageJobsWithoutQueueName),
-		jobframework.WithWaitForPodsReady(config.IsWaitForPodsReadyEnable(cfg)),
+		jobframework.WithWaitForPodsReady(config.IsWaitForPodsReadyEnabled(cfg)),
 		jobframework.WithKubeServerVersion(serverVersionFetcher),
 		jobframework.WithEnabledFrameworks(config.EnabledFrameworks(cfg)),
 		jobframework.WithManagerName(constants.KueueName),

pkg/config/config.go

@@ -177,6 +177,6 @@ func EnabledFrameworks(cfg *configapi.Configuration) sets.Set[string] {
 	return sets.New(cfg.Integrations.Frameworks...)
 }
 
-func IsWaitForPodsReadyEnable(cfg *configapi.Configuration) bool {
+func IsWaitForPodsReadyEnabled(cfg *configapi.Configuration) bool {
 	return cfg.WaitForPodsReady != nil && cfg.WaitForPodsReady.Enable
 }

pkg/controller/jobframework/jobframework.go

@@ -1,3 +1,19 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
 package jobframework
 
 import (

pkg/controller/jobframework/jobframework_test.go

@@ -1,3 +1,19 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
 package jobframework
 
 import (
@@ -18,6 +34,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/rest"
+	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	ctrlmgr "sigs.k8s.io/controller-runtime/pkg/manager"
 	jobset "sigs.k8s.io/jobset/api/jobset/v1alpha2"
@@ -59,6 +76,14 @@ func TestSetupControllers(t *testing.T) {
 			},
 			wantError: errModifyOpts,
 		},
+		"mapper doesn't have kubeflow.org/mpijob, but no error occur": {
+			opts: []Option{
+				WithEnabledFrameworks(sets.New("batch/job", "kubeflow.org/mpijob")),
+			},
+			mapperGVKs: []schema.GroupVersionKind{
+				batchv1.SchemeGroupVersion.WithKind("Job"),
+			},
+		},
 	}
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
@@ -102,33 +127,33 @@ func TestSetupIndexes(t *testing.T) {
 
 	cases := map[string]struct {
 		opts                  []Option
-		wls                   []client.Object
+		workloads             []kueue.Workload
 		filter                client.ListOption
 		wantError             error
 		wantFieldMatcherError bool
-		wantList              []string
+		wantWorkloads         []string
 	}{
 		"proper indexes are set": {
-			wls: []client.Object{
-				utiltesting.MakeWorkload("alpha-wl", testNamespace).
+			workloads: []kueue.Workload{
+				*utiltesting.MakeWorkload("alpha-wl", testNamespace).
 					OwnerReference(batchv1.SchemeGroupVersion.WithKind("Job"), "alpha", "job", true, true).
 					Obj(),
-				utiltesting.MakeWorkload("beta-wl", testNamespace).
+				*utiltesting.MakeWorkload("beta-wl", testNamespace).
 					OwnerReference(batchv1.SchemeGroupVersion.WithKind("Job"), "beta", "job", true, true).
 					Obj(),
 			},
 			opts: []Option{
 				WithEnabledFrameworks(sets.New("batch/job")),
 			},
-			filter:   client.MatchingFields{GetOwnerKey(batchv1.SchemeGroupVersion.WithKind("Job")): "alpha"},
-			wantList: []string{"alpha-wl"},
+			filter:        client.MatchingFields{GetOwnerKey(batchv1.SchemeGroupVersion.WithKind("Job")): "alpha"},
+			wantWorkloads: []string{"alpha-wl"},
 		},
 		"kubeflow.org/mpijob is disabled in the configAPI": {
-			wls: []client.Object{
-				utiltesting.MakeWorkload("alpha-wl", testNamespace).
+			workloads: []kueue.Workload{
+				*utiltesting.MakeWorkload("alpha-wl", testNamespace).
 					OwnerReference(kubeflow.SchemeGroupVersionKind, "alpha", "mpijob", true, true).
 					Obj(),
-				utiltesting.MakeWorkload("beta-wl", testNamespace).
+				*utiltesting.MakeWorkload("beta-wl", testNamespace).
 					OwnerReference(kubeflow.SchemeGroupVersionKind, "beta", "mpijob", true, true).
 					Obj(),
 			},
@@ -142,30 +167,39 @@ func TestSetupIndexes(t *testing.T) {
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
 			ctx := context.Background()
-			builder := utiltesting.NewClientBuilder()
-			builder.WithObjects(append(tc.wls, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNamespace}})...)
-
+			builder := utiltesting.NewClientBuilder().WithObjects(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNamespace}})
 			gotIndexerErr := SetupIndexes(ctx, utiltesting.AsIndexer(builder), tc.opts...)
 			if diff := cmp.Diff(tc.wantError, gotIndexerErr, cmpopts.EquateErrors()); len(diff) != 0 {
 				t.Fatalf("Unexpected setupIndexer error (-want,+got):\n%s", diff)
 			}
-
 			k8sClient := builder.Build()
+			for _, wl := range tc.workloads {
+				if err := k8sClient.Create(ctx, &wl); err != nil {
+					t.Fatalf("Unable to create workload, %q: %v", klog.KObj(&wl), err)
+				}
+			}
 
+			// In any case, a list operation without fieldMatcher should succeed.
 			gotWls := &kueue.WorkloadList{}
-			if tc.wantFieldMatcherError {
-				// Given that the `wantFieldMatcherError` is `true`, a list operation without fieldMatcher should succeed.
-				if gotListErr := k8sClient.List(ctx, gotWls, client.InNamespace(testNamespace)); gotListErr != nil {
-					t.Fatalf("Failed to list workloads without a fieldMatcher: %v", gotListErr)
-				}
+			if gotListErr := k8sClient.List(ctx, gotWls, client.InNamespace(testNamespace)); gotListErr != nil {
+				t.Fatalf("Failed to list workloads without a fieldMatcher: %v", gotListErr)
+			}
+			deployedWlNames := slices.Map(tc.workloads, func(j *kueue.Workload) string { return j.Name })
+			gotWlNames := slices.Map(gotWls.Items, func(j *kueue.Workload) string { return j.Name })
+			if diff := cmp.Diff(deployedWlNames, gotWlNames, cmpopts.EquateEmpty(),
+				cmpopts.SortSlices(func(a, b string) bool { return a < b })); len(diff) != 0 {
+				t.Errorf("Unexpected list workloads (-want,+got):\n%s", diff)
 			}
+
+			// List workloads with fieldMatcher.
 			gotListErr := k8sClient.List(ctx, gotWls, client.InNamespace(testNamespace), tc.filter)
 			if (gotListErr != nil) != tc.wantFieldMatcherError {
 				t.Errorf("Unexpected list error\nwant: %v\ngot: %v", tc.wantFieldMatcherError, gotListErr)
 			}
+
 			if !tc.wantFieldMatcherError {
-				gotWlNames := slices.Map(gotWls.Items, func(j *kueue.Workload) string { return j.Name })
-				if diff := cmp.Diff(tc.wantList, gotWlNames, cmpopts.EquateEmpty(),
+				gotWlNames = slices.Map(gotWls.Items, func(j *kueue.Workload) string { return j.Name })
+				if diff := cmp.Diff(tc.wantWorkloads, gotWlNames, cmpopts.EquateEmpty(),
 					cmpopts.SortSlices(func(a, b string) bool { return a < b })); len(diff) != 0 {
 					t.Errorf("Unexpected list workloads (-want,+got):\n%s", diff)
 				}