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

Check conformance test should not call any Skip #76734

Merged
merged 1 commit into from
May 11, 2019

Conversation

oomichi
Copy link
Member

@oomichi oomichi commented Apr 17, 2019

What type of PR is this?

/kind bug

What this PR does / why we need it:

Basically conformance test checks the target k8s cluster works all
features which are specified in each test and that should not depend
on any condition.
This adds checking that conformance test should not call any Skip
methods. And it detects the existing conformance test
"creating/deleting custom resource definition objects works"
calls framework.SkipUnlessServerVersionGTE().
This removes the Skip also.

Ref: #74432

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 17, 2019
@oomichi
Copy link
Member Author

oomichi commented Apr 17, 2019

pull-kubernetes-verify should detect it and be failed.

@oomichi
Copy link
Member Author

oomichi commented Apr 17, 2019

/area conformance

@k8s-ci-robot k8s-ci-robot added the area/conformance Issues or PRs related to kubernetes conformance tests label Apr 17, 2019
@oomichi
Copy link
Member Author

oomichi commented Apr 17, 2019

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/76734/pull-kubernetes-verify/1118601635224883211

/home/prow/go/src/k8s.io/kubernetes/test/e2e//apimachinery/custom_resource_definition.go: Conformance test should not call any framework.Skip*()
Error: We need to fix the above errors.
exit status 1
Errors from lint:
Please review the above warnings.

@spiffxp
Copy link
Member

spiffxp commented Apr 19, 2019

/priority important-soon
/sig architecture
/sig testing

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 19, 2019
@spiffxp
Copy link
Member

spiffxp commented Apr 19, 2019

The Skip line in the offending test can definitely be removed.

var crdVersion = utilversion.MustParseSemantic("v1.7.0")
//...
framework.SkipUnlessServerVersionGTE(crdVersion, f.ClientSet.Discovery())

/approve

Now that you've used this PR to demonstrate that we're catching a Skip, remove it and we should see the verify job go green?

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2019
@oomichi
Copy link
Member Author

oomichi commented Apr 22, 2019

@spiffxp

Now that you've used this PR to demonstrate that we're catching a Skip, remove it and we should see the verify job go green?

Yep, right. I wanted to demonstrate we can catch invalid usage of Skip in conformance tests and make a consensus that we should not have such Skip.
Let me remove it for passing the verifying job :-)

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/test and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 22, 2019
@oomichi oomichi force-pushed the conformance-req-01 branch 2 times, most recently from 20d6e29 to 1a39f8e Compare April 22, 2019 21:48
@oomichi
Copy link
Member Author

oomichi commented Apr 22, 2019

/retest

1 similar comment
@oomichi
Copy link
Member Author

oomichi commented Apr 23, 2019

/retest

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2019
@timothysc timothysc self-assigned this Apr 26, 2019
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2019
Basically conformance test checks the target k8s cluster works all
features which are specified in each test and that should not depend
on any condition.
This adds checking that conformance test should not call any Skip
methods. And it detects the existing conformance test
"creating/deleting custom resource definition objects works"
calls framework.SkipUnlessServerVersionGTE(). So this removes the
Skip also.
@oomichi
Copy link
Member Author

oomichi commented May 6, 2019

@timothysc Hi, could you take a look at this again? I have fixed your point, I think.

@spiffxp
Copy link
Member

spiffxp commented May 6, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2019
@timothysc
Copy link
Member

@oomichi lgtm I put you in the approval column waiting on final approval.

@timothysc timothysc added this to To Triage in conformance-definition via automation May 6, 2019
@timothysc timothysc moved this from To Triage to In Review in conformance-definition May 6, 2019
@timothysc timothysc moved this from In Review to Needs Approval in conformance-definition May 6, 2019
@oomichi
Copy link
Member Author

oomichi commented May 7, 2019

@timothysc

lgtm I put you in the approval column waiting on final approval.

I guess you are saying 'cncf-k8s-conformance-wg' needs to approve this before merging, right?

@spiffxp
Copy link
Member

spiffxp commented May 7, 2019

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from spiffxp May 7, 2019 15:01
@timothysc
Copy link
Member

/assign @bgrant0607 @smarterclayton

@smarterclayton
Copy link
Contributor

/hold cancel

/approve

@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 May 10, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oomichi, smarterclayton, spiffxp, timothysc

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

@k8s-ci-robot k8s-ci-robot merged commit a245408 into kubernetes:master May 11, 2019
conformance-definition automation moved this from Needs Approval to Done May 11, 2019
@smarterclayton
Copy link
Contributor

A conformance 1.14 cluster is hitting this (a test is skipped). Going to try a cherry-pick to 1.14, and if it doesn't work will do a more targeted fix.

/cherrypick release-1.14

@smarterclayton
Copy link
Contributor

Cherrypicked to #80598 manually, only the removal of the skips.

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. area/conformance Issues or PRs related to kubernetes conformance tests area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants