use subtest for table units

pkg/scheduler/factory/cache_comparer_test.go

@@ -27,27 +27,29 @@ import (
 )
 
 func TestCompareNodes(t *testing.T) {
-	compare := compareStrategy{}
-
 	tests := []struct {
+		name      string
 		actual    []string
 		cached    []string
 		missing   []string
 		redundant []string
 	}{
 		{
+			name:      "redundant cached value",
 			actual:    []string{"foo", "bar"},
 			cached:    []string{"bar", "foo", "foobar"},
 			missing:   []string{},
 			redundant: []string{"foobar"},
 		},
 		{
+			name:      "missing cached value",
 			actual:    []string{"foo", "bar", "foobar"},
 			cached:    []string{"bar", "foo"},
 			missing:   []string{"foobar"},
 			redundant: []string{},
 		},
 		{
+			name:      "proper cache set",
 			actual:    []string{"foo", "bar", "foobar"},
 			cached:    []string{"bar", "foobar", "foo"},
 			missing:   []string{},
@@ -56,34 +58,40 @@ func TestCompareNodes(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		nodes := []*v1.Node{}
-		for _, nodeName := range test.actual {
-			node := &v1.Node{}
-			node.Name = nodeName
-			nodes = append(nodes, node)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			testCompareNodes(test.actual, test.cached, test.missing, test.redundant, t)
+		})
+	}
+}
 
-		nodeInfo := make(map[string]*schedulercache.NodeInfo)
-		for _, nodeName := range test.cached {
-			nodeInfo[nodeName] = &schedulercache.NodeInfo{}
-		}
+func testCompareNodes(actual, cached, missing, redundant []string, t *testing.T) {
+	compare := compareStrategy{}
+	nodes := []*v1.Node{}
+	for _, nodeName := range actual {
+		node := &v1.Node{}
+		node.Name = nodeName
+		nodes = append(nodes, node)
+	}
+
+	nodeInfo := make(map[string]*schedulercache.NodeInfo)
+	for _, nodeName := range cached {
+		nodeInfo[nodeName] = &schedulercache.NodeInfo{}
+	}
 
-		m, r := compare.CompareNodes(nodes, nodeInfo)
+	m, r := compare.CompareNodes(nodes, nodeInfo)
 
-		if !reflect.DeepEqual(m, test.missing) {
-			t.Errorf("missing expected to be %s; got %s", test.missing, m)
-		}
+	if !reflect.DeepEqual(m, missing) {
+		t.Errorf("missing expected to be %s; got %s", missing, m)
+	}
 
-		if !reflect.DeepEqual(r, test.redundant) {
-			t.Errorf("redundant expected to be %s; got %s", test.redundant, r)
-		}
+	if !reflect.DeepEqual(r, redundant) {
+		t.Errorf("redundant expected to be %s; got %s", redundant, r)
 	}
 }
 
 func TestComparePods(t *testing.T) {
-	compare := compareStrategy{}
-
 	tests := []struct {
+		name      string
 		actual    []string
 		cached    []string
 		queued    []string
@@ -91,6 +99,7 @@ func TestComparePods(t *testing.T) {
 		redundant []string
 	}{
 		{
+			name:      "redundant cached value",
 			actual:    []string{"foo", "bar"},
 			cached:    []string{"bar", "foo", "foobar"},
 			queued:    []string{},
@@ -98,6 +107,7 @@ func TestComparePods(t *testing.T) {
 			redundant: []string{"foobar"},
 		},
 		{
+			name:      "redundant and queued values",
 			actual:    []string{"foo", "bar"},
 			cached:    []string{"foo", "foobar"},
 			queued:    []string{"bar"},
@@ -105,6 +115,7 @@ func TestComparePods(t *testing.T) {
 			redundant: []string{"foobar"},
 		},
 		{
+			name:      "missing cached value",
 			actual:    []string{"foo", "bar", "foobar"},
 			cached:    []string{"bar", "foo"},
 			queued:    []string{},
@@ -112,6 +123,7 @@ func TestComparePods(t *testing.T) {
 			redundant: []string{},
 		},
 		{
+			name:      "missing and queued values",
 			actual:    []string{"foo", "bar", "foobar"},
 			cached:    []string{"foo"},
 			queued:    []string{"bar"},
@@ -119,6 +131,7 @@ func TestComparePods(t *testing.T) {
 			redundant: []string{},
 		},
 		{
+			name:      "correct cache set",
 			actual:    []string{"foo", "bar", "foobar"},
 			cached:    []string{"bar", "foobar", "foo"},
 			queued:    []string{},
@@ -126,6 +139,7 @@ func TestComparePods(t *testing.T) {
 			redundant: []string{},
 		},
 		{
+			name:      "queued cache value",
 			actual:    []string{"foo", "bar", "foobar"},
 			cached:    []string{"foobar", "foo"},
 			queued:    []string{"bar"},
@@ -135,64 +149,73 @@ func TestComparePods(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		pods := []*v1.Pod{}
-		for _, uid := range test.actual {
-			pod := &v1.Pod{}
-			pod.UID = types.UID(uid)
-			pods = append(pods, pod)
-		}
-
-		queuedPods := []*v1.Pod{}
-		for _, uid := range test.queued {
-			pod := &v1.Pod{}
-			pod.UID = types.UID(uid)
-			queuedPods = append(queuedPods, pod)
-		}
-
-		nodeInfo := make(map[string]*schedulercache.NodeInfo)
-		for _, uid := range test.cached {
-			pod := &v1.Pod{}
-			pod.UID = types.UID(uid)
-			pod.Namespace = "ns"
-			pod.Name = uid
-
-			nodeInfo[uid] = schedulercache.NewNodeInfo(pod)
-		}
-
-		m, r := compare.ComparePods(pods, queuedPods, nodeInfo)
-
-		if !reflect.DeepEqual(m, test.missing) {
-			t.Errorf("missing expected to be %s; got %s", test.missing, m)
-		}
-
-		if !reflect.DeepEqual(r, test.redundant) {
-			t.Errorf("redundant expected to be %s; got %s", test.redundant, r)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			testComparePods(test.actual, test.cached, test.queued, test.missing, test.redundant, t)
+		})
 	}
 }
 
-func TestComparePdbs(t *testing.T) {
+func testComparePods(actual, cached, queued, missing, redundant []string, t *testing.T) {
 	compare := compareStrategy{}
+	pods := []*v1.Pod{}
+	for _, uid := range actual {
+		pod := &v1.Pod{}
+		pod.UID = types.UID(uid)
+		pods = append(pods, pod)
+	}
+
+	queuedPods := []*v1.Pod{}
+	for _, uid := range queued {
+		pod := &v1.Pod{}
+		pod.UID = types.UID(uid)
+		queuedPods = append(queuedPods, pod)
+	}
+
+	nodeInfo := make(map[string]*schedulercache.NodeInfo)
+	for _, uid := range cached {
+		pod := &v1.Pod{}
+		pod.UID = types.UID(uid)
+		pod.Namespace = "ns"
+		pod.Name = uid
+
+		nodeInfo[uid] = schedulercache.NewNodeInfo(pod)
+	}
 
+	m, r := compare.ComparePods(pods, queuedPods, nodeInfo)
+
+	if !reflect.DeepEqual(m, missing) {
+		t.Errorf("missing expected to be %s; got %s", missing, m)
+	}
+
+	if !reflect.DeepEqual(r, redundant) {
+		t.Errorf("redundant expected to be %s; got %s", redundant, r)
+	}
+}
+
+func TestComparePdbs(t *testing.T) {
 	tests := []struct {
+		name      string
 		actual    []string
 		cached    []string
 		missing   []string
 		redundant []string
 	}{
 		{
+			name:      "redundant cache value",
 			actual:    []string{"foo", "bar"},
 			cached:    []string{"bar", "foo", "foobar"},
 			missing:   []string{},
 			redundant: []string{"foobar"},
 		},
 		{
+			name:      "missing cache value",
 			actual:    []string{"foo", "bar", "foobar"},
 			cached:    []string{"bar", "foo"},
 			missing:   []string{"foobar"},
 			redundant: []string{},
 		},
 		{
+			name:      "correct cache",
 			actual:    []string{"foo", "bar", "foobar"},
 			cached:    []string{"bar", "foobar", "foo"},
 			missing:   []string{},
@@ -201,28 +224,35 @@ func TestComparePdbs(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		pdbs := []*policy.PodDisruptionBudget{}
-		for _, uid := range test.actual {
-			pdb := &policy.PodDisruptionBudget{}
-			pdb.UID = types.UID(uid)
-			pdbs = append(pdbs, pdb)
-		}
-
-		cache := make(map[string]*policy.PodDisruptionBudget)
-		for _, uid := range test.cached {
-			pdb := &policy.PodDisruptionBudget{}
-			pdb.UID = types.UID(uid)
-			cache[uid] = pdb
-		}
-
-		m, r := compare.ComparePdbs(pdbs, cache)
-
-		if !reflect.DeepEqual(m, test.missing) {
-			t.Errorf("missing expected to be %s; got %s", test.missing, m)
-		}
-
-		if !reflect.DeepEqual(r, test.redundant) {
-			t.Errorf("redundant expected to be %s; got %s", test.redundant, r)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			testComparePdbs(test.actual, test.cached, test.missing, test.redundant, t)
+		})
+	}
+}
+
+func testComparePdbs(actual, cached, missing, redundant []string, t *testing.T) {
+	compare := compareStrategy{}
+	pdbs := []*policy.PodDisruptionBudget{}
+	for _, uid := range actual {
+		pdb := &policy.PodDisruptionBudget{}
+		pdb.UID = types.UID(uid)
+		pdbs = append(pdbs, pdb)
+	}
+
+	cache := make(map[string]*policy.PodDisruptionBudget)
+	for _, uid := range cached {
+		pdb := &policy.PodDisruptionBudget{}
+		pdb.UID = types.UID(uid)
+		cache[uid] = pdb
+	}
+
+	m, r := compare.ComparePdbs(pdbs, cache)
+
+	if !reflect.DeepEqual(m, missing) {
+		t.Errorf("missing expected to be %s; got %s", missing, m)
+	}
+
+	if !reflect.DeepEqual(r, redundant) {
+		t.Errorf("redundant expected to be %s; got %s", redundant, r)
 	}
 }

pkg/scheduler/factory/factory_test.go

@@ -331,51 +331,63 @@ 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",
-			},
-			Target: v1.ObjectReference{
-				Name: "foohost.kubernetes.mydomain.com",
+		{
+			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",
+				},
 			},
-		}},
+		},
 	}
 
-	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}
+	for _, test := range table {
+		t.Run(test.name, func(t *testing.T) {
+			testBind(test.binding, t)
+		})
+	}
+}
 
-		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)
+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) {
@@ -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,
-			},
-		}
+	}
+
+	for _, test := range table {
+		t.Run(test.name, func(t *testing.T) {
+			testGetBinderFunc(test.expectedBinderType, test.podName, test.extenders, t)
+		})
+	}
+}
 
-		f := &configFactory{}
-		binderFunc := f.getBinderFunc(test.extenders)
-		binder := binderFunc(pod)
+func testGetBinderFunc(expectedBinderType, podName string, extenders []algorithm.SchedulerExtender, t *testing.T) {
+	pod := &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: podName,
+		},
+	}
 
-		binderType := fmt.Sprintf("%s", reflect.TypeOf(binder))
-		if binderType != test.expectedBinderType {
-			t.Errorf("Expected binder %q but got %q", test.expectedBinderType, binderType)
-		}
+	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)
 	}
 }

pkg/scheduler/factory/plugins_test.go

@@ -34,14 +34,18 @@ func TestAlgorithmNameValidation(t *testing.T) {
 		"Some,Alg:orithm",
 	}
 	for _, name := range algorithmNamesShouldValidate {
-		if !validName.MatchString(name) {
-			t.Errorf("%v should be a valid algorithm name but is not valid.", name)
-		}
+		t.Run(name, func(t *testing.T) {
+			if !validName.MatchString(name) {
+				t.Errorf("should be a valid algorithm name but is not valid.")
+			}
+		})
 	}
 	for _, name := range algorithmNamesShouldNotValidate {
-		if validName.MatchString(name) {
-			t.Errorf("%v should be an invalid algorithm name but is valid.", name)
-		}
+		t.Run(name, func(t *testing.T) {
+			if validName.MatchString(name) {
+				t.Errorf("should be an invalid algorithm name but is valid.")
+			}
+		})
 	}
 }
 
@@ -68,15 +72,17 @@ func TestValidatePriorityConfigOverFlow(t *testing.T) {
 		},
 	}
 	for _, test := range tests {
-		err := validateSelectedConfigs(test.configs)
-		if test.expected {
-			if err == nil {
-				t.Errorf("Expected Overflow for %s", test.description)
-			}
-		} else {
-			if err != nil {
-				t.Errorf("Did not expect an overflow for %s", test.description)
+		t.Run(test.description, func(t *testing.T) {
+			err := validateSelectedConfigs(test.configs)
+			if test.expected {
+				if err == nil {
+					t.Errorf("Expected Overflow")
+				}
+			} else {
+				if err != nil {
+					t.Errorf("Did not expect an overflow")
+				}
 			}
-		}
+		})
 	}
 }