From 228640ffabcdcf38e6a221589c6d1d9f0f765f6f Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 11 May 2022 10:47:19 +0200 Subject: [PATCH 1/4] e2e: fix logs unit test The test had two problems: - the expected line was off by one (probably modified import statements) - when Gomega failed in TestFailureOutput, Ginkgo panicked because its fail handler was called outside of a Ginkgo node Now github.com/stretchr/testify/assert is used for comparing the output because it works in a unit test without further customization and because the failure messages are more useful. --- test/e2e/framework/log_test.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/test/e2e/framework/log_test.go b/test/e2e/framework/log_test.go index 103c7748b87..d63c38bbcb0 100644 --- a/test/e2e/framework/log_test.go +++ b/test/e2e/framework/log_test.go @@ -27,6 +27,7 @@ import ( "github.com/onsi/ginkgo/config" "github.com/onsi/ginkgo/reporters" "github.com/onsi/gomega" + "github.com/stretchr/testify/assert" "k8s.io/kubernetes/test/e2e/framework" ) @@ -40,7 +41,6 @@ import ( // // // -// func runTests(t *testing.T, reporter ginkgo.Reporter) { // This source code line will be part of the stack dump comparison. @@ -115,17 +115,14 @@ func TestFailureOutput(t *testing.T) { stack: "k8s.io/kubernetes/test/e2e/framework_test.glob..func1.2()\n\tlog_test.go:57\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47\n", }, } - // Compare individual fields. Comparing the slices leads to unreadable error output when there is any mismatch. - framework.ExpectEqual(len(actual), len(expected), "%d entries in %v", len(expected), actual) - for i, a := range actual { - b := expected[i] - framework.ExpectEqual(a.name, b.name, "name in %d", i) - framework.ExpectEqual(a.output, b.output, "output in %d", i) - framework.ExpectEqual(a.failure, b.failure, "failure in %d", i) - // There may be additional stack entries from the "testing" package at the - // end. We ignore those in the comparison because the line number in them - // varies. - framework.ExpectEqual(a.stack, b.stack, "stack in %d: %s", i, a.stack) + // assert.Equal prints a useful diff if the slices are not + // equal. However, the diff does not show changes inside the + // strings. Therefore we also compare the individual fields. + if !assert.Equal(t, expected, actual) { + for i := 0; i < len(expected) && i < len(actual); i++ { + assert.Equal(t, expected[i].output, actual[i].output, "output from test #%d: %s", i, expected[i].name) + assert.Equal(t, expected[i].stack, actual[i].stack, "stack from test #%d: %s", i, expected[i].name) + } } } From 38348532a306611effcfad978db1a24672e6510e Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 11 May 2022 11:29:56 +0200 Subject: [PATCH 2/4] e2e: reformat log test Using multi-line strings makes the test source code easier to read. --- test/e2e/framework/log_test.go | 119 ++++++++++++++++++++++++++++----- 1 file changed, 103 insertions(+), 16 deletions(-) diff --git a/test/e2e/framework/log_test.go b/test/e2e/framework/log_test.go index d63c38bbcb0..5a3cee1b07f 100644 --- a/test/e2e/framework/log_test.go +++ b/test/e2e/framework/log_test.go @@ -86,33 +86,120 @@ func TestFailureOutput(t *testing.T) { actual := normalizeReport(*reporter) // output from AfterEach - commonOutput := "\n\nINFO: after\nFAIL: true is never false either\nExpected\n : true\nto equal\n : false\n\nFull Stack Trace\nk8s.io/kubernetes/test/e2e/framework_test.glob..func1.6()\n\tlog_test.go:71\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47\n\n" + commonOutput := ` + +INFO: after +FAIL: true is never false either +Expected + : true +to equal + : false + +Full Stack Trace +k8s.io/kubernetes/test/e2e/framework_test.glob..func1.6() + log_test.go:71 +k8s.io/kubernetes/test/e2e/framework_test.runTests() + log_test.go:47 + +` // Sorted by name! expected := suiteResults{ testResult{ - name: "[Top Level] log asserts", - output: "INFO: before\nFAIL: false is never true\nExpected\n : false\nto equal\n : true\n\nFull Stack Trace\nk8s.io/kubernetes/test/e2e/framework_test.glob..func1.3()\n\tlog_test.go:60\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47" + commonOutput, - failure: "false is never true\nExpected\n : false\nto equal\n : true", - stack: "k8s.io/kubernetes/test/e2e/framework_test.glob..func1.3()\n\tlog_test.go:60\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47\n", + name: "[Top Level] log asserts", + output: `INFO: before +FAIL: false is never true +Expected + : false +to equal + : true + +Full Stack Trace +k8s.io/kubernetes/test/e2e/framework_test.glob..func1.3() + log_test.go:60 +k8s.io/kubernetes/test/e2e/framework_test.runTests() + log_test.go:47` + commonOutput, + failure: `false is never true +Expected + : false +to equal + : true`, + stack: `k8s.io/kubernetes/test/e2e/framework_test.glob..func1.3() + log_test.go:60 +k8s.io/kubernetes/test/e2e/framework_test.runTests() + log_test.go:47 +`, }, testResult{ - name: "[Top Level] log equal", - output: "INFO: before\nFAIL: of course it's not equal...\nExpected\n : 0\nto equal\n : 1\n\nFull Stack Trace\nk8s.io/kubernetes/test/e2e/framework_test.glob..func1.5()\n\tlog_test.go:67\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47" + commonOutput, - failure: "of course it's not equal...\nExpected\n : 0\nto equal\n : 1", - stack: "k8s.io/kubernetes/test/e2e/framework_test.glob..func1.5()\n\tlog_test.go:67\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47\n", + name: "[Top Level] log equal", + output: `INFO: before +FAIL: of course it's not equal... +Expected + : 0 +to equal + : 1 + +Full Stack Trace +k8s.io/kubernetes/test/e2e/framework_test.glob..func1.5() + log_test.go:67 +k8s.io/kubernetes/test/e2e/framework_test.runTests() + log_test.go:47` + commonOutput, + failure: `of course it's not equal... +Expected + : 0 +to equal + : 1`, + stack: `k8s.io/kubernetes/test/e2e/framework_test.glob..func1.5() + log_test.go:67 +k8s.io/kubernetes/test/e2e/framework_test.runTests() + log_test.go:47 +`, }, testResult{ - name: "[Top Level] log error", - output: "INFO: before\nFAIL: hard-coded error\nUnexpected error:\n <*errors.errorString>: {\n s: \"an error with a long, useless description\",\n }\n an error with a long, useless description\noccurred\n\nFull Stack Trace\nk8s.io/kubernetes/test/e2e/framework_test.glob..func1.4()\n\tlog_test.go:64\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47" + commonOutput, - failure: "hard-coded error\nUnexpected error:\n <*errors.errorString>: {\n s: \"an error with a long, useless description\",\n }\n an error with a long, useless description\noccurred", - stack: "k8s.io/kubernetes/test/e2e/framework_test.glob..func1.4()\n\tlog_test.go:64\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47\n", + name: "[Top Level] log error", + output: `INFO: before +FAIL: hard-coded error +Unexpected error: + <*errors.errorString>: { + s: "an error with a long, useless description", + } + an error with a long, useless description +occurred + +Full Stack Trace +k8s.io/kubernetes/test/e2e/framework_test.glob..func1.4() + log_test.go:64 +k8s.io/kubernetes/test/e2e/framework_test.runTests() + log_test.go:47` + commonOutput, + failure: `hard-coded error +Unexpected error: + <*errors.errorString>: { + s: "an error with a long, useless description", + } + an error with a long, useless description +occurred`, + stack: `k8s.io/kubernetes/test/e2e/framework_test.glob..func1.4() + log_test.go:64 +k8s.io/kubernetes/test/e2e/framework_test.runTests() + log_test.go:47 +`, }, testResult{ - name: "[Top Level] log fails", - output: "INFO: before\nFAIL: I'm failing.\n\nFull Stack Trace\nk8s.io/kubernetes/test/e2e/framework_test.glob..func1.2()\n\tlog_test.go:57\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47" + commonOutput, + name: "[Top Level] log fails", + output: `INFO: before +FAIL: I'm failing. + +Full Stack Trace +k8s.io/kubernetes/test/e2e/framework_test.glob..func1.2() + log_test.go:57 +k8s.io/kubernetes/test/e2e/framework_test.runTests() + log_test.go:47` + commonOutput, failure: "I'm failing.", - stack: "k8s.io/kubernetes/test/e2e/framework_test.glob..func1.2()\n\tlog_test.go:57\nk8s.io/kubernetes/test/e2e/framework_test.runTests()\n\tlog_test.go:47\n", + stack: `k8s.io/kubernetes/test/e2e/framework_test.glob..func1.2() + log_test.go:57 +k8s.io/kubernetes/test/e2e/framework_test.runTests() + log_test.go:47 +`, }, } // assert.Equal prints a useful diff if the slices are not From e198c3a544f42952eca6d586175b2d841df3518c Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 11 May 2022 11:43:55 +0200 Subject: [PATCH 3/4] e2e: fix node wait test Because these tests don't run in the CI, the test did not quite match the actual code anymore. Apparently a retry mechanism was added after the test was written. --- test/e2e/framework/node/wait_test.go | 29 +++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/test/e2e/framework/node/wait_test.go b/test/e2e/framework/node/wait_test.go index 174d55016d6..3b3373de891 100644 --- a/test/e2e/framework/node/wait_test.go +++ b/test/e2e/framework/node/wait_test.go @@ -168,15 +168,26 @@ func TestCheckReadyForTests(t *testing.T) { return true, nodeList, tc.nodeListErr }) checkFunc := CheckReadyForTests(c, tc.nonblockingTaints, tc.allowedNotReadyNodes, testLargeClusterThreshold) - out, err := checkFunc() - if out != tc.expected { - t.Errorf("Expected %v but got %v", tc.expected, out) - } - switch { - case err == nil && len(tc.expectedErr) > 0: - t.Errorf("Expected error %q nil", tc.expectedErr) - case err != nil && err.Error() != tc.expectedErr: - t.Errorf("Expected error %q but got %q", tc.expectedErr, err.Error()) + // The check function returns "false, nil" during its + // first two calls, therefore we have to try several + // times until we get the expected error. + for attempt := 0; attempt <= 3; attempt++ { + out, err := checkFunc() + expected := tc.expected + expectedErr := tc.expectedErr + if tc.nodeListErr != nil && attempt < 2 { + expected = false + expectedErr = "" + } + if out != expected { + t.Errorf("Expected %v but got %v", expected, out) + } + switch { + case err == nil && expectedErr != "": + t.Errorf("attempt #%d: expected error %q nil", attempt, expectedErr) + case err != nil && err.Error() != expectedErr: + t.Errorf("attempt #%d: expected error %q but got %q", attempt, expectedErr, err.Error()) + } } }) } From 1aa58532c856b62453d47b54c0501810ff1372bd Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 11 May 2022 11:48:58 +0200 Subject: [PATCH 4/4] test: enable unit tests under test/e2e The test/e2e directory contains several unit tests that should run as part of "make test": ./test/e2e/chaosmonkey/chaosmonkey_test.go ./test/e2e/storage/external/external_test.go ./test/e2e/storage/utils/utils_test.go ./test/e2e/framework/log_test.go ./test/e2e/framework/testfiles/testfiles_test.go ./test/e2e/framework/timer/timer_test.go ./test/e2e/framework/node/wait_test.go ./test/e2e/framework/pod/resource_test.go ./test/e2e/framework/config/config_test.go ./test/e2e/framework/ingress/ingress_utils_test.go ./test/e2e/framework/providers/gce/firewall_test.go Because they were excluded by "./test/e2e/*", some of them became outdated. ./test/e2e/e2e_test.go is the only test that needs to be excluded because it is the E2E test suite that depends on a functional cluster. --- hack/make-rules/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hack/make-rules/test.sh b/hack/make-rules/test.sh index 33e8c311dc7..d58bc929752 100755 --- a/hack/make-rules/test.sh +++ b/hack/make-rules/test.sh @@ -44,7 +44,7 @@ kube::test::find_dirs() { -o -path './output/*' \ -o -path './release/*' \ -o -path './target/*' \ - -o -path './test/e2e/*' \ + -o -path './test/e2e/e2e_test.go' \ -o -path './test/e2e_node/*' \ -o -path './test/e2e_kubeadm/*' \ -o -path './test/integration/*' \