WaitFor<something> + ExpectError anti-pattern

pohly commented 1 year ago

What would you like to be added?

While working on https://github.com/kubernetes/kubernetes/pull/114825 I found tests that used WaitForPodRunning followed by ExpectError because they create a pod that is not supposed to get started.

A better approach is to gomega.Consistently for pod is pending:

diff --git a/test/e2e/common/node/expansion.go b/test/e2e/common/node/expansion.go
index 1b0b13bf287..07e55dfad63 100644
--- a/test/e2e/common/node/expansion.go
+++ b/test/e2e/common/node/expansion.go
@@ -29,6 +29,7 @@ import (
        admissionapi "k8s.io/pod-security-admission/api"

        "github.com/onsi/ginkgo/v2"
+       "github.com/onsi/gomega"
 )

 // These tests exercise the Kubernetes expansion syntax $(VAR).
@@ -267,8 +268,8 @@ var _ = SIGDescribe("Variable Expansion", func() {
                podClient := e2epod.NewPodClient(f)
                pod = podClient.Create(ctx, pod)

-               err := e2epod.WaitTimeoutForPodRunningInNamespace(ctx, f.ClientSet, pod.Name, pod.Namespace, framework.PodStartShortTimeout)
-               framework.ExpectError(err, "while waiting for pod to be running")
+               getPod := e2epod.Get(f.ClientSet, pod)
+               gomega.Consistently(ctx, getPod).WithTimeout(framework.PodStartShortTimeout).Should(e2epod.BeInPhase(v1.PodPending))

                ginkgo.By("updating the pod")
                podClient.Update(ctx, pod.ObjectMeta.Name, func(pod *v1.Pod) {

I know of at least two more tests which I haven't changed yet:

PersistentVolumes-local Local volume that cannot be mounted [Slow] should fail due to non-existent path
PersistentVolumes-local Local volume that cannot be mounted [Slow] should fail due to wrong node

Besides fixing those two tests, we also need to think about how we can find other instances and how to guide developers towards writing better tests.

Perhaps framework.ExpectError should be deprecated? If an error is expected, shouldn't the test ensure that the expected error occurred and not some error?

Why is this needed?

test semantic more obvious
gomega.Consistently integrates better with test execution

pohly commented 1 year ago

/sig storage

pohly commented 1 year ago

/sig testing

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue as fresh with /remove-lifecycle stale
Close this issue with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

pohly commented 1 year ago

/remove-lifecycle stale

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue as fresh with /remove-lifecycle stale
Close this issue with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

xing-yang commented 1 year ago

/triage accepted

xing-yang commented 1 year ago

/help

k8s-ci-robot commented 1 year ago

@xing-yang: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

Why are we solving this issue?
To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
Does this issue have zero to low barrier of entry?
How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes/kubernetes/issues/115164): >/help Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

carlory commented 1 year ago

/assign

vaibhav2107 commented 1 year ago

/remove-lifecycle stale

k8s-triage-robot commented 3 months ago

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

Confirm that this issue is still relevant with /triage accepted (org members only)
Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

carlory commented 3 months ago

/triage accepted

kubernetes / kubernetes