From 623d7c42ac2b0c24a609c48fa83905f4f36360b6 Mon Sep 17 00:00:00 2001 From: dhilipkumars Date: Sat, 2 Dec 2017 12:47:31 +0530 Subject: [PATCH] Move some tests to use go sub-test --- pkg/controller/deployment/progress_test.go | 70 ++++++------ pkg/controller/deployment/recreate_test.go | 8 +- pkg/controller/deployment/sync_test.go | 122 +++++++++++---------- 3 files changed, 104 insertions(+), 96 deletions(-) diff --git a/pkg/controller/deployment/progress_test.go b/pkg/controller/deployment/progress_test.go index 9677728e7d0..978b21469fb 100644 --- a/pkg/controller/deployment/progress_test.go +++ b/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) + } + }) } } diff --git a/pkg/controller/deployment/recreate_test.go b/pkg/controller/deployment/recreate_test.go index 2cf8661780a..d557b5633ab 100644 --- a/pkg/controller/deployment/recreate_test.go +++ b/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) + } + }) } } diff --git a/pkg/controller/deployment/sync_test.go b/pkg/controller/deployment/sync_test.go index ce74a3eead6..6f5cc96b344 100644 --- a/pkg/controller/deployment/sync_test.go +++ b/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]) + } + } + }) } }