Skip to content

Clean up disabled tests #2 #22912

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 4 commits into from
Aug 13, 2019
Merged

Clean up disabled tests #2 #22912

openshift-merge-robot merged 4 commits into from
Aug 13, 2019

Conversation

@jsafrane
Copy link
Contributor

@jsafrane jsafrane commented May 28, 2019

  • All tests that need specific cloud have proper Skip()
  • Remove rules that don't match anything
  • Enable tests that depend on 1.13 kubelet

This is continuation of #22460

@smarterclayton: This looks like it substantially increased test runtime.

I don't see any increase (openshift/conformance/parallel was actually slower before the PR than it is now, I blame low nr. of samples).

There are two new real tests:

"[sig-storage] GCP Volumes GlusterFS should be mountable [Suite:openshift/conformance/parallel] [Suite:k8s]"
"[sig-storage] PersistentVolumes NFS when invoking the Recycle reclaim policy should test that a PV becomes Available and is clean after the PVC is deleted. 
[Suite:openshift/conformance/parallel] [Suite:k8s]"

Additional 37 tests are skipped (missing vsphere / openstack), each Skip() takes 2-3 seconds for BeforeEach + AfterEach (divide by nr. of parallel tests).

cc @openshift/storage

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 28, 2019
@jsafrane jsafrane mentioned this pull request May 28, 2019
Closed
@jsafrane
Copy link
Contributor Author

jsafrane commented May 28, 2019

Compared to a random green pull request #22898:

/test e2e-aws
/test e2e-aws-serial
(to get more numbers)

@jsafrane
Copy link
Contributor Author

jsafrane commented May 28, 2019

/test e2e-aws

@gnufied
Copy link
Member

gnufied commented May 29, 2019

@jsafrane mostly looks good to me. I am skeptical about re-enabling ceph-rbd tests. They do have Feature:Volumes tag but otherwise there is no logic to skip them currently. And since we still haven't sorted out installation of ceph-common, they will be broken.

@jsafrane
Copy link
Contributor Author

jsafrane commented May 29, 2019

I am skeptical about re-enabling ceph-rbd tests.

I think we should not enable them until we have all the binaries in place.

I don't want to do any smart detection in the tests - they should clearly fail when a binary is missing, so we get alert when someone removes them from our CoreOS images instead of silently skipping them.

@jsafrane
Copy link
Contributor Author

jsafrane commented May 29, 2019

/test e2e-aws

1 similar comment
@petr-muller
Copy link
Member

petr-muller commented May 29, 2019

/test e2e-aws

@jsafrane
Copy link
Contributor Author

jsafrane commented May 29, 2019

/test e2e-aws-serial

1 similar comment
@petr-muller
Copy link
Member

petr-muller commented May 29, 2019

/test e2e-aws-serial

@jsafrane
Copy link
Contributor Author

jsafrane commented May 29, 2019

/test unit

@jsafrane
Copy link
Contributor Author

jsafrane commented May 29, 2019

/test e2e-aws

@jsafrane
Copy link
Contributor Author

jsafrane commented May 29, 2019

/test e2e-aws-serial

@jsafrane
Copy link
Contributor Author

jsafrane commented May 29, 2019

/test verify

jsafrane
jsafrane commented May 29, 2019
View reviewed changes
test/extended/util/test.go
`vsphere`,
`Cinder`, // requires an OpenStack cluster
// See the CanSupport implementation in upstream to determine wether these work.
`Ceph RBD`, // Works if ceph-common Binary installed (but we can't guarantee this on all clusters).
Copy link
Contributor Author

@jsafrane jsafrane May 29, 2019

Choose a reason for hiding this comment

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

Note to reviewers, this does not enable ceph tests! This rule does not match any test, ceph tests have something like [Driver: rbd][Feature: Volumes].

@gnufied
Copy link
Member

gnufied commented May 29, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2019
@jsafrane
Copy link
Contributor Author

jsafrane commented May 29, 2019

/assign @smarterclayton
for approval

@smarterclayton
Copy link
Contributor

smarterclayton commented May 29, 2019

Only two new tests got run. Is that expected? (Master has 846, this has 848)

@jsafrane
Copy link
Contributor Author

jsafrane commented May 30, 2019

Yes, it is expected. Most of this PR is removing useless lines that don't match anything.

@jsafrane
Copy link
Contributor Author

jsafrane commented Jul 11, 2019

/retest

@smarterclayton
Copy link
Contributor

smarterclayton commented Jul 23, 2019

/test e2e-aws

@smarterclayton
Copy link
Contributor

smarterclayton commented Jul 23, 2019

/test all

@smarterclayton
Copy link
Contributor

smarterclayton commented Jul 23, 2019

/refresh

@jsafrane
Copy link
Contributor Author

jsafrane commented Jul 24, 2019

/retest

@jsafrane
Copy link
Contributor Author

jsafrane commented Jul 25, 2019

@smarterclayton now the tests are clean

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2019
@jsafrane
Enable cloud-based storage tests
7ff65f6 
All tests that need specific cloud have proper Skip().
@jsafrane
Remove Ceph RBD rule
52b5b5f 
It does not match any test. RBD tests are matched by [Driver: rbd]
below.
@jsafrane
Remove GlusterFS rule
66b2880 
It matches only one test and that one can run. Rest of Gluster tests have
[Driver: gluster] and they pass.
@jsafrane
Enable tests than need 1.13 kubelet
a9d7bb7
@jsafrane jsafrane force-pushed the enable-cloud-tests branch from 0e2e0a3 to a9d7bb7 Compare August 12, 2019 13:30
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 12, 2019
@jsafrane
Copy link
Contributor Author

jsafrane commented Aug 12, 2019

/retest

@smarterclayton
Copy link
Contributor

smarterclayton commented Aug 12, 2019

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2019
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, jsafrane, smarterclayton

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2019
@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 12, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 12, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 12, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 12, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 13, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 13, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit a547cf3 into openshift:master Aug 13, 2019
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 13, 2019

@jsafrane: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-cmd a9d7bb7 link /test e2e-cmd

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bparees bparees Awaiting requested review from bparees

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@gnufied gnufied

@smarterclayton smarterclayton

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jsafrane @gnufied @petr-muller @smarterclayton @openshift-ci-robot @openshift-bot @openshift-merge-robot