Merge pull request #63666 from xchapter7x/pkg-scheduler-factory

Automatic merge from submit-queue (batch tested with PRs 58487, 63666). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

use subtest for table units (pkg/scheduler/factory)

**What this PR does / why we need it**: Update scheduler's unit table tests to use subtest

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:

**Special notes for your reviewer**:
breaks up PR: https://github.com/kubernetes/kubernetes/pull/63281
/ref #63267

**Release note**:

```release-note
This PR will leverage subtests on the existing table tests for the scheduler units.
Some refactoring of error/status messages and functions to align with new approach.

```

pkg/scheduler/factory/factory_test.go

@@ -331,53 +331,65 @@ func TestNodeEnumerator(t *testing.T) {
 		t.Fatalf("expected %v, got %v", e, a)
 	}
 	for i := range testList.Items {
-		gotObj := me.Get(i)
-		if e, a := testList.Items[i].Name, gotObj.(*v1.Node).Name; e != a {
-			t.Errorf("Expected %v, got %v", e, a)
-		}
-		if e, a := &testList.Items[i], gotObj; !reflect.DeepEqual(e, a) {
-			t.Errorf("Expected %#v, got %v#", e, a)
-		}
+		t.Run(fmt.Sprintf("node enumerator/%v", i), func(t *testing.T) {
+			gotObj := me.Get(i)
+			if e, a := testList.Items[i].Name, gotObj.(*v1.Node).Name; e != a {
+				t.Errorf("Expected %v, got %v", e, a)
+			}
+			if e, a := &testList.Items[i], gotObj; !reflect.DeepEqual(e, a) {
+				t.Errorf("Expected %#v, got %v#", e, a)
+			}
+		})
 	}
 }
 
 func TestBind(t *testing.T) {
 	table := []struct {
+		name    string
 		binding *v1.Binding
 	}{
-		{binding: &v1.Binding{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: metav1.NamespaceDefault,
-				Name:      "foo",
+		{
+			name: "binding can bind and validate request",
+			binding: &v1.Binding{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: metav1.NamespaceDefault,
+					Name:      "foo",
+				},
+				Target: v1.ObjectReference{
+					Name: "foohost.kubernetes.mydomain.com",
+				},
 			},
-			Target: v1.ObjectReference{
-				Name: "foohost.kubernetes.mydomain.com",
-			},
-		}},
+		},
 	}
 
-	for _, item := range table {
-		handler := utiltesting.FakeHandler{
-			StatusCode:   200,
-			ResponseBody: "",
-			T:            t,
-		}
-		server := httptest.NewServer(&handler)
-		defer server.Close()
-		client := clientset.NewForConfigOrDie(&restclient.Config{Host: server.URL, ContentConfig: restclient.ContentConfig{GroupVersion: &schema.GroupVersion{Group: "", Version: "v1"}}})
-		b := binder{client}
-
-		if err := b.Bind(item.binding); err != nil {
-			t.Errorf("Unexpected error: %v", err)
-			continue
-		}
-		expectedBody := runtime.EncodeOrDie(schedulertesting.Test.Codec(), item.binding)
-		handler.ValidateRequest(t,
-			schedulertesting.Test.SubResourcePath(string(v1.ResourcePods), metav1.NamespaceDefault, "foo", "binding"),
-			"POST", &expectedBody)
+	for _, test := range table {
+		t.Run(test.name, func(t *testing.T) {
+			testBind(test.binding, t)
+		})
 	}
 }
 
+func testBind(binding *v1.Binding, t *testing.T) {
+	handler := utiltesting.FakeHandler{
+		StatusCode:   200,
+		ResponseBody: "",
+		T:            t,
+	}
+	server := httptest.NewServer(&handler)
+	defer server.Close()
+	client := clientset.NewForConfigOrDie(&restclient.Config{Host: server.URL, ContentConfig: restclient.ContentConfig{GroupVersion: &schema.GroupVersion{Group: "", Version: "v1"}}})
+	b := binder{client}
+
+	if err := b.Bind(binding); err != nil {
+		t.Errorf("Unexpected error: %v", err)
+		return
+	}
+	expectedBody := runtime.EncodeOrDie(schedulertesting.Test.Codec(), binding)
+	handler.ValidateRequest(t,
+		schedulertesting.Test.SubResourcePath(string(v1.ResourcePods), metav1.NamespaceDefault, "foo", "binding"),
+		"POST", &expectedBody)
+}
+
 func TestInvalidHardPodAffinitySymmetricWeight(t *testing.T) {
 	handler := utiltesting.FakeHandler{
 		StatusCode:   500,
@@ -406,38 +418,44 @@ func TestInvalidFactoryArgs(t *testing.T) {
 	client := clientset.NewForConfigOrDie(&restclient.Config{Host: server.URL, ContentConfig: restclient.ContentConfig{GroupVersion: &schema.GroupVersion{Group: "", Version: "v1"}}})
 
 	testCases := []struct {
+		name                           string
 		hardPodAffinitySymmetricWeight int32
 		expectErr                      string
 	}{
 		{
+			name: "symmetric weight below range",
 			hardPodAffinitySymmetricWeight: -1,
 			expectErr:                      "invalid hardPodAffinitySymmetricWeight: -1, must be in the range 0-100",
 		},
 		{
+			name: "symmetric weight above range",
 			hardPodAffinitySymmetricWeight: 101,
 			expectErr:                      "invalid hardPodAffinitySymmetricWeight: 101, must be in the range 0-100",
 		},
 	}
 
 	for _, test := range testCases {
-		factory := newConfigFactory(client, test.hardPodAffinitySymmetricWeight)
-		_, err := factory.Create()
-		if err == nil {
-			t.Errorf("expected err: %s, got nothing", test.expectErr)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			factory := newConfigFactory(client, test.hardPodAffinitySymmetricWeight)
+			_, err := factory.Create()
+			if err == nil {
+				t.Errorf("expected err: %s, got nothing", test.expectErr)
+			}
+		})
 	}
 
 }
 
 func TestSkipPodUpdate(t *testing.T) {
-	for _, test := range []struct {
+	table := []struct {
 		pod              *v1.Pod
 		isAssumedPodFunc func(*v1.Pod) bool
 		getPodFunc       func(*v1.Pod) *v1.Pod
 		expected         bool
+		name             string
 	}{
-		// Non-assumed pod should not be skipped.
 		{
+			name: "Non-assumed pod",
 			pod: &v1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "pod-0",
@@ -453,9 +471,8 @@ func TestSkipPodUpdate(t *testing.T) {
 			},
 			expected: false,
 		},
-		// Pod update (with changes on ResourceVersion, Spec.NodeName and/or
-		// Annotations) for an already assumed pod should be skipped.
 		{
+			name: "with changes on ResourceVersion, Spec.NodeName and/or Annotations",
 			pod: &v1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:            "pod-0",
@@ -483,9 +500,8 @@ func TestSkipPodUpdate(t *testing.T) {
 			},
 			expected: true,
 		},
-		// Pod update (with changes on Labels) for an already assumed pod
-		// should not be skipped.
 		{
+			name: "with changes on Labels",
 			pod: &v1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:   "pod-0",
@@ -505,17 +521,20 @@ func TestSkipPodUpdate(t *testing.T) {
 			},
 			expected: false,
 		},
-	} {
-		c := &configFactory{
-			schedulerCache: &schedulertesting.FakeCache{
-				IsAssumedPodFunc: test.isAssumedPodFunc,
-				GetPodFunc:       test.getPodFunc,
-			},
-		}
-		got := c.skipPodUpdate(test.pod)
-		if got != test.expected {
-			t.Errorf("skipPodUpdate() = %t, expected = %t", got, test.expected)
-		}
+	}
+	for _, test := range table {
+		t.Run(test.name, func(t *testing.T) {
+			c := &configFactory{
+				schedulerCache: &schedulertesting.FakeCache{
+					IsAssumedPodFunc: test.isAssumedPodFunc,
+					GetPodFunc:       test.getPodFunc,
+				},
+			}
+			got := c.skipPodUpdate(test.pod)
+			if got != test.expected {
+				t.Errorf("skipPodUpdate() = %t, expected = %t", got, test.expected)
+			}
+		})
 	}
 }
 
@@ -593,24 +612,22 @@ func (f *fakeExtender) IsInterested(pod *v1.Pod) bool {
 }
 
 func TestGetBinderFunc(t *testing.T) {
-	for _, test := range []struct {
-		podName   string
-		extenders []algorithm.SchedulerExtender
-
+	table := []struct {
+		podName            string
+		extenders          []algorithm.SchedulerExtender
 		expectedBinderType string
+		name               string
 	}{
-		// Expect to return the default binder because the extender is not a
-		// binder, even though it's interested in the pod.
 		{
+			name:    "the extender is not a binder",
 			podName: "pod0",
 			extenders: []algorithm.SchedulerExtender{
 				&fakeExtender{isBinder: false, interestedPodName: "pod0"},
 			},
 			expectedBinderType: "*factory.binder",
 		},
-		// Expect to return the fake binder because one of the extenders is a
-		// binder and it's interested in the pod.
 		{
+			name:    "one of the extenders is a binder and interested in pod",
 			podName: "pod0",
 			extenders: []algorithm.SchedulerExtender{
 				&fakeExtender{isBinder: false, interestedPodName: "pod0"},
@@ -618,9 +635,8 @@ func TestGetBinderFunc(t *testing.T) {
 			},
 			expectedBinderType: "*factory.fakeExtender",
 		},
-		// Expect to return the default binder because one of the extenders is
-		// a binder but the binder is not interested in the pod.
 		{
+			name:    "one of the extenders is a binder, but not interested in pod",
 			podName: "pod1",
 			extenders: []algorithm.SchedulerExtender{
 				&fakeExtender{isBinder: false, interestedPodName: "pod1"},
@@ -628,20 +644,28 @@ func TestGetBinderFunc(t *testing.T) {
 			},
 			expectedBinderType: "*factory.binder",
 		},
-	} {
-		pod := &v1.Pod{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: test.podName,
-			},
-		}
+	}
 
-		f := &configFactory{}
-		binderFunc := f.getBinderFunc(test.extenders)
-		binder := binderFunc(pod)
-
-		binderType := fmt.Sprintf("%s", reflect.TypeOf(binder))
-		if binderType != test.expectedBinderType {
-			t.Errorf("Expected binder %q but got %q", test.expectedBinderType, binderType)
-		}
+	for _, test := range table {
+		t.Run(test.name, func(t *testing.T) {
+			testGetBinderFunc(test.expectedBinderType, test.podName, test.extenders, t)
+		})
+	}
+}
+
+func testGetBinderFunc(expectedBinderType, podName string, extenders []algorithm.SchedulerExtender, t *testing.T) {
+	pod := &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: podName,
+		},
+	}
+
+	f := &configFactory{}
+	binderFunc := f.getBinderFunc(extenders)
+	binder := binderFunc(pod)
+
+	binderType := fmt.Sprintf("%s", reflect.TypeOf(binder))
+	if binderType != expectedBinderType {
+		t.Errorf("Expected binder %q but got %q", expectedBinderType, binderType)
 	}
 }