From b646a72938084cf6a1435dd47d42489d81465743 Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Thu, 7 Sep 2017 13:09:05 -0700 Subject: [PATCH] Address comments --- .../integration/deployment/deployment_test.go | 86 +++++++++++++------ test/integration/deployment/util.go | 45 ++++++---- 2 files changed, 90 insertions(+), 41 deletions(-) diff --git a/test/integration/deployment/deployment_test.go b/test/integration/deployment/deployment_test.go index 0c59309e78c..1aa271df87a 100644 --- a/test/integration/deployment/deployment_test.go +++ b/test/integration/deployment/deployment_test.go @@ -54,13 +54,20 @@ func TestNewDeployment(t *testing.T) { go dc.Run(5, stopCh) // Wait for the Deployment to be updated to revision 1 - tester.waitForDeploymentRevisionAndImage("1", fakeImage) + if err := tester.waitForDeploymentRevisionAndImage("1", fakeImage); err != nil { + t.Fatal(err) + } // Make sure the Deployment status becomes valid while manually marking Deployment pods as ready at the same time - tester.waitForDeploymentStatusValidAndMarkPodsReady() + if err := tester.waitForDeploymentStatusValidAndMarkPodsReady(); err != nil { + t.Fatal(err) + } // Check new RS annotations - newRS := tester.expectNewReplicaSet() + newRS, err := tester.expectNewReplicaSet() + if err != nil { + t.Fatal(err) + } if newRS.Annotations["test"] != "should-copy-to-replica-set" { t.Errorf("expected new ReplicaSet annotations copied from Deployment %s, got: %v", tester.deployment.Name, newRS.Annotations) } @@ -159,35 +166,47 @@ func TestPausedDeployment(t *testing.T) { go dc.Run(5, stopCh) // Verify that the paused deployment won't create new replica set. - tester.expectNoNewReplicaSet() + if err := tester.expectNoNewReplicaSet(); err != nil { + t.Fatal(err) + } // Resume the deployment tester.deployment, err = tester.updateDeployment(resumeFn()) if err != nil { - t.Errorf("failed to resume deployment %s: %v", tester.deployment.Name, err) + t.Fatalf("failed to resume deployment %s: %v", tester.deployment.Name, err) } // Wait for the controller to notice the resume. - tester.waitForObservedDeployment(tester.deployment.Generation) + if err := tester.waitForObservedDeployment(tester.deployment.Generation); err != nil { + t.Fatal(err) + } // Wait for the Deployment to be updated to revision 1 - tester.waitForDeploymentRevisionAndImage("1", fakeImage) + if err := tester.waitForDeploymentRevisionAndImage("1", fakeImage); err != nil { + t.Fatal(err) + } // Make sure the Deployment status becomes valid while manually marking Deployment pods as ready at the same time - tester.waitForDeploymentStatusValidAndMarkPodsReady() + if err := tester.waitForDeploymentStatusValidAndMarkPodsReady(); err != nil { + t.Fatal(err) + } // A new replicaset should be created. - tester.expectNewReplicaSet() + if _, err := tester.expectNewReplicaSet(); err != nil { + t.Fatal(err) + } // Pause the deployment. // The paused deployment shouldn't trigger a new rollout. tester.deployment, err = tester.updateDeployment(pauseFn()) if err != nil { - t.Errorf("failed to pause deployment %s: %v", tester.deployment.Name, err) + t.Fatalf("failed to pause deployment %s: %v", tester.deployment.Name, err) } // Wait for the controller to notice the pause. - tester.waitForObservedDeployment(tester.deployment.Generation) + if err := tester.waitForObservedDeployment(tester.deployment.Generation); err != nil { + t.Fatal(err) + } // Update the deployment template newTGPS := int64(0) @@ -195,18 +214,22 @@ func TestPausedDeployment(t *testing.T) { update.Spec.Template.Spec.TerminationGracePeriodSeconds = &newTGPS }) if err != nil { - t.Errorf("failed updating deployment %s: %v", tester.deployment.Name, err) + t.Fatalf("failed updating template of deployment %s: %v", tester.deployment.Name, err) } // Wait for the controller to notice the rollout. - tester.waitForObservedDeployment(tester.deployment.Generation) + if err := tester.waitForObservedDeployment(tester.deployment.Generation); err != nil { + t.Fatal(err) + } // Verify that the paused deployment won't create new replica set. - tester.expectNoNewReplicaSet() + if err := tester.expectNoNewReplicaSet(); err != nil { + t.Fatal(err) + } _, allOldRs, err := deploymentutil.GetOldReplicaSets(tester.deployment, c.ExtensionsV1beta1()) if err != nil { - t.Errorf("failed retrieving old replicasets of deployment %s: %v", tester.deployment.Name, err) + t.Fatalf("failed retrieving old replicasets of deployment %s: %v", tester.deployment.Name, err) } if len(allOldRs) != 1 { t.Errorf("expected an old replica set, got %v", allOldRs) @@ -243,22 +266,30 @@ func TestScalePausedDeployment(t *testing.T) { go dc.Run(5, stopCh) // Wait for the Deployment to be updated to revision 1 - tester.waitForDeploymentRevisionAndImage("1", fakeImage) + if err := tester.waitForDeploymentRevisionAndImage("1", fakeImage); err != nil { + t.Fatal(err) + } // Make sure the Deployment status becomes valid while manually marking Deployment pods as ready at the same time - tester.waitForDeploymentStatusValidAndMarkPodsReady() + if err := tester.waitForDeploymentStatusValidAndMarkPodsReady(); err != nil { + t.Fatal(err) + } // A new replicaset should be created. - tester.expectNewReplicaSet() + if _, err := tester.expectNewReplicaSet(); err != nil { + t.Fatal(err) + } // Pause the deployment. tester.deployment, err = tester.updateDeployment(pauseFn()) if err != nil { - t.Errorf("failed to pause deployment %s: %v", tester.deployment.Name, err) + t.Fatalf("failed to pause deployment %s: %v", tester.deployment.Name, err) } // Wait for the controller to notice the scale. - tester.waitForObservedDeployment(tester.deployment.Generation) + if err := tester.waitForObservedDeployment(tester.deployment.Generation); err != nil { + t.Fatal(err) + } // Scale the paused deployment. newReplicas := int32(10) @@ -266,18 +297,25 @@ func TestScalePausedDeployment(t *testing.T) { update.Spec.Replicas = &newReplicas }) if err != nil { - t.Errorf("failed updating deployment %s: %v", tester.deployment.Name, err) + t.Fatalf("failed updating deployment %s: %v", tester.deployment.Name, err) } // Wait for the controller to notice the scale. - tester.waitForObservedDeployment(tester.deployment.Generation) + if err := tester.waitForObservedDeployment(tester.deployment.Generation); err != nil { + t.Fatal(err) + } // Verify that the new replicaset is scaled. - rs := tester.expectNewReplicaSet() + rs, err := tester.expectNewReplicaSet() + if err != nil { + t.Fatal(err) + } if *rs.Spec.Replicas != newReplicas { t.Errorf("expected new replicaset replicas = %d, got %d", newReplicas, *rs.Spec.Replicas) } // Make sure the Deployment status becomes valid while manually marking Deployment pods as ready at the same time - tester.waitForDeploymentStatusValidAndMarkPodsReady() + if err := tester.waitForDeploymentStatusValidAndMarkPodsReady(); err != nil { + t.Fatal(err) + } } diff --git a/test/integration/deployment/util.go b/test/integration/deployment/util.go index bd854751e68..6201c1d3038 100644 --- a/test/integration/deployment/util.go +++ b/test/integration/deployment/util.go @@ -17,6 +17,7 @@ limitations under the License. package deployment import ( + "fmt" "net/http/httptest" "testing" "time" @@ -144,10 +145,11 @@ func addPodConditionReady(pod *v1.Pod, time metav1.Time) { } } -func (d *deploymentTester) waitForDeploymentRevisionAndImage(revision, image string) { +func (d *deploymentTester) waitForDeploymentRevisionAndImage(revision, image string) error { if err := testutil.WaitForDeploymentRevisionAndImage(d.c, d.deployment.Namespace, d.deployment.Name, revision, image, d.t.Logf, pollInterval, pollTimeout); err != nil { - d.t.Fatalf("failed to wait for Deployment revision %s: %v", d.deployment.Name, err) + return fmt.Errorf("failed to wait for Deployment revision %s: %v", d.deployment.Name, err) } + return nil } // markAllPodsReady manually updates all Deployment pods status to ready @@ -194,48 +196,57 @@ func (d *deploymentTester) waitForDeploymentStatusValid() error { // waitForDeploymentStatusValidAndMarkPodsReady waits for the Deployment status to become valid // while marking all Deployment pods as ready at the same time. -func (d *deploymentTester) waitForDeploymentStatusValidAndMarkPodsReady() { +func (d *deploymentTester) waitForDeploymentStatusValidAndMarkPodsReady() error { // Manually mark all Deployment pods as ready in a separate goroutine go d.markAllPodsReady() // Make sure the Deployment status is valid while Deployment pods are becoming ready err := d.waitForDeploymentStatusValid() if err != nil { - d.t.Fatalf("failed to wait for Deployment status %s: %v", d.deployment.Name, err) + return fmt.Errorf("failed to wait for Deployment status %s: %v", d.deployment.Name, err) } + return nil } func (d *deploymentTester) updateDeployment(applyUpdate testutil.UpdateDeploymentFunc) (*v1beta1.Deployment, error) { return testutil.UpdateDeploymentWithRetries(d.c, d.deployment.Namespace, d.deployment.Name, applyUpdate, d.t.Logf) } -func (d *deploymentTester) waitForObservedDeployment(desiredGeneration int64) { +func (d *deploymentTester) waitForObservedDeployment(desiredGeneration int64) error { if err := testutil.WaitForObservedDeployment(d.c, d.deployment.Namespace, d.deployment.Name, desiredGeneration); err != nil { - d.t.Fatalf("failed waiting for ObservedGeneration of deployment %s to become %d: %v", d.deployment.Name, desiredGeneration, err) + return fmt.Errorf("failed waiting for ObservedGeneration of deployment %s to become %d: %v", d.deployment.Name, desiredGeneration, err) } + return nil } -func (d *deploymentTester) getNewReplicaSet() *v1beta1.ReplicaSet { +func (d *deploymentTester) getNewReplicaSet() (*v1beta1.ReplicaSet, error) { rs, err := deploymentutil.GetNewReplicaSet(d.deployment, d.c.ExtensionsV1beta1()) if err != nil { - d.t.Fatalf("failed retrieving new replicaset of deployment %s: %v", d.deployment.Name, err) + return nil, fmt.Errorf("failed retrieving new replicaset of deployment %s: %v", d.deployment.Name, err) } - return rs + return rs, nil } -func (d *deploymentTester) expectNoNewReplicaSet() { - rs := d.getNewReplicaSet() +func (d *deploymentTester) expectNoNewReplicaSet() error { + rs, err := d.getNewReplicaSet() + if err != nil { + return err + } if rs != nil { - d.t.Fatalf("expected deployment %s not to create a new replicaset, got %v", d.deployment.Name, rs) + return fmt.Errorf("expected deployment %s not to create a new replicaset, got %v", d.deployment.Name, rs) } + return nil } -func (d *deploymentTester) expectNewReplicaSet() *v1beta1.ReplicaSet { - rs := d.getNewReplicaSet() - if rs == nil { - d.t.Fatalf("expected deployment %s to create a new replicaset, got nil", d.deployment.Name) +func (d *deploymentTester) expectNewReplicaSet() (*v1beta1.ReplicaSet, error) { + rs, err := d.getNewReplicaSet() + if err != nil { + return nil, err } - return rs + if rs == nil { + return nil, fmt.Errorf("expected deployment %s to create a new replicaset, got nil", d.deployment.Name) + } + return rs, nil } func pauseFn() func(update *v1beta1.Deployment) {