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.

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

test/e2e: rename ginkgo focus for tests #1065

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Feb 21, 2023

Make it easier to only run tests for nfd master/worker and skip topology-updater tests.

Make it easier to only run tests for nfd master/worker and skip
topology-updater tests.
@netlify
Copy link

netlify bot commented Feb 21, 2023

Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit adf79d5
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/63f4ae05aec68600086cb884
😎 Deploy Preview https://deploy-preview-1065--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 21, 2023
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

/hold
/lgtm
feel free to unhold anytime. I think we should start to use ginkgo v2 labels (https://onsi.github.io/ginkgo/MIGRATING_TO_V2#spec-labels)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 36ce3236d82efb845354824d7210580e5f1c9464

@marquiz
Copy link
Contributor Author

marquiz commented Feb 21, 2023

Thanks @ffromani.

feel free to unhold anytime. I think we should start to use ginkgo v2 labels (https://onsi.github.io/ginkgo/MIGRATING_TO_V2#spec-labels)

Maybe 🤔 We're using kubernetest e2e framework and basically just copied test/e2e/apps/framework.go from K8s. I think nothing prevents us from diverging from the k8s e2e-framework in this aspect. But maybe that's a subject of a separate PR. Something that possibly needs a bit more thought – the tests should be split into multiple independent specs in order to get something out of the labels. WDYT?

@ffromani
Copy link
Contributor

ffromani commented Feb 21, 2023

Thanks @ffromani.

feel free to unhold anytime. I think we should start to use ginkgo v2 labels (https://onsi.github.io/ginkgo/MIGRATING_TO_V2#spec-labels)

Maybe thinking We're using kubernetest e2e framework and basically just copied test/e2e/apps/framework.go from K8s. I think nothing prevents us from diverging from the k8s e2e-framework in this aspect. But maybe that's a subject of a separate PR. Something that possibly needs a bit more thought – the tests should be split into multiple independent specs in order to get something out of the labels. WDYT?

these are very good points I don't have answers for and it's possible this is indeed (a temporary) blocker for labels.
I brought the labels up because it seems a much nicer and more flexible way to select specific tests than the focus approach.
Obviously I'm fine fixing the description. I agree reorganizing the test suite would be beneficial.
And yes, this is definitely material for a separate discussion.

NOTE: I'm holding only to allow other reviewers to chime in - holding is not about the label conversation

Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, marquiz

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:

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

@ffromani
Copy link
Contributor

/hold cancel

because #1065 (review)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2023
@k8s-ci-robot k8s-ci-robot merged commit fab3c88 into kubernetes-sigs:master Feb 22, 2023
@marquiz marquiz mentioned this pull request Apr 12, 2023
24 tasks
@marquiz marquiz deleted the devel/e2e-focus branch April 19, 2023 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants