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

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
@oomichi

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

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

@oomichi

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

/area conformance

@oomichi

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

/priority important-soon
/sig architecture
/sig testing

@spiffxp

This comment has been minimized.

Copy link
Member

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?

@oomichi

This comment has been minimized.

Copy link
Member Author

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 :-)

@oomichi oomichi force-pushed the oomichi:conformance-req-01 branch from e6bc109 to 462083f Apr 22, 2019

@oomichi oomichi force-pushed the oomichi:conformance-req-01 branch 2 times, most recently from 20d6e29 to 1a39f8e Apr 22, 2019

@oomichi

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

/retest

1 similar comment
@oomichi

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

/retest

@timothysc
Copy link
Member

left a comment

/approve

please address comment

/hold

Show resolved Hide resolved test/e2e/common/downward_api.go

@timothysc timothysc self-assigned this Apr 26, 2019

@oomichi oomichi force-pushed the oomichi:conformance-req-01 branch from 1a39f8e to 5c108b0 May 3, 2019

@oomichi oomichi force-pushed the oomichi:conformance-req-01 branch from 5c108b0 to ec32403 May 3, 2019

Check conformance test should not call any Skip
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 oomichi force-pushed the oomichi:conformance-req-01 branch from ec32403 to 52885a8 May 3, 2019

@oomichi

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

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

@spiffxp

This comment has been minimized.

Copy link
Member

commented May 6, 2019

/lgtm

@timothysc

This comment has been minimized.

Copy link
Member

commented May 6, 2019

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

@timothysc timothysc added this to To Triage in cncf-k8s-conformance-wg via automation May 6, 2019

@timothysc timothysc moved this from To Triage to In Review in cncf-k8s-conformance-wg May 6, 2019

@timothysc timothysc moved this from In Review to Needs Approval in cncf-k8s-conformance-wg May 6, 2019

@oomichi

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented May 7, 2019

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from spiffxp May 7, 2019

@timothysc

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

/hold cancel

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

[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

20 checks passed

cla/linuxfoundation oomichi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

cncf-k8s-conformance-wg automation moved this from Needs Approval to Done May 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.