Changed code to improve output for files under test/e2e/framework #105939

NikhilSharmaWe · 2021-10-27T16:39:18Z

What type of PR is this?

Enhancement

What this PR does / why we need it:

For better (more informative) output for developers when test fails. Changed files are under test/e2e/framework for this PR.

Which issue(s) this PR fixes:

Part of #105678

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-10-27T16:39:27Z

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

NikhilSharmaWe · 2021-10-27T16:41:22Z

$ git grep -n 'Expect.*Equal([^,]*, *$true\|false$' -- test/e2e/framework test/e2e/framework/log_test.go:60: framework.ExpectEqual(false, true, "false is never true") test/e2e/framework/log_test.go:71: framework.ExpectEqual(true, false, "true is never false either") test/e2e/framework/util.go:1380: ExpectEqual(len(expectedWatchEvents) <= len(actualWatchEvents), true, "Error: actual watch events amount (%d) must be greater than or equal to expected watch events amount (%d)", len(actualWatchEvents), len(expectedWatchEvents)) test/e2e/framework/util.go:1409: ExpectEqual(errs.Len() > 0, false, strings.Join(errs.List(), "\n - "))
According to me the rest of the cases are giving output which does not need the be changed. If they need change please inform.

oomichi · 2021-10-27T22:20:48Z

/triage accepted
/ok-to-test

oomichi · 2021-10-27T22:21:04Z

/kind cleanup

NikhilSharmaWe · 2021-10-28T04:26:22Z

@oomichi and @pohly how to proceed, what can be the reason of the test failing.

pohly · 2021-10-28T05:53:04Z

According to me the rest of the cases are giving output which does not need the be changed. If they need change please inform.

While some of those assertions have a reasonable message text, the assertion output itself is still going to be fairly useless. Therefore it makes sense to convert these:

test/e2e/framework/util.go:1380: ExpectEqual(len(expectedWatchEvents) <= len(actualWatchEvents), true, "Error: actual watch events amount (%d) must be greater than or equal to expected watch events amount (%d)", len(actualWatchEvents), len(expectedWatchEvents))
test/e2e/framework/util.go:1409: ExpectEqual(errs.Len() > 0, false, strings.Join(errs.List(), "\n - "))

log_test.go can be ignored, that isn't a real test.

how to proceed, what can be the reason of the test failing.

You can find that out by following the "Details" link for a failed job. "verify: gofmt" failed because code formatting isn't quite right. "verify: govet" failed because of an invalid Failf format specifier.

The integration test failure looks like an unrelated test flake.

NikhilSharmaWe · 2021-10-28T07:49:14Z

What should be the correction here, what should be written in Failf func in this case.

https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/util.go#L1409

pohly · 2021-10-30T22:06:15Z

test/e2e/framework/util.go

@@ -1377,7 +1378,9 @@ retriesLoop:
 		// NOTE the test may need access to the events to see what's going on, such as a change in status
 		actualWatchEvents := scenario(resourceWatch)
 		errs := sets.NewString()
-		ExpectEqual(len(expectedWatchEvents) <= len(actualWatchEvents), true, "Error: actual watch events amount (%d) must be greater than or equal to expected watch events amount (%d)", len(actualWatchEvents), len(expectedWatchEvents))
+		if len(expectedWatchEvents) > len(actualWatchEvents) {
+			framework.Failf("Error: actual watch events amount", len(actualWatchEvents), "must be greater than or equal to expected watch events", len(expectedWatchEvents))


Inside the "framework" package itself, Failf can be invoked without the framework. prefix.

pohly · 2021-10-30T22:07:13Z

test/e2e/framework/util.go

@@ -1377,7 +1378,9 @@ retriesLoop:
 		// NOTE the test may need access to the events to see what's going on, such as a change in status
 		actualWatchEvents := scenario(resourceWatch)
 		errs := sets.NewString()
-		ExpectEqual(len(expectedWatchEvents) <= len(actualWatchEvents), true, "Error: actual watch events amount (%d) must be greater than or equal to expected watch events amount (%d)", len(actualWatchEvents), len(expectedWatchEvents))
+		if len(expectedWatchEvents) > len(actualWatchEvents) {


This is a case for Expect(len(expectedWatchEvents)).To(BeNumerically(">", len(actualWatchEvents), ...)

Removed framework. prefix

The entire if Failf can be replaced with:

Expect(len(expectedWatchEvents)).To(BeNumerically("<=", len(actualWatchEvents), "Did not get enough watch events")

NikhilSharmaWe · 2021-10-31T00:55:10Z

@pohly and @oomichi One of the test is saying that
k8s.io/kubernetes/test/e2e/framework test/e2e/framework/util.go:1381:9: Failf call has arguments but no formatting directives k8s.io/kubernetes/test/e2e/framework/providers/kubemark test/e2e/framework/providers/kubemark/kubemark.go:79:19: Failf call has arguments but no formatting directives
so, should I add %v format directive in the Failf function.

NikhilSharmaWe · 2021-10-31T05:10:02Z

Will this output will be fine
if !errs.Len() > 0 { Failf("Error : %v", strings.Join(errs.List(), "\n - ")) }
at place of
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/util.go#L1409

NikhilSharmaWe · 2021-11-01T08:20:18Z

@pohly and @oomichi if you can confirm that the output for all cases are fine, then I can convert the changes to a single commit.

pohly · 2021-11-03T06:59:42Z

test/e2e/framework/providers/kubemark/kubemark.go

@@ -75,7 +75,9 @@ func (p *Provider) FrameworkBeforeEach(f *framework.Framework) {
 		p.controller, err = kubemark.NewKubemarkController(externalClient, externalInformerFactory, f.ClientSet, kubemarkNodeInformer)
 		framework.ExpectNoError(err)
 		externalInformerFactory.Start(p.closeChannel)
-		framework.ExpectEqual(p.controller.WaitForCacheSync(p.closeChannel), true)
+		if !p.controller.WaitForCacheSync(p.closeChannel) {
+			framework.Failf("Unable to syn caches for %v", p.controller)


Suggested change

framework.Failf("Unable to syn caches for %v", p.controller)

framework.Failf("Unable to sync caches for %v", p.controller)

pohly · 2021-11-03T07:03:18Z

test/e2e/framework/util.go

@@ -1377,7 +1378,9 @@ retriesLoop:
 		// NOTE the test may need access to the events to see what's going on, such as a change in status
 		actualWatchEvents := scenario(resourceWatch)
 		errs := sets.NewString()
-		ExpectEqual(len(expectedWatchEvents) <= len(actualWatchEvents), true, "Error: actual watch events amount (%d) must be greater than or equal to expected watch events amount (%d)", len(actualWatchEvents), len(expectedWatchEvents))
+		if len(expectedWatchEvents) > len(actualWatchEvents) {


The entire if Failf can be replaced with:

Expect(len(expectedWatchEvents)).To(BeNumerically("<=", len(actualWatchEvents), "Did not get enough watch events")

pohly · 2021-11-03T07:04:26Z

test/e2e/framework/util.go

@@ -1406,7 +1408,9 @@ retriesLoop:
 			fmt.Println("invariants violated:\n", strings.Join(errs.List(), "\n - "))
 			continue retriesLoop
 		}
-		ExpectEqual(errs.Len() > 0, false, strings.Join(errs.List(), "\n - "))
+		if errs.Len() > 0 {
+			Failf("Error: %v", strings.Join(errs.List(), "\n - "))


Suggested change

Failf("Error: %v", strings.Join(errs.List(), "\n - "))

Failf("Unexpected error(s): %v", strings.Join(errs.List(), "\n - "))

pohly · 2021-11-03T07:06:10Z

then I can convert the changes to a single commit

Feel free to do that right away before pushing an update. At least I prefer it that way (avoids one roundtrip when the result is okay).

pohly

/lgtm
/approve

k8s-ci-robot · 2021-11-03T14:04:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: NikhilSharmaWe, pohly

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:

~~test/e2e/framework/OWNERS~~ [pohly]

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

NikhilSharmaWe · 2021-11-09T19:13:38Z

@oomichi could you please sponsor my application for becoming a member of Kubernetes organization.

oomichi · 2021-11-09T22:20:03Z

@oomichi could you please sponsor my application for becoming a member of Kubernetes organization.

According to https://k8s.devstats.cncf.io/d/13/developer-activity-counts-by-repository-group?orgId=1[…]var-country_name=All&var-repo_name=kubernetes%2Fkubernetes
you already have good contributions on this develop cycle.
I am happy to support you, please let me know if you open an issue for the organization membership.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 27, 2021

k8s-ci-robot requested review from neolit123 and pohly October 27, 2021 16:40

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 27, 2021

NikhilSharmaWe force-pushed the betterOutputFramework branch from 455d3c2 to 6c739fb Compare October 28, 2021 03:38

pohly mentioned this pull request Oct 28, 2021

replace ExpectEqual(someBoolean, ...) #105678

Open

22 tasks

NikhilSharmaWe force-pushed the betterOutputFramework branch from 1a08111 to 2d81f09 Compare October 28, 2021 08:06

pohly reviewed Oct 30, 2021

View reviewed changes

NikhilSharmaWe force-pushed the betterOutputFramework branch 3 times, most recently from 9537fa7 to e52e671 Compare October 30, 2021 22:48

NikhilSharmaWe requested review from pohly and oomichi October 31, 2021 05:13

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 31, 2021

pohly suggested changes Nov 3, 2021

View reviewed changes

NikhilSharmaWe force-pushed the betterOutputFramework branch 3 times, most recently from 4c8c0f3 to c2809f4 Compare November 3, 2021 11:44

Changed code to improve output for files under test/e2e/framework

2e18992

NikhilSharmaWe force-pushed the betterOutputFramework branch from c2809f4 to 2e18992 Compare November 3, 2021 11:59

NikhilSharmaWe requested a review from pohly November 3, 2021 12:55

pohly approved these changes Nov 3, 2021

View reviewed changes

k8s-ci-robot assigned pohly Nov 3, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2021

k8s-ci-robot merged commit 16c8640 into kubernetes:master Nov 3, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 3, 2021

NikhilSharmaWe mentioned this pull request Nov 10, 2021

REQUEST: New membership for <NikhilSharmaWe> kubernetes/org#3088

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed code to improve output for files under test/e2e/framework #105939

Changed code to improve output for files under test/e2e/framework #105939

NikhilSharmaWe commented Oct 27, 2021

k8s-ci-robot commented Oct 27, 2021

NikhilSharmaWe commented Oct 27, 2021

oomichi commented Oct 27, 2021

oomichi commented Oct 27, 2021

NikhilSharmaWe commented Oct 28, 2021 •

edited

pohly commented Oct 28, 2021

NikhilSharmaWe commented Oct 28, 2021 •

edited

pohly Oct 30, 2021

pohly Oct 30, 2021

NikhilSharmaWe Oct 31, 2021

pohly Nov 3, 2021

NikhilSharmaWe commented Oct 31, 2021

NikhilSharmaWe commented Oct 31, 2021 •

edited

NikhilSharmaWe commented Nov 1, 2021

pohly Nov 3, 2021

pohly Nov 3, 2021

pohly Nov 3, 2021

pohly commented Nov 3, 2021

pohly left a comment

k8s-ci-robot commented Nov 3, 2021

NikhilSharmaWe commented Nov 9, 2021

oomichi commented Nov 9, 2021

	framework.Failf("Unable to syn caches for %v", p.controller)
	framework.Failf("Unable to sync caches for %v", p.controller)

	Failf("Error: %v", strings.Join(errs.List(), "\n - "))
	Failf("Unexpected error(s): %v", strings.Join(errs.List(), "\n - "))

Changed code to improve output for files under test/e2e/framework #105939

Changed code to improve output for files under test/e2e/framework #105939

Conversation

NikhilSharmaWe commented Oct 27, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 27, 2021

NikhilSharmaWe commented Oct 27, 2021

oomichi commented Oct 27, 2021

oomichi commented Oct 27, 2021

NikhilSharmaWe commented Oct 28, 2021 • edited

pohly commented Oct 28, 2021

NikhilSharmaWe commented Oct 28, 2021 • edited

pohly Oct 30, 2021

Choose a reason for hiding this comment

pohly Oct 30, 2021

Choose a reason for hiding this comment

NikhilSharmaWe Oct 31, 2021

Choose a reason for hiding this comment

pohly Nov 3, 2021

Choose a reason for hiding this comment

NikhilSharmaWe commented Oct 31, 2021

NikhilSharmaWe commented Oct 31, 2021 • edited

NikhilSharmaWe commented Nov 1, 2021

pohly Nov 3, 2021

Choose a reason for hiding this comment

pohly Nov 3, 2021

Choose a reason for hiding this comment

pohly Nov 3, 2021

Choose a reason for hiding this comment

pohly commented Nov 3, 2021

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 3, 2021

NikhilSharmaWe commented Nov 9, 2021

oomichi commented Nov 9, 2021

NikhilSharmaWe commented Oct 28, 2021 •

edited

NikhilSharmaWe commented Oct 28, 2021 •

edited

NikhilSharmaWe commented Oct 31, 2021 •

edited