Improve storage test skipping pattern. #103876

juanfescobar · 2021-07-23T14:50:15Z

What type of PR is this?

/kind bug
/kind flake

What this PR does / why we need it:

Include a [Flaky] tag in the test name so that it can be always skipped, as it is expected currently.

The reason for this skipping-pattern update is that the current one does not address the current expected behavior. Including a skip method as the first line of a test does not prevent the test to run the BeforeEach function (which can fail before the test is skipped), and thus the test could fail even though it was supposed to be always skipped.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

Including a skip method as the first line of a test does not prevent the test to fail in the BeforeEach function. If the test is skipped because of a tag in the name, then we can prevent such odd behavior.

k8s-ci-robot · 2021-07-23T14:50:17Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2021-07-23T14:50:22Z

Welcome @juanfescobar!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-07-23T14:50:23Z

Hi @juanfescobar. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

juanfescobar · 2021-07-23T14:59:55Z

/assign @spiffxp

spiffxp · 2021-07-23T15:26:56Z

/ok-to-test

spiffxp · 2021-07-23T15:29:30Z

ref: #73168

spiffxp

Thanks, this is a much better way of describing that a test is flaky

/milestone v1.23
If this is impacting v1.22 we can change, otherwise we should wait until the branch opens back up

spiffxp · 2021-07-23T15:53:30Z

/lgtm

BenTheElder · 2021-07-23T15:58:45Z

test/e2e/storage/persistent_volumes-local.go

@@ -283,8 +283,8 @@ var _ = utils.SIGDescribe("PersistentVolumes-local ", func() {
 					e2epod.DeletePodOrFail(config.client, config.ns, pod2.Name)
 				})

-				ginkgo.It("should set different fsGroup for second pod if first pod is deleted", func() {
-					e2eskipper.Skipf("Disabled temporarily, reopen after #73168 is fixed")


FYI @kubernetes/sig-storage-bugs this previous approach does not result in skipping the BeforeEach which can then fail (setting up volumes to test) and waste time. It also prevents the possibility of actually running the test. When the BeforeEach fails it will report red, resulting in a dashboard of Red or no result which can be confusing 🙃

We should prefer adding a [Flaky] tag or similar instead which the user can select and should prevent running the case entirely instead of running it up to the skip line (including Before*).

BenTheElder · 2021-07-23T16:00:16Z

/triage accepted
/priority important-soon

jingxu97 · 2021-07-23T18:19:22Z

/lgtm

msau42 · 2021-07-23T18:22:06Z

#102344 may actually fix the flakiness. We can followup and monitor the test once it's running again.

msau42 · 2021-07-23T18:22:11Z

/approve

k8s-ci-robot · 2021-07-23T18:22:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, juanfescobar, msau42, spiffxp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/e2e/storage/OWNERS~~ [BenTheElder,msau42,spiffxp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spiffxp · 2021-07-23T19:50:46Z

Thanks @msau42 I meant for you to /approve I didn't realize my review got flagged as an approve as well

dims · 2021-08-05T10:33:25Z

/ok-to-test

k8s-triage-robot · 2021-08-05T15:26:08Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

juanfescobar · 2021-08-05T16:39:08Z

/retest

Improve storage test skipping pattern.

fef5c0c

Including a skip method as the first line of a test does not prevent the test to fail in the BeforeEach function. If the test is skipped because of a tag in the name, then we can prevent such odd behavior.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. labels Jul 23, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 23, 2021

k8s-ci-robot requested review from jeffvance and jingxu97 July 23, 2021 14:50

k8s-ci-robot added area/test sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 23, 2021

k8s-ci-robot assigned spiffxp Jul 23, 2021

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 23, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 23, 2021

spiffxp approved these changes Jul 23, 2021

View reviewed changes

k8s-ci-robot added this to the v1.23 milestone Jul 23, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2021

BenTheElder reviewed Jul 23, 2021

View reviewed changes

BenTheElder approved these changes Jul 23, 2021

View reviewed changes

k8s-ci-robot assigned jingxu97 Jul 23, 2021

k8s-ci-robot merged commit 7820de3 into kubernetes:master Aug 5, 2021

juanfescobar mentioned this pull request Jan 18, 2022

REQUEST: New membership for juanfescobar kubernetes/org#3193

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve storage test skipping pattern. #103876

Improve storage test skipping pattern. #103876

juanfescobar commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

juanfescobar commented Jul 23, 2021

spiffxp commented Jul 23, 2021

spiffxp commented Jul 23, 2021

spiffxp left a comment

spiffxp commented Jul 23, 2021

BenTheElder Jul 23, 2021 •

edited

BenTheElder commented Jul 23, 2021

jingxu97 commented Jul 23, 2021

msau42 commented Jul 23, 2021

msau42 commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

spiffxp commented Jul 23, 2021

dims commented Aug 5, 2021

k8s-triage-robot commented Aug 5, 2021

juanfescobar commented Aug 5, 2021

Improve storage test skipping pattern. #103876

Improve storage test skipping pattern. #103876

Conversation

juanfescobar commented Jul 23, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

juanfescobar commented Jul 23, 2021

spiffxp commented Jul 23, 2021

spiffxp commented Jul 23, 2021

spiffxp left a comment

Choose a reason for hiding this comment

spiffxp commented Jul 23, 2021

BenTheElder Jul 23, 2021 • edited

Choose a reason for hiding this comment

BenTheElder commented Jul 23, 2021

jingxu97 commented Jul 23, 2021

msau42 commented Jul 23, 2021

msau42 commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

spiffxp commented Jul 23, 2021

dims commented Aug 5, 2021

k8s-triage-robot commented Aug 5, 2021

juanfescobar commented Aug 5, 2021

BenTheElder Jul 23, 2021 •

edited