Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Changed code to improve output messages on error for files under test/e2e/apps #109944

Merged
merged 6 commits into from Jan 11, 2023
Merged

Changed code to improve output messages on error for files under test/e2e/apps #109944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a8198b1
Improving the output of tests in case of error
rafadc May 8, 2022
870d5d2
Better error message
rafadc May 31, 2022
f603dfa
Fixing 2 tests whose condition was inverted
rafadc Jun 1, 2022
5d0ec1d
Again I got the conditions wrong
rafadc Jun 1, 2022
92b15ca
Sorry for the confusion
rafadc Jun 1, 2022
f6ab8f6
Improved error messages on failures
rafadc Jun 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 25 additions & 8 deletions test/e2e/apps/cronjob.go
View file
Open in desktop
Expand Up @@ -261,7 +261,9 @@ var _ = SIGDescribe("CronJob", func() {
ginkgo.By("Ensuring job was deleted")
_, err = e2ejob.GetJob(f.ClientSet, f.Namespace.Name, job.Name)
framework.ExpectError(err)
framework.ExpectEqual(apierrors.IsNotFound(err), true)
if !apierrors.IsNotFound(err) {
framework.Failf("Failed to delete %s cronjob in namespace %s", cronJob.Name, f.Namespace.Name)
}

ginkgo.By("Ensuring the job is not in the cronjob active list")
err = waitForJobNotActive(f.ClientSet, f.Namespace.Name, cronJob.Name, job.Name)
Expand Down Expand Up @@ -374,10 +376,14 @@ var _ = SIGDescribe("CronJob", func() {
for sawAnnotations := false; !sawAnnotations; {
select {
case evt, ok := <-cjWatch.ResultChan():
framework.ExpectEqual(ok, true, "watch channel should not close")
if !ok {
framework.Fail("watch channel should not close")
}
framework.ExpectEqual(evt.Type, watch.Modified)
watchedCronJob, isCronJob := evt.Object.(*batchv1.CronJob)
framework.ExpectEqual(isCronJob, true, fmt.Sprintf("expected CronJob, got %T", evt.Object))
if !isCronJob {
framework.Failf("expected CronJob, got %T", evt.Object)
}
if watchedCronJob.Annotations["patched"] == "true" {
framework.Logf("saw patched and updated annotations")
sawAnnotations = true
Expand All @@ -403,7 +409,9 @@ var _ = SIGDescribe("CronJob", func() {
[]byte(`{"metadata":{"annotations":{"patchedstatus":"true"}},"status":`+string(cjStatusJSON)+`}`),
metav1.PatchOptions{}, "status")
framework.ExpectNoError(err)
framework.ExpectEqual(patchedStatus.Status.LastScheduleTime.Equal(&now1), true, "patched object should have the applied lastScheduleTime status")
if !patchedStatus.Status.LastScheduleTime.Equal(&now1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I am assuming that the intent here was to do the equal comparison with patchedStatus.Status.LastScheduleTime.Equal, either to exercise that function or because it does something special that framework.ExpectEqual wouldn't do.

Either way, not changing the logic of the test makes sense, so 👍

framework.Failf("patched object should have the applied lastScheduleTime %#v, got %#v instead", cjStatus.LastScheduleTime, patchedStatus.Status.LastScheduleTime)
}
framework.ExpectEqual(patchedStatus.Annotations["patchedstatus"], "true", "patched object should have the applied annotation")

ginkgo.By("updating /status")
Expand All @@ -420,7 +428,9 @@ var _ = SIGDescribe("CronJob", func() {
return err
})
framework.ExpectNoError(err)
framework.ExpectEqual(updatedStatus.Status.LastScheduleTime.Equal(&now2), true, fmt.Sprintf("updated object status expected to have updated lastScheduleTime %#v, got %#v", statusToUpdate.Status.LastScheduleTime, updatedStatus.Status.LastScheduleTime))
if !updatedStatus.Status.LastScheduleTime.Equal(&now2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this working then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, the commits need to be squashed

framework.Failf("updated object status expected to have updated lastScheduleTime %#v, got %#v", statusToUpdate.Status.LastScheduleTime, updatedStatus.Status.LastScheduleTime)
}

ginkgo.By("get /status")
cjResource := schema.GroupVersionResource{Group: "batch", Version: cjVersion, Resource: "cronjobs"}
Expand All @@ -433,7 +443,9 @@ var _ = SIGDescribe("CronJob", func() {
// CronJob resource delete operations
expectFinalizer := func(cj *batchv1.CronJob, msg string) {
framework.ExpectNotEqual(cj.DeletionTimestamp, nil, fmt.Sprintf("expected deletionTimestamp, got nil on step: %q, cronjob: %+v", msg, cj))
framework.ExpectEqual(len(cj.Finalizers) > 0, true, fmt.Sprintf("expected finalizers on cronjob, got none on step: %q, cronjob: %+v", msg, cj))
if len(cj.Finalizers) == 0 {
framework.Failf("expected finalizers on cronjob, got none on step: %q, cronjob: %+v", msg, cj)
}
}

ginkgo.By("deleting")
Expand All @@ -447,7 +459,9 @@ var _ = SIGDescribe("CronJob", func() {
if err == nil {
expectFinalizer(cj, "deleting cronjob")
} else {
framework.ExpectEqual(apierrors.IsNotFound(err), true, fmt.Sprintf("expected 404, got %v", err))
if !apierrors.IsNotFound(err) {
framework.Failf("expected 404, got %v", err)
}
}

ginkgo.By("deleting a collection")
Expand All @@ -456,7 +470,10 @@ var _ = SIGDescribe("CronJob", func() {
cjs, err = cjClient.List(context.TODO(), metav1.ListOptions{LabelSelector: "special-label=" + f.UniqueName})
framework.ExpectNoError(err)
// Should have <= 2 items since some cronjobs might not have been deleted yet due to finalizers
framework.ExpectEqual(len(cjs.Items) <= 2, true, "filtered list should be <= 2")
if len(cjs.Items) > 2 {
framework.Logf("got unexpected filtered list: %v", cjs.Items)
framework.Fail("filtered list should be <= 2")
rafadc marked this conversation as resolved.
Show resolved Hide resolved
}
// Validate finalizers
for _, cj := range cjs.Items {
expectFinalizer(&cj, "deleting cronjob collection")
Expand Down
4 changes: 3 additions & 1 deletion test/e2e/apps/daemon_set.go
View file
Open in desktop
Expand Up @@ -495,7 +495,9 @@ var _ = SIGDescribe("Daemon set [Serial]", func() {
rollbackPods[pod.Name] = true
}
for _, pod := range existingPods {
framework.ExpectEqual(rollbackPods[pod.Name], true, fmt.Sprintf("unexpected pod %s be restarted", pod.Name))
if !rollbackPods[pod.Name] {
framework.Failf("unexpected pod %s be restarted", pod.Name)
}
}
})

Expand Down
8 changes: 6 additions & 2 deletions test/e2e/apps/deployment.go
View file
Open in desktop
Expand Up @@ -325,7 +325,9 @@ var _ = SIGDescribe("Deployment", func() {
break
}
}
framework.ExpectEqual(foundDeployment, true, "unable to find the Deployment in list", deploymentsList)
if !foundDeployment {
framework.Failf("unable to find the Deployment in the following list %v", deploymentsList)
}

ginkgo.By("updating the Deployment")
testDeploymentUpdate := testDeployment
Expand Down Expand Up @@ -677,7 +679,9 @@ func stopDeployment(c clientset.Interface, ns, deploymentName string) {
framework.Logf("Ensuring deployment %s was deleted", deploymentName)
_, err = c.AppsV1().Deployments(ns).Get(context.TODO(), deployment.Name, metav1.GetOptions{})
framework.ExpectError(err)
framework.ExpectEqual(apierrors.IsNotFound(err), true)
if !apierrors.IsNotFound(err) {
framework.Failf("Expected deployment %s to be deleted", deploymentName)
}
framework.Logf("Ensuring deployment %s's RSes were deleted", deploymentName)
selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector)
framework.ExpectNoError(err)
Expand Down
12 changes: 9 additions & 3 deletions test/e2e/apps/disruption.go
View file
Open in desktop
Expand Up @@ -317,7 +317,9 @@ var _ = SIGDescribe("DisruptionController", func() {
if c.shouldDeny {
err = cs.CoreV1().Pods(ns).EvictV1(context.TODO(), e)
framework.ExpectError(err, "pod eviction should fail")
framework.ExpectEqual(apierrors.HasStatusCause(err, policyv1.DisruptionBudgetCause), true, "pod eviction should fail with DisruptionBudget cause")
if !apierrors.HasStatusCause(err, policyv1.DisruptionBudgetCause) {
framework.Fail("pod eviction should fail with DisruptionBudget cause")
}
} else {
// Only wait for running pods in the "allow" case
// because one of shouldDeny cases relies on the
Expand Down Expand Up @@ -361,7 +363,9 @@ var _ = SIGDescribe("DisruptionController", func() {
}
err = cs.CoreV1().Pods(ns).EvictV1(context.TODO(), e)
framework.ExpectError(err, "pod eviction should fail")
framework.ExpectEqual(apierrors.HasStatusCause(err, policyv1.DisruptionBudgetCause), true, "pod eviction should fail with DisruptionBudget cause")
if !apierrors.HasStatusCause(err, policyv1.DisruptionBudgetCause) {
framework.Failf("pod eviction should fail with DisruptionBudget cause. The error was \"%v\"", err)
Copy link
Contributor Author

@rafadc rafadc Jun 4, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the error here in case of failure too

}

ginkgo.By("Updating the pdb to allow a pod to be evicted")
updatePDBOrDie(cs, ns, defaultName, func(pdb *policyv1.PodDisruptionBudget) *policyv1.PodDisruptionBudget {
Expand Down Expand Up @@ -399,7 +403,9 @@ var _ = SIGDescribe("DisruptionController", func() {
}
err = cs.CoreV1().Pods(ns).EvictV1(context.TODO(), e)
framework.ExpectError(err, "pod eviction should fail")
framework.ExpectEqual(apierrors.HasStatusCause(err, policyv1.DisruptionBudgetCause), true, "pod eviction should fail with DisruptionBudget cause")
if !apierrors.HasStatusCause(err, policyv1.DisruptionBudgetCause) {
framework.Failf("pod eviction should fail with DisruptionBudget cause. The error was \"%v\"", err)
}

ginkgo.By("Deleting the pdb to allow a pod to be evicted")
deletePDBOrDie(cs, ns, defaultName)
Expand Down
20 changes: 15 additions & 5 deletions test/e2e/apps/job.go
View file
Open in desktop
Expand Up @@ -126,7 +126,9 @@ var _ = SIGDescribe("Job", func() {
break
}
}
framework.ExpectEqual(exists, true)
if !exists {
framework.Failf("Expected suspended job to exist. It was not found.")
}

ginkgo.By("Updating the job with suspend=false")
job.Spec.Suspend = pointer.BoolPtr(false)
Expand Down Expand Up @@ -182,7 +184,9 @@ var _ = SIGDescribe("Job", func() {
break
}
}
framework.ExpectEqual(exists, true)
if !exists {
framework.Failf("Expected suspended job to exist. It was not found.")
}
})

/*
Expand Down Expand Up @@ -322,7 +326,9 @@ var _ = SIGDescribe("Job", func() {
ginkgo.By("Ensuring job was deleted")
_, err = e2ejob.GetJob(f.ClientSet, f.Namespace.Name, job.Name)
framework.ExpectError(err, "failed to ensure job %s was deleted in namespace: %s", job.Name, f.Namespace.Name)
framework.ExpectEqual(apierrors.IsNotFound(err), true)
if !apierrors.IsNotFound(err) {
framework.Failf("failed to ensure job %s was deleted in namespace: %s", job.Name, f.Namespace.Name)
}
})

/*
Expand Down Expand Up @@ -489,7 +495,9 @@ var _ = SIGDescribe("Job", func() {
[]byte(`{"metadata":{"annotations":{"patchedstatus":"true"}},"status":`+string(jStatusJSON)+`}`),
metav1.PatchOptions{}, "status")
framework.ExpectNoError(err)
framework.ExpectEqual(patchedStatus.Status.StartTime.Equal(&now1), true, "patched object should have the applied StartTime status")
if !patchedStatus.Status.StartTime.Equal(&now1) {
framework.Failf("patched object should have the applied StartTime %#v, got %#v instead", jStatus.StartTime, patchedStatus.Status.StartTime)
}
framework.ExpectEqual(patchedStatus.Annotations["patchedstatus"], "true", "patched object should have the applied annotation")

ginkgo.By("updating /status")
Expand All @@ -506,7 +514,9 @@ var _ = SIGDescribe("Job", func() {
return err
})
framework.ExpectNoError(err)
framework.ExpectEqual(updatedStatus.Status.StartTime.Equal(&now2), true, fmt.Sprintf("updated object status expected to have updated StartTime %#v, got %#v", statusToUpdate.Status.StartTime, updatedStatus.Status.StartTime))
if !updatedStatus.Status.StartTime.Equal(&now2) {
framework.Failf("updated object status expected to have updated StartTime %#v, got %#v", statusToUpdate.Status.StartTime, updatedStatus.Status.StartTime)
}

ginkgo.By("get /status")
jResource := schema.GroupVersionResource{Group: "batch", Version: "v1", Resource: "jobs"}
Expand Down
45 changes: 34 additions & 11 deletions test/e2e/apps/rc.go
View file
Open in desktop
Expand Up @@ -163,7 +163,9 @@ var _ = SIGDescribe("ReplicationController", func() {
return true, nil
})
framework.ExpectNoError(err, "Wait until condition with watch events should not return an error")
framework.ExpectEqual(eventFound, true, "failed to find RC %v event", watch.Added)
if !eventFound {
framework.Failf("failed to find RC %v event", watch.Added)
}

ginkgo.By("waiting for available Replicas")
eventFound = false
Expand All @@ -186,7 +188,9 @@ var _ = SIGDescribe("ReplicationController", func() {
return true, nil
})
framework.ExpectNoError(err, "Wait for condition with watch events should not return an error")
framework.ExpectEqual(eventFound, true, "RC has not reached ReadyReplicas count of %v", testRcInitialReplicaCount)
if !eventFound {
framework.Failf("RC has not reached ReadyReplicas count of %v", testRcInitialReplicaCount)
}

rcLabelPatchPayload, err := json.Marshal(v1.ReplicationController{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -212,7 +216,9 @@ var _ = SIGDescribe("ReplicationController", func() {
return true, nil
})
framework.ExpectNoError(err, "Wait until condition with watch events should not return an error")
framework.ExpectEqual(eventFound, true, "failed to find RC %v event", watch.Added)
if !eventFound {
framework.Failf("failed to find RC %v event", watch.Added)
}

rcStatusPatchPayload, err := json.Marshal(map[string]interface{}{
"status": map[string]interface{}{
Expand Down Expand Up @@ -240,7 +246,9 @@ var _ = SIGDescribe("ReplicationController", func() {
return true, nil
})
framework.ExpectNoError(err, "Wait until condition with watch events should not return an error")
framework.ExpectEqual(eventFound, true, "failed to find RC %v event", watch.Added)
if !eventFound {
framework.Failf("failed to find RC %v event", watch.Added)
}

ginkgo.By("waiting for available Replicas")
_, err = watchUntilWithoutRetry(context.TODO(), retryWatcher, func(watchEvent watch.Event) (bool, error) {
Expand All @@ -259,7 +267,9 @@ var _ = SIGDescribe("ReplicationController", func() {
return true, nil
})
framework.ExpectNoError(err, "Failed to find updated ready replica count")
framework.ExpectEqual(eventFound, true, "Failed to find updated ready replica count")
if !eventFound {
framework.Fail("Failed to find updated ready replica count")
}

ginkgo.By("fetching ReplicationController status")
rcStatusUnstructured, err := dc.Resource(rcResource).Namespace(testRcNamespace).Get(context.TODO(), testRcName, metav1.GetOptions{}, "status")
Expand Down Expand Up @@ -294,7 +304,9 @@ var _ = SIGDescribe("ReplicationController", func() {
return true, nil
})
framework.ExpectNoError(err, "Wait until condition with watch events should not return an error")
framework.ExpectEqual(eventFound, true, "failed to find RC %v event", watch.Added)
if !eventFound {
framework.Failf("failed to find RC %v event", watch.Added)
}

ginkgo.By("waiting for ReplicationController's scale to be the max amount")
eventFound = false
Expand All @@ -315,7 +327,9 @@ var _ = SIGDescribe("ReplicationController", func() {
return true, nil
})
framework.ExpectNoError(err, "Wait until condition with watch events should not return an error")
framework.ExpectEqual(eventFound, true, "Failed to find updated ready replica count")
if !eventFound {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this condition needed at all? We are not setting eventFound in the previous block.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It gets initialized as false in line 312 and set to true inside the callback function in line 326, so this check seems legitimate to me.

framework.Fail("Failed to find updated ready replica count")
}

// Get the ReplicationController
ginkgo.By("fetching ReplicationController; ensuring that it's patched")
Expand Down Expand Up @@ -345,12 +359,16 @@ var _ = SIGDescribe("ReplicationController", func() {
return true, nil
})
framework.ExpectNoError(err, "Wait until condition with watch events should not return an error")
framework.ExpectEqual(eventFound, true, "failed to find RC %v event", watch.Added)
if !eventFound {
framework.Failf("failed to find RC %v event", watch.Added)
}

ginkgo.By("listing all ReplicationControllers")
rcs, err := f.ClientSet.CoreV1().ReplicationControllers("").List(context.TODO(), metav1.ListOptions{LabelSelector: "test-rc-static=true"})
framework.ExpectNoError(err, "failed to list ReplicationController")
framework.ExpectEqual(len(rcs.Items) > 0, true)
if len(rcs.Items) == 0 {
framework.Fail("Expected to find a ReplicationController but none was found")
}

ginkgo.By("checking that ReplicationController has expected values")
foundRc := false
Expand All @@ -362,7 +380,10 @@ var _ = SIGDescribe("ReplicationController", func() {
foundRc = true
}
}
framework.ExpectEqual(foundRc, true)
if !foundRc {
framework.Logf("Got unexpected replication controller list %v", rcs.Items)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the suggestion before I logged this list too and made the message a bit more descriptive

framework.Failf("could not find ReplicationController %s", testRcName)
}

// Delete ReplicationController
ginkgo.By("deleting ReplicationControllers by collection")
Expand All @@ -382,7 +403,9 @@ var _ = SIGDescribe("ReplicationController", func() {
return true, nil
})
framework.ExpectNoError(err, "Wait until condition with watch events should not return an error")
framework.ExpectEqual(eventFound, true, "failed to find RC %v event", watch.Added)
if !eventFound {
framework.Failf("failed to find RC %v event", watch.Added)
}

return actualWatchEvents
}, func() (err error) {
Expand Down
8 changes: 6 additions & 2 deletions test/e2e/apps/ttl_after_finished.go
View file
Open in desktop
Expand Up @@ -97,13 +97,17 @@ func testFinishedJob(f *framework.Framework) {
framework.ExpectNoError(err)
jobFinishTime := finishTime(job)
finishTimeUTC := jobFinishTime.UTC()
framework.ExpectNotEqual(jobFinishTime.IsZero(), true)
if jobFinishTime.IsZero() {
framework.Fail("Expected job finish time not to be zero.")
}

deleteAtUTC := job.ObjectMeta.DeletionTimestamp.UTC()
framework.ExpectNotEqual(deleteAtUTC, nil)

expireAtUTC := finishTimeUTC.Add(time.Duration(ttl) * time.Second)
framework.ExpectEqual(deleteAtUTC.Before(expireAtUTC), false)
if deleteAtUTC.Before(expireAtUTC) {
framework.Fail("Expected job's deletion time to be after expiration time.")
}
}

// finishTime returns finish time of the specified job.
Expand Down