From 181fc50f8e13a2328a8311de0ab2c6c99389c11f Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 22 Feb 2023 11:00:24 +0100 Subject: [PATCH] e2e framework: deprecate gomega wrappers All wrappers except for ExpectNoError are identical to their gomega counterparts. The only advantage that they have is that their invocations are shorter. That advantage does not outweigh their disadvantages: - cannot be used in combination with gomega.Eventually/Consistently - not a full replacement for gomega, so we just end up using both - don't support passing a stack offset and thus cannot be used in helper functions - ginkgolinter does not work for them, so sub-optimal calls like this one are not reported: framework.ExpectEqual(len(items), 0) -> gomega.Expect(items).To(gomega.BeEmpty()) - developers try to make do with what's available in the framework, leading to sub-optimal checks like this: framework.ExpectEqual(true, strings.Contains(event.Message, expectedEventError), "Event error should indicate non-root policy caused container to not start") -> gomega.Expect(event.Message).To(gomega.ContainSubstring(expectedEventError), "Event error should indicate non-root policy caused container to not start") So let's remove these wrappers. As a first step they get marked as deprecated. This enables stricter linting (https://github.com/kubernetes/kubernetes/pull/109728), once enabled, to report new code which uses them. --- hack/verify-test-code.sh | 69 ------------------------------------ test/e2e/framework/expect.go | 14 ++++++++ 2 files changed, 14 insertions(+), 69 deletions(-) diff --git a/hack/verify-test-code.sh b/hack/verify-test-code.sh index d83983e3107..46737628643 100755 --- a/hack/verify-test-code.sh +++ b/hack/verify-test-code.sh @@ -41,33 +41,6 @@ do fi done -errors_expect_error=() -for file in "${all_e2e_files[@]}" -do - if grep "Expect(.*)\.To(.*HaveOccurred()" "${file}" > /dev/null - then - errors_expect_error+=( "${file}" ) - fi -done - -errors_expect_no_equal=() -for file in "${all_e2e_files[@]}" -do - if grep -E "Expect\(.*\)\.(NotTo|ToNot)\((gomega\.Equal|Equal)" "${file}" > /dev/null - then - errors_expect_no_equal+=( "${file}" ) - fi -done - -errors_expect_equal=() -for file in "${all_e2e_files[@]}" -do - if grep -E "Expect\(.*\)\.To\((gomega\.Equal|Equal)" "${file}" > /dev/null - then - errors_expect_equal+=( "${file}" ) - fi -done - all_e2e_framework_files=() kube::util::read-array all_e2e_framework_files < <(find test/e2e/framework/ -name '*.go' | grep -v "_test.go") errors_framework_contains_tests=() @@ -93,48 +66,6 @@ if [ ${#errors_expect_no_error[@]} -ne 0 ]; then exit 1 fi -if [ ${#errors_expect_error[@]} -ne 0 ]; then - { - echo "Errors:" - for err in "${errors_expect_error[@]}"; do - echo "$err" - done - echo - echo 'The above files need to use framework.ExpectError(err) instead of ' - echo 'Expect(err).To(HaveOccurred()) or gomega.Expect(err).To(gomega.HaveOccurred())' - echo - } >&2 - exit 1 -fi - -if [ ${#errors_expect_no_equal[@]} -ne 0 ]; then - { - echo "Errors:" - for err in "${errors_expect_no_equal[@]}"; do - echo "$err" - done - echo - echo 'The above files need to use framework.ExpectNotEqual(foo, bar) instead of ' - echo 'Expect(foo).NotTo(Equal(bar)) or gomega.Expect(foo).NotTo(gomega.Equal(bar))' - echo - } >&2 - exit 1 -fi - -if [ ${#errors_expect_equal[@]} -ne 0 ]; then - { - echo "Errors:" - for err in "${errors_expect_equal[@]}"; do - echo "$err" - done - echo - echo 'The above files need to use framework.ExpectEqual(foo, bar) instead of ' - echo 'Expect(foo).To(Equal(bar)) or gomega.Expect(foo).To(gomega.Equal(bar))' - echo - } >&2 - exit 1 -fi - if [ ${#errors_framework_contains_tests[@]} -ne 0 ]; then { echo "Errors:" diff --git a/test/e2e/framework/expect.go b/test/e2e/framework/expect.go index da0cde08df7..7908cdcf377 100644 --- a/test/e2e/framework/expect.go +++ b/test/e2e/framework/expect.go @@ -293,16 +293,24 @@ func (f *FailureError) backtrace() { var ErrFailure error = FailureError{} // ExpectEqual expects the specified two are the same, otherwise an exception raises +// +// Deprecated: use gomega.Expect().To(gomega.BeEqual()) func ExpectEqual(actual interface{}, extra interface{}, explain ...interface{}) { gomega.ExpectWithOffset(1, actual).To(gomega.Equal(extra), explain...) } // ExpectNotEqual expects the specified two are not the same, otherwise an exception raises +// +// Deprecated: use gomega.Expect().ToNot(gomega.BeEqual()) func ExpectNotEqual(actual interface{}, extra interface{}, explain ...interface{}) { gomega.ExpectWithOffset(1, actual).NotTo(gomega.Equal(extra), explain...) } // ExpectError expects an error happens, otherwise an exception raises +// +// Deprecated: use gomega.Expect().To(gomega.HaveOccurred()) or (better!) check +// specifically for the error that is expected with +// gomega.Expect().To(gomega.MatchError(gomega.ContainSubstring())) func ExpectError(err error, explain ...interface{}) { gomega.ExpectWithOffset(1, err).To(gomega.HaveOccurred(), explain...) } @@ -356,16 +364,22 @@ func ExpectNoErrorWithOffset(offset int, err error, explain ...interface{}) { } // ExpectConsistOf expects actual contains precisely the extra elements. The ordering of the elements does not matter. +// +// Deprecated: use gomega.Expect().To(gomega.ConsistOf()) instead func ExpectConsistOf(actual interface{}, extra interface{}, explain ...interface{}) { gomega.ExpectWithOffset(1, actual).To(gomega.ConsistOf(extra), explain...) } // ExpectHaveKey expects the actual map has the key in the keyset +// +// Deprecated: use gomega.Expect().To(gomega.HaveKey()) instead func ExpectHaveKey(actual interface{}, key interface{}, explain ...interface{}) { gomega.ExpectWithOffset(1, actual).To(gomega.HaveKey(key), explain...) } // ExpectEmpty expects actual is empty +// +// Deprecated: use gomega.Expect().To(gomega.BeEmpty()) instead func ExpectEmpty(actual interface{}, explain ...interface{}) { gomega.ExpectWithOffset(1, actual).To(gomega.BeEmpty(), explain...) }