Enforce test style on e2e_node and e2e_kubeadm tests #78779

odinuge · 2019-06-06T22:30:45Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

This enforces the policy no more "Expect(err).NotTo(HaveOccurred())" on the e2e_node and e2e_kubeadm folders. Previously it was just enforced on the e2e-tests.

Which issue(s) this PR fixes:

Fixes #34059 (enforced by the script), there are still ~1500 framework.ExpectNoError(err)s without an assertion message tho.

Fixes #77103
/priority backlog

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-06-06T22:30:52Z

Hi @odinuge. 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.

odinuge · 2019-06-06T22:42:14Z

/assign @oomichi

oomichi · 2019-06-06T23:22:34Z

@odinuge big thanks for doing this :-)

/ok-to-test

oomichi · 2019-06-08T00:44:35Z

This is necessary to be updated for passing pull-kubernetes-bazel-build job by fixing the following failures:

/b/f/w/test/e2e_node/benchmark_util.go:36:2: imported and not used: "k8s.io/kubernetes/vendor/github.com/onsi/gomega"
/b/f/w/test/e2e_node/pods_container_manager_test.go:31:2: imported and not used: "k8s.io/kubernetes/vendor/github.com/onsi/gomega"
/b/f/w/test/e2e_node/resource_usage_test.go:33:2: imported and not used: "k8s.io/kubernetes/vendor/github.com/onsi/gomega"
/b/f/w/test/e2e_node/runtime_conformance_test.go:34:2: imported and not used: "k8s.io/kubernetes/vendor/github.com/onsi/gomega"
/b/f/w/test/e2e_node/volume_manager_test.go:30:2: imported and not used: "k8s.io/kubernetes/vendor/github.com/onsi/gomega"

oomichi · 2019-06-10T17:03:43Z

Thanks for updating,

/lgtm

oomichi · 2019-06-11T21:44:08Z

/approve

Remove the use of Except(err).NotTo(HaveOccured()), and switch to using framework ramework.ExpectNoError(err)

odinuge · 2019-06-14T16:31:57Z

/retest

Had to fix some conflicting files @oomichi 😄

oomichi · 2019-06-14T17:08:56Z

@odinuge OK, I will check this again after passing all test jobs.
Thanks for updating :-)

oomichi · 2019-06-14T17:44:42Z

/test pull-kubernetes-integration

odinuge · 2019-06-14T18:25:19Z

/test pull-kubernetes-integration

oomichi · 2019-06-14T19:05:53Z

/test pull-kubernetes-bazel-test

oomichi · 2019-06-14T19:59:36Z

/lgtm

odinuge · 2019-06-18T17:51:10Z

/assign @vishh

BenTheElder · 2019-06-19T20:18:02Z

hack/verify-test-code.sh

@@ -21,7 +21,7 @@ KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/..
 cd "${KUBE_ROOT}"

 # NOTE: This checks e2e test code without the e2e framework which contains Expect().To(HaveOccurred())
-mapfile -t all_e2e_files < <(find test/e2e -name '*.go' | grep -v 'test/e2e/framework/')
+mapfile -t all_e2e_files < <(find test/e2e{,_node,_kubeadm} -name '*.go' | grep -v 'test/e2e/framework/')


NOTE: see previous discussions in this repo: we don't use mapfile because it doesn't exist on macOS. there are examples of alternatives eleswhere in hack/ such as the verify-shellcheck script (read loop)

this particular change looks fine, but the original implementation probably should not have introduced mapfile usage

@BenTheElder I see, thanks for your comment. I will follow the mapfile issue with another PR with some digging.

There is a tool inside hack/lib/util.sh called kube::util::read-array that works the same way as mapfile -t, and works in bash3 😄 @oomichi @BenTheElder But yeah, that can be fixed in another PR.

kubernetes/hack/lib/util.sh

Line 715 in b3a73f7

function kube::util::read-array {

@oomichi @odinuge thanks! that's exactly the thing to do, let's do that in another PR. feel free to ping me on that one for approval 😅

#79186 is the follow-up PR.

BenTheElder

/lgtm
/approve
we should follow up on the one comment, but it wasn't introduced in this PR..

k8s-ci-robot · 2019-06-19T20:20:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, odinuge, oomichi

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:

~~hack/OWNERS~~ [BenTheElder]
~~test/e2e_node/OWNERS~~ [BenTheElder,oomichi]

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

During the review of kubernetes#78779 we've known mapfile doesn't work on macOS. So we need to use alternative way instead and this is it.

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 6, 2019

k8s-ci-robot requested review from Random-Liu and sttts June 6, 2019 22:31

k8s-ci-robot assigned oomichi Jun 6, 2019

odinuge mentioned this pull request Jun 6, 2019

Use framework.ExpectNoError() for e2e tests #77103

Closed

23 tasks

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 Jun 6, 2019

odinuge force-pushed the test-cleanup branch from 935ece6 to 05964d6 Compare June 8, 2019 17:44

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2019

odinuge force-pushed the test-cleanup branch from 05964d6 to ae57b7c Compare June 14, 2019 15:44

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2019

odinuge added 2 commits June 14, 2019 17:51

Cleanup e2e_node test style

6bb8bb0

Remove the use of Except(err).NotTo(HaveOccured()), and switch to using framework ramework.ExpectNoError(err)

Run verify-test-code on all e2e tests

b0a1cba

odinuge force-pushed the test-cleanup branch from ae57b7c to b0a1cba Compare June 14, 2019 15:51

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2019

k8s-ci-robot assigned vishh Jun 18, 2019

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 18, 2019

BenTheElder reviewed Jun 19, 2019

View reviewed changes

k8s-ci-robot assigned BenTheElder Jun 19, 2019

BenTheElder approved these changes Jun 19, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2019

oomichi mentioned this pull request Jun 19, 2019

Replace mapfile with kube::util::read-array #79186

Merged

k8s-ci-robot merged commit c2ff62c into kubernetes:master Jun 19, 2019

oomichi pushed a commit to oomichi/kubernetes that referenced this pull request Jun 20, 2019

Replace mapfile with kube::util::read-array

5628b6b

During the review of kubernetes#78779 we've known mapfile doesn't work on macOS. So we need to use alternative way instead and this is it.

odinuge mentioned this pull request Jul 1, 2019

REQUEST: New membership for odinuge kubernetes/org#975

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce test style on e2e_node and e2e_kubeadm tests #78779

Enforce test style on e2e_node and e2e_kubeadm tests #78779

odinuge commented Jun 6, 2019 •

edited

k8s-ci-robot commented Jun 6, 2019

odinuge commented Jun 6, 2019

oomichi commented Jun 6, 2019

oomichi commented Jun 8, 2019

oomichi commented Jun 10, 2019

oomichi commented Jun 11, 2019

odinuge commented Jun 14, 2019

oomichi commented Jun 14, 2019

oomichi commented Jun 14, 2019

odinuge commented Jun 14, 2019

oomichi commented Jun 14, 2019

oomichi commented Jun 14, 2019

odinuge commented Jun 18, 2019

BenTheElder Jun 19, 2019

BenTheElder Jun 19, 2019

oomichi Jun 19, 2019

odinuge Jun 19, 2019

BenTheElder Jun 19, 2019

oomichi Jun 19, 2019

BenTheElder left a comment

k8s-ci-robot commented Jun 19, 2019

Enforce test style on e2e_node and e2e_kubeadm tests #78779

Enforce test style on e2e_node and e2e_kubeadm tests #78779

Conversation

odinuge commented Jun 6, 2019 • edited

k8s-ci-robot commented Jun 6, 2019

odinuge commented Jun 6, 2019

oomichi commented Jun 6, 2019

oomichi commented Jun 8, 2019

oomichi commented Jun 10, 2019

oomichi commented Jun 11, 2019

odinuge commented Jun 14, 2019

oomichi commented Jun 14, 2019

oomichi commented Jun 14, 2019

odinuge commented Jun 14, 2019

oomichi commented Jun 14, 2019

oomichi commented Jun 14, 2019

odinuge commented Jun 18, 2019

BenTheElder Jun 19, 2019

Choose a reason for hiding this comment

BenTheElder Jun 19, 2019

Choose a reason for hiding this comment

oomichi Jun 19, 2019

Choose a reason for hiding this comment

odinuge Jun 19, 2019

Choose a reason for hiding this comment

BenTheElder Jun 19, 2019

Choose a reason for hiding this comment

oomichi Jun 19, 2019

Choose a reason for hiding this comment

BenTheElder left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 19, 2019

odinuge commented Jun 6, 2019 •

edited