Use framework.ExpectNoError() for e2e tests #77103

oomichi · 2019-04-26T00:05:05Z

What happened:

After fixing golint failures under test/e2e (now still in progress), we need to put gomega for each Expect() and HaveOccurred().
Then very common error checking code has become like:

gomega.Expect(err).NotTo(gomega.HaveOccurred())

We can replace this code with framework.ExpectNoError(err) which is more readable and easy to be understood what the code does.

Now we have following e2e test directories which can be replaced:
( - [x] shows done)

After finishing all the above, we need to add the checking script to the CI system for blocking PRs which contain this issue:

Add hack/verify-test-code.sh #77612

The text was updated successfully, but these errors were encountered:

oomichi · 2019-04-26T00:05:22Z

/sig testing

atoato88 · 2019-04-26T01:24:30Z

I can work to following items.

test/e2e/lifecycle
test/e2e/lifecycle/bootstrap

k-toyoda-pi · 2019-04-26T01:27:40Z

I think that we can replace gomega.Expect(err).NotTo(gomega.HaveOccurred()), too.
Files in the following directories have it. Could you add these directories to list?

test/e2e/apimachinery
test/e2e/apps
test/e2e/instrumentation
test/e2e/kubectl
test/e2e/servicecatalog
test/e2e/ui
test/e2e/upgrades/apps
test/e2e/upgrades
test/e2e/upgrades/storage
test/e2e/windows

oomichi · 2019-04-26T01:33:40Z

@k-toyoda-pi Thanks, I updated the list based on your comment :-)

k-toyoda-pi · 2019-04-26T01:36:49Z

Thanks, I can work to the following item.

test/e2e/apps

s-ito-ts · 2019-04-26T01:45:33Z

I can work to the following item.

test/e2e/autoscaling

draveness · 2019-04-27T07:15:55Z

I want to take the files below.

test/e2e/apimachinery
test/e2e/network
test/e2e/node
test/e2e/instrumentation
test/e2e/kubectl
test/e2e/scalability
test/e2e/scheduling
test/e2e/servicecatalog
test/e2e/ui
test/e2e/upgrades/apps
test/e2e/upgrades
test/e2e/upgrades/storage
test/e2e/windows

And I use the following cmd to do the refactor, which may be helpful.

ag -l "gomega\.Expect\(err\)\.NotTo\(gomega.HaveOccurred\(\)\)" test/e2e/network/**/*.go test/e2e/node/**/*.go test/e2e/instrumentation/**/*.go ... | xargs sed -i '' -e 's/gomega\.Expect(err)\.NotTo(gomega.HaveOccurred())/framework\.ExpectNoError(err)/g'

oomichi · 2019-05-08T18:06:59Z

I am creating a hacking script which detects what code we need to update for this issue as #77612
Still we have almost 400 lines what we need to update at this time.

draveness · 2019-05-09T01:47:32Z

Here is the remaining files with gomega.Expect(err)...

$ ag -l "gomega\.Expect\(err\)\.NotTo\(gomega.HaveOccurred\(\)\)" .
test/e2e/lifecycle/kubelet_security.go
test/e2e/lifecycle/reboot.go
test/e2e/lifecycle/restart.go
test/e2e/lifecycle/resize_nodes.go
test/e2e/common/pods.go
test/e2e/upgrades/storage/persistent_volumes.go
test/e2e/upgrades/storage/volume_mode.go
test/e2e/upgrades/apps/statefulset.go
test/e2e/upgrades/apps/job.go
test/e2e/apps/deployment.go
test/e2e/apps/daemon_set.go

I'd like to take files except for test/e2e/apps and test/e2e/lifecycle since they are already taken.

test/e2e/common/pods.go
test/e2e/upgrades/storage/persistent_volumes.go
test/e2e/upgrades/storage/volume_mode.go
test/e2e/upgrades/apps/statefulset.go
test/e2e/upgrades/apps/job.go

draveness · 2019-05-09T16:25:44Z

It seems like this issue will be solved after #77627 #77649 #77579 merged. :)

oomichi · 2019-05-09T18:39:38Z

@draveness

$ ag -l "gomega\.Expect$err$\.NotTo$gomega.HaveOccurred\($\)" which you are using cannot cover the following case which contains error message:
gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get pod %q", pod.Name)
So still we have a lot of targets at this time :-(

oomichi · 2019-05-09T19:19:03Z

Thank you everyone who are fighting for this issue.
After the pull-kubernetes-verify job of #77612 pass, we can close this issue.

oomichi · 2019-05-09T19:19:53Z

Feel free to /retest on #77612 to verify our work status.

draveness · 2019-05-15T02:05:36Z

This is the remaining file with the common formats

gomega.Expect(err).NotTo(gomega.HaveOccurred(), msg)
gomega.Expect(err).NotTo(gomega.HaveOccurred())

$ ag -l "gomega\.Expect\(err\)\.NotTo\(gomega.HaveOccurred\(\).*"
test/e2e/network/dns.go
test/e2e/network/firewall.go
test/e2e/network/proxy.go
test/e2e/network/service.go
test/e2e/network/ingress.go
test/e2e/network/dns_common.go
test/e2e/network/example_cluster_dns.go
test/e2e/network/network_tiers.go
test/e2e/network/network_policy.go
test/e2e/common/pods.go
test/e2e/node/kubelet.go
test/e2e/node/security_context.go
test/e2e/node/pre_stop.go
test/e2e/node/pods.go
test/e2e/node/ttlafterfinished.go
test/e2e/node/kubelet_perf.go
test/e2e/apps/disruption.go

I'll take care of the test/e2e/apps/disruption.go, and the other files seem like already been taken.

oomichi · 2019-05-15T02:34:26Z

Thanks for doing this again @draveness
test/e2e/apps/disruption.go had been fixed at once, but after that new test code contains gomega.Expect(err).NotTo(gomega.HaveOccurred()) again.
So it is nice to take care of that again and now I really want to enable #77612 at the CI so that we don't need to take care of this issue forever.

draveness · 2019-05-15T02:44:59Z

Thanks for doing this again @draveness
test/e2e/apps/disruption.go had been fixed at once, but after that new test code contains gomega.Expect(err).NotTo(gomega.HaveOccurred()) again.
So it is nice to take care of that again and now I really want to enable #77612 at the CI so that we don't need to take care of this issue forever.

Yeah, that would be great, but we have to fix all the gomega.Expect(err).NotTo(gomega.HaveOccurred()) ahead.

k-toyoda-pi · 2019-05-15T07:48:21Z

I'm done all files in e2e/network at #77901 #77909.

oomichi · 2019-05-15T21:51:32Z

Thanks everyone, all PRs are ready for fixing this issue at this time.
I'd like to get attention from reviewers for making progress :-)

k-toyoda-pi · 2019-05-17T04:07:57Z

It seems that some files in e2e/common contain "Expect(err).NotTo(HaveOccurred()".
Is someone working to that? Can I take it?

test/e2e/common/configmap_volume.go
test/e2e/common/downwardapi_volume.go
test/e2e/common/expansion.go
test/e2e/common/init_container.go
test/e2e/common/node_lease.go
test/e2e/common/privileged.go
test/e2e/common/projected_configmap.go
test/e2e/common/projected_downwardapi.go
test/e2e/common/projected_secret.go
test/e2e/common/secrets_volume.go
test/e2e/common/volumes.go

Oops, it might cause conflict between #77852 and this ...

draveness · 2019-05-17T06:22:27Z

It seems that some files in e2e/common contain "Expect(err).NotTo(HaveOccurred()".
Is someone working to that? Can I take it?

test/e2e/common/configmap_volume.go

test/e2e/common/downwardapi_volume.go

test/e2e/common/expansion.go

test/e2e/common/init_container.go

test/e2e/common/node_lease.go

test/e2e/common/privileged.go

test/e2e/common/projected_configmap.go

test/e2e/common/projected_downwardapi.go

test/e2e/common/projected_secret.go

test/e2e/common/secrets_volume.go

test/e2e/common/volumes.go

Oops, it might cause conflict between #77852 and this ...

I'll work on this after the above PRs get merged.

k-toyoda-pi · 2019-05-17T09:00:53Z

I believe that NotTo is the same behavior as ToNot.
So, can we replace Expect(err).ToNot(HaveOccurred()) to ExpectNoError?

s-ito-ts · 2019-05-17T10:01:16Z

It seems that "Expect(err).NotTo(gomega.HaveOccurred())" remains with some files under test/e2e/storage/vsphere.
I fix it in #78021

oomichi · 2019-05-17T16:44:13Z

@k-toyoda-pi

I believe that NotTo is the same behavior as ToNot.
So, can we replace Expect(err).ToNot(HaveOccurred()) to ExpectNoError?

This is a really nice point, I also think they are the same.
However, it is better to concentrate on 'NotTo' case only at this time because that is majority and many people are still adding it. We need to put the CI script for blocking these additions with #77612
After that, we will be able to take care of 'ToNot' case with another issue.

oomichi · 2019-05-17T16:44:41Z

@s-ito-ts Thanks approved.

oomichi · 2019-05-17T17:35:09Z

@k-toyoda-pi

It seems that some files in e2e/common contain "Expect(err).NotTo(HaveOccurred()".
Is someone working to that? Can I take it?

Sorry, that was my bad. I've taken care of that with the above PR.
I will try asking folks in KubeCon next week so that these all PRs are merged.

odinuge · 2019-06-06T22:46:20Z

Hi! Noticed that this only checked the normal e2e tests. I have made the script check e2e_node and e2e_kubeadm as well in #78779.

oomichi added the kind/bug Categorizes issue or PR as related to a bug. label Apr 26, 2019

k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Apr 26, 2019

k8s-ci-robot added 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 Apr 26, 2019

oomichi mentioned this issue Apr 26, 2019

Use framework.ExpectNoError() for e2e/auth #77093

Merged

s-ito-ts mentioned this issue Apr 26, 2019

Use framework.ExpectNoError() for e2e/autoscaling #77113

Merged

k-toyoda-pi mentioned this issue Apr 26, 2019

use framework.ExpectNoError() for e2e/apps #77123

Merged

liggitt added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/bug Categorizes issue or PR as related to a bug. labels Apr 27, 2019

draveness mentioned this issue Apr 27, 2019

refactor: use framework.ExpectNoError instead in e2e tests #77159

Merged

k-toyoda-pi mentioned this issue May 8, 2019

use framework.ExpectNoError() for daemon_set.go and deployment.go #77579

Merged

oomichi mentioned this issue May 8, 2019

Add hack/verify-test-code.sh #77612

Merged

draveness mentioned this issue May 9, 2019

refactor: use framework.ExpectNoError instead #77627

Merged

atoato88 mentioned this issue May 9, 2019

Use framework.ExpectNoError() for e2e/lifecycle #77649

Merged

This was referenced May 9, 2019

Use framework.ExpectNoError() for e2e/common #77692

Merged

Use framework.ExpectNoError() for e2e/ #77693

Merged

draveness mentioned this issue May 10, 2019

feat: use framework.ExpectNoError instead in e2e test #77708

Merged

draveness mentioned this issue May 15, 2019

feat: use framework.ExpectNoError in e2e apps disruption #77894

Merged

k-toyoda-pi mentioned this issue May 15, 2019

use framework.ExpectNoError() for e2e/network #77901

Merged

s-ito-ts mentioned this issue May 15, 2019

Use framework.ExpectNoError() for e2e/node #77902

Merged

k-toyoda-pi mentioned this issue May 15, 2019

use framework.ExpectNoError() for e2e/network/ingress.go service.go #77909

Merged

This was referenced May 15, 2019

Use framework.ExpectError() for e2e tests #77706

Closed

Use ExpectNoError() for e2e/upgrades #77955

Merged

oomichi mentioned this issue May 16, 2019

E2E test for GPU job interrupted by node recreate #76401

Merged

s-ito-ts mentioned this issue May 17, 2019

Use framework.ExpectNoError() for e2e/storage/vsphere #78021

Merged

oomichi mentioned this issue May 17, 2019

Use framework.ExpectNoError() on e2e/common #78042

Merged

k8s-ci-robot closed this as completed in #77612 May 29, 2019

draveness mentioned this issue May 29, 2019

fix: use framework.ExpectNoError instead of gomega #78478

Merged

k-toyoda-pi mentioned this issue Jun 5, 2019

REQUEST: New membership for k-toyoda-pi kubernetes/org#885

Closed

6 tasks

s-ito-ts mentioned this issue Jun 5, 2019

REQUEST: New membership for s-ito-ts kubernetes/org#886

Closed

6 tasks

oomichi mentioned this issue Jun 5, 2019

Make the check strict to use ExpectNoError() #78740

Merged

odinuge mentioned this issue Jun 6, 2019

Enforce test style on e2e_node and e2e_kubeadm tests #78779

Merged

odinuge mentioned this issue Jul 1, 2019

REQUEST: New membership for odinuge kubernetes/org#975

Closed

6 tasks

alejandrox1 mentioned this issue Jan 9, 2020

Replace gomega.Expect(bool).To(gomega.BeTrue(),...) with framework.ExpectEqual() #86206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use framework.ExpectNoError() for e2e tests #77103

Use framework.ExpectNoError() for e2e tests #77103

oomichi commented Apr 26, 2019 •

edited

Loading

oomichi commented Apr 26, 2019

atoato88 commented Apr 26, 2019

k-toyoda-pi commented Apr 26, 2019

oomichi commented Apr 26, 2019

k-toyoda-pi commented Apr 26, 2019 •

edited

Loading

s-ito-ts commented Apr 26, 2019

draveness commented Apr 27, 2019 •

edited

Loading

oomichi commented May 8, 2019

draveness commented May 9, 2019 •

edited

Loading

draveness commented May 9, 2019 •

edited

Loading

oomichi commented May 9, 2019

oomichi commented May 9, 2019

oomichi commented May 9, 2019

draveness commented May 15, 2019 •

edited

Loading

oomichi commented May 15, 2019

draveness commented May 15, 2019

k-toyoda-pi commented May 15, 2019 •

edited

Loading

oomichi commented May 15, 2019

k-toyoda-pi commented May 17, 2019

draveness commented May 17, 2019

k-toyoda-pi commented May 17, 2019

s-ito-ts commented May 17, 2019

oomichi commented May 17, 2019

oomichi commented May 17, 2019

oomichi commented May 17, 2019

odinuge commented Jun 6, 2019

Use framework.ExpectNoError() for e2e tests #77103

Use framework.ExpectNoError() for e2e tests #77103

Comments

oomichi commented Apr 26, 2019 • edited Loading

oomichi commented Apr 26, 2019

atoato88 commented Apr 26, 2019

k-toyoda-pi commented Apr 26, 2019

oomichi commented Apr 26, 2019

k-toyoda-pi commented Apr 26, 2019 • edited Loading

s-ito-ts commented Apr 26, 2019

draveness commented Apr 27, 2019 • edited Loading

oomichi commented May 8, 2019

draveness commented May 9, 2019 • edited Loading

draveness commented May 9, 2019 • edited Loading

oomichi commented May 9, 2019

oomichi commented May 9, 2019

oomichi commented May 9, 2019

draveness commented May 15, 2019 • edited Loading

oomichi commented May 15, 2019

draveness commented May 15, 2019

k-toyoda-pi commented May 15, 2019 • edited Loading

oomichi commented May 15, 2019

k-toyoda-pi commented May 17, 2019

draveness commented May 17, 2019

k-toyoda-pi commented May 17, 2019

s-ito-ts commented May 17, 2019

oomichi commented May 17, 2019

oomichi commented May 17, 2019

oomichi commented May 17, 2019

odinuge commented Jun 6, 2019

oomichi commented Apr 26, 2019 •

edited

Loading

k-toyoda-pi commented Apr 26, 2019 •

edited

Loading

draveness commented Apr 27, 2019 •

edited

Loading

draveness commented May 9, 2019 •

edited

Loading

draveness commented May 9, 2019 •

edited

Loading

draveness commented May 15, 2019 •

edited

Loading

k-toyoda-pi commented May 15, 2019 •

edited

Loading