Merge pull request #56740 from dhilipkumars/UTImprove

Automatic merge from submit-queue. 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>.

[Deployments] Move some tests to use go sub-test

**What this PR does / why we need it**:

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

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```

pkg/controller/deployment/progress_test.go

@@ -163,13 +163,15 @@ func TestRequeueStuckDeployment(t *testing.T) {
 	dc.enqueueDeployment = dc.enqueue
 
 	for _, test := range tests {
-		if test.nowFn != nil {
-			nowFn = test.nowFn
-		}
-		got := dc.requeueStuckDeployment(test.d, test.status)
-		if got != test.expected {
-			t.Errorf("%s: got duration: %v, expected duration: %v", test.name, got, test.expected)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			if test.nowFn != nil {
+				nowFn = test.nowFn
+			}
+			got := dc.requeueStuckDeployment(test.d, test.status)
+			if got != test.expected {
+				t.Errorf("%s: got duration: %v, expected duration: %v", test.name, got, test.expected)
+			}
+		})
 	}
 }
 
@@ -310,32 +312,34 @@ func TestSyncRolloutStatus(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		fake := fake.Clientset{}
-		dc := &DeploymentController{
-			client: &fake,
-		}
-
-		if test.newRS != nil {
-			test.allRSs = append(test.allRSs, test.newRS)
-		}
-
-		err := dc.syncRolloutStatus(test.allRSs, test.newRS, test.d)
-		if err != nil {
-			t.Error(err)
-		}
-
-		newCond := util.GetDeploymentCondition(test.d.Status, test.conditionType)
-		switch {
-		case newCond == nil:
-			if test.d.Spec.ProgressDeadlineSeconds != nil {
-				t.Errorf("%s: expected deployment condition: %s", test.name, test.conditionType)
+		t.Run(test.name, func(t *testing.T) {
+			fake := fake.Clientset{}
+			dc := &DeploymentController{
+				client: &fake,
 			}
-		case newCond.Status != test.conditionStatus || newCond.Reason != test.conditionReason:
-			t.Errorf("%s: DeploymentProgressing has status %s with reason %s. Expected %s with %s.", test.name, newCond.Status, newCond.Reason, test.conditionStatus, test.conditionReason)
-		case !test.lastUpdate.IsZero() && test.lastUpdate != testTime:
-			t.Errorf("%s: LastUpdateTime was changed to %s but expected %s;", test.name, test.lastUpdate, testTime)
-		case !test.lastTransition.IsZero() && test.lastTransition != testTime:
-			t.Errorf("%s: LastTransitionTime was changed to %s but expected %s;", test.name, test.lastTransition, testTime)
-		}
+
+			if test.newRS != nil {
+				test.allRSs = append(test.allRSs, test.newRS)
+			}
+
+			err := dc.syncRolloutStatus(test.allRSs, test.newRS, test.d)
+			if err != nil {
+				t.Error(err)
+			}
+
+			newCond := util.GetDeploymentCondition(test.d.Status, test.conditionType)
+			switch {
+			case newCond == nil:
+				if test.d.Spec.ProgressDeadlineSeconds != nil {
+					t.Errorf("%s: expected deployment condition: %s", test.name, test.conditionType)
+				}
+			case newCond.Status != test.conditionStatus || newCond.Reason != test.conditionReason:
+				t.Errorf("%s: DeploymentProgressing has status %s with reason %s. Expected %s with %s.", test.name, newCond.Status, newCond.Reason, test.conditionStatus, test.conditionReason)
+			case !test.lastUpdate.IsZero() && test.lastUpdate != testTime:
+				t.Errorf("%s: LastUpdateTime was changed to %s but expected %s;", test.name, test.lastUpdate, testTime)
+			case !test.lastTransition.IsZero() && test.lastTransition != testTime:
+				t.Errorf("%s: LastTransitionTime was changed to %s but expected %s;", test.name, test.lastTransition, testTime)
+			}
+		})
 	}
 }

pkg/controller/deployment/recreate_test.go

@@ -115,9 +115,11 @@ func TestOldPodsRunning(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		if expected, got := test.expected, oldPodsRunning(test.newRS, test.oldRSs, test.podMap); expected != got {
-			t.Errorf("%s: expected %t, got %t", test.name, expected, got)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			if expected, got := test.expected, oldPodsRunning(test.newRS, test.oldRSs, test.podMap); expected != got {
+				t.Errorf("%s: expected %t, got %t", test.name, expected, got)
+			}
+		})
 	}
 }
 

pkg/controller/deployment/sync_test.go

@@ -267,72 +267,74 @@ func TestScale(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		_ = olderTimestamp
-		t.Log(test.name)
-		fake := fake.Clientset{}
-		dc := &DeploymentController{
-			client:        &fake,
-			eventRecorder: &record.FakeRecorder{},
-		}
-
-		if test.newRS != nil {
-			desiredReplicas := *(test.oldDeployment.Spec.Replicas)
-			if desired, ok := test.desiredReplicasAnnotations[test.newRS.Name]; ok {
-				desiredReplicas = desired
+		t.Run(test.name, func(t *testing.T) {
+			_ = olderTimestamp
+			t.Log(test.name)
+			fake := fake.Clientset{}
+			dc := &DeploymentController{
+				client:        &fake,
+				eventRecorder: &record.FakeRecorder{},
 			}
-			deploymentutil.SetReplicasAnnotations(test.newRS, desiredReplicas, desiredReplicas+deploymentutil.MaxSurge(*test.oldDeployment))
-		}
-		for i := range test.oldRSs {
-			rs := test.oldRSs[i]
-			if rs == nil {
-				continue
-			}
-			desiredReplicas := *(test.oldDeployment.Spec.Replicas)
-			if desired, ok := test.desiredReplicasAnnotations[rs.Name]; ok {
-				desiredReplicas = desired
-			}
-			deploymentutil.SetReplicasAnnotations(rs, desiredReplicas, desiredReplicas+deploymentutil.MaxSurge(*test.oldDeployment))
-		}
 
-		if err := dc.scale(test.deployment, test.newRS, test.oldRSs); err != nil {
-			t.Errorf("%s: unexpected error: %v", test.name, err)
-			continue
-		}
+			if test.newRS != nil {
+				desiredReplicas := *(test.oldDeployment.Spec.Replicas)
+				if desired, ok := test.desiredReplicasAnnotations[test.newRS.Name]; ok {
+					desiredReplicas = desired
+				}
+				deploymentutil.SetReplicasAnnotations(test.newRS, desiredReplicas, desiredReplicas+deploymentutil.MaxSurge(*test.oldDeployment))
+			}
+			for i := range test.oldRSs {
+				rs := test.oldRSs[i]
+				if rs == nil {
+					continue
+				}
+				desiredReplicas := *(test.oldDeployment.Spec.Replicas)
+				if desired, ok := test.desiredReplicasAnnotations[rs.Name]; ok {
+					desiredReplicas = desired
+				}
+				deploymentutil.SetReplicasAnnotations(rs, desiredReplicas, desiredReplicas+deploymentutil.MaxSurge(*test.oldDeployment))
+			}
 
-		// Construct the nameToSize map that will hold all the sizes we got our of tests
-		// Skip updating the map if the replica set wasn't updated since there will be
-		// no update action for it.
-		nameToSize := make(map[string]int32)
-		if test.newRS != nil {
-			nameToSize[test.newRS.Name] = *(test.newRS.Spec.Replicas)
-		}
-		for i := range test.oldRSs {
-			rs := test.oldRSs[i]
-			nameToSize[rs.Name] = *(rs.Spec.Replicas)
-		}
-		// Get all the UPDATE actions and update nameToSize with all the updated sizes.
-		for _, action := range fake.Actions() {
-			rs := action.(testclient.UpdateAction).GetObject().(*extensions.ReplicaSet)
-			if !test.wasntUpdated[rs.Name] {
+			if err := dc.scale(test.deployment, test.newRS, test.oldRSs); err != nil {
+				t.Errorf("%s: unexpected error: %v", test.name, err)
+				return
+			}
+
+			// Construct the nameToSize map that will hold all the sizes we got our of tests
+			// Skip updating the map if the replica set wasn't updated since there will be
+			// no update action for it.
+			nameToSize := make(map[string]int32)
+			if test.newRS != nil {
+				nameToSize[test.newRS.Name] = *(test.newRS.Spec.Replicas)
+			}
+			for i := range test.oldRSs {
+				rs := test.oldRSs[i]
 				nameToSize[rs.Name] = *(rs.Spec.Replicas)
 			}
-		}
-
-		if test.expectedNew != nil && test.newRS != nil && *(test.expectedNew.Spec.Replicas) != nameToSize[test.newRS.Name] {
-			t.Errorf("%s: expected new replicas: %d, got: %d", test.name, *(test.expectedNew.Spec.Replicas), nameToSize[test.newRS.Name])
-			continue
-		}
-		if len(test.expectedOld) != len(test.oldRSs) {
-			t.Errorf("%s: expected %d old replica sets, got %d", test.name, len(test.expectedOld), len(test.oldRSs))
-			continue
-		}
-		for n := range test.oldRSs {
-			rs := test.oldRSs[n]
-			expected := test.expectedOld[n]
-			if *(expected.Spec.Replicas) != nameToSize[rs.Name] {
-				t.Errorf("%s: expected old (%s) replicas: %d, got: %d", test.name, rs.Name, *(expected.Spec.Replicas), nameToSize[rs.Name])
+			// Get all the UPDATE actions and update nameToSize with all the updated sizes.
+			for _, action := range fake.Actions() {
+				rs := action.(testclient.UpdateAction).GetObject().(*extensions.ReplicaSet)
+				if !test.wasntUpdated[rs.Name] {
+					nameToSize[rs.Name] = *(rs.Spec.Replicas)
+				}
 			}
-		}
+
+			if test.expectedNew != nil && test.newRS != nil && *(test.expectedNew.Spec.Replicas) != nameToSize[test.newRS.Name] {
+				t.Errorf("%s: expected new replicas: %d, got: %d", test.name, *(test.expectedNew.Spec.Replicas), nameToSize[test.newRS.Name])
+				return
+			}
+			if len(test.expectedOld) != len(test.oldRSs) {
+				t.Errorf("%s: expected %d old replica sets, got %d", test.name, len(test.expectedOld), len(test.oldRSs))
+				return
+			}
+			for n := range test.oldRSs {
+				rs := test.oldRSs[n]
+				expected := test.expectedOld[n]
+				if *(expected.Spec.Replicas) != nameToSize[rs.Name] {
+					t.Errorf("%s: expected old (%s) replicas: %d, got: %d", test.name, rs.Name, *(expected.Spec.Replicas), nameToSize[rs.Name])
+				}
+			}
+		})
 	}
 }