From 80c554c5b02953b1ce0fbd048bed7c342c696210 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Sun, 23 Oct 2022 13:00:32 +0200 Subject: [PATCH] e2e framework: simplify failure and skip handling The original intention (adding more information for later analysis) is probably obsolete because there is no code which does anything with the extended error. The code in upgrade_suite.go collected it in an in-memory JUnit report, but then didn't do anything with that field. The code also wouldn't work for failures detected by Ginkgo itself, like the upcoming timeout handling. If the upgrade suite needs the information, it probably should get it from Gingko with a ReportAfterSuite call instead of depending in some fragile interception mechanism. --- .../e2e/cloud/gcp/common/upgrade_mechanics.go | 6 -- test/e2e/framework/log.go | 53 +---------- test/e2e/framework/skipper/skipper.go | 87 +----------------- test/e2e/framework/skipper/skipper_test.go | 88 +++++++++++++++++++ test/e2e/upgrades/upgrade_suite.go | 36 -------- 5 files changed, 94 insertions(+), 176 deletions(-) create mode 100644 test/e2e/framework/skipper/skipper_test.go diff --git a/test/e2e/cloud/gcp/common/upgrade_mechanics.go b/test/e2e/cloud/gcp/common/upgrade_mechanics.go index dc829cc48fe..93c1fb6ef8f 100644 --- a/test/e2e/cloud/gcp/common/upgrade_mechanics.go +++ b/test/e2e/cloud/gcp/common/upgrade_mechanics.go @@ -36,8 +36,6 @@ import ( // ControlPlaneUpgradeFunc returns a function that performs control plane upgrade. func ControlPlaneUpgradeFunc(f *framework.Framework, upgCtx *upgrades.UpgradeContext, testCase *junit.TestCase, controlPlaneExtraEnvs []string) func() { return func() { - start := time.Now() - defer upgrades.FinalizeUpgradeTest(start, testCase) target := upgCtx.Versions[1].Version.String() framework.ExpectNoError(controlPlaneUpgrade(f, target, controlPlaneExtraEnvs)) framework.ExpectNoError(checkControlPlaneVersion(f.ClientSet, target)) @@ -47,8 +45,6 @@ func ControlPlaneUpgradeFunc(f *framework.Framework, upgCtx *upgrades.UpgradeCon // ClusterUpgradeFunc returns a function that performs full cluster upgrade (both control plane and nodes). func ClusterUpgradeFunc(f *framework.Framework, upgCtx *upgrades.UpgradeContext, testCase *junit.TestCase, controlPlaneExtraEnvs, nodeExtraEnvs []string) func() { return func() { - start := time.Now() - defer upgrades.FinalizeUpgradeTest(start, testCase) target := upgCtx.Versions[1].Version.String() image := upgCtx.Versions[1].NodeImage framework.ExpectNoError(controlPlaneUpgrade(f, target, controlPlaneExtraEnvs)) @@ -61,8 +57,6 @@ func ClusterUpgradeFunc(f *framework.Framework, upgCtx *upgrades.UpgradeContext, // ClusterDowngradeFunc returns a function that performs full cluster downgrade (both nodes and control plane). func ClusterDowngradeFunc(f *framework.Framework, upgCtx *upgrades.UpgradeContext, testCase *junit.TestCase, controlPlaneExtraEnvs, nodeExtraEnvs []string) func() { return func() { - start := time.Now() - defer upgrades.FinalizeUpgradeTest(start, testCase) target := upgCtx.Versions[1].Version.String() image := upgCtx.Versions[1].NodeImage // Yes this really is a downgrade. And nodes must downgrade first. diff --git a/test/e2e/framework/log.go b/test/e2e/framework/log.go index 875df20fcec..e41c8329b15 100644 --- a/test/e2e/framework/log.go +++ b/test/e2e/framework/log.go @@ -20,7 +20,6 @@ import ( "bytes" "fmt" "regexp" - "runtime" "runtime/debug" "time" @@ -47,7 +46,7 @@ func Failf(format string, args ...interface{}) { msg := fmt.Sprintf(format, args...) skip := 1 log("FAIL", "%s\n\nFull Stack Trace\n%s", msg, PrunedStack(skip)) - fail(nowStamp()+": "+msg, skip) + ginkgo.Fail(nowStamp()+": "+msg, skip) panic("unreachable") } @@ -59,55 +58,7 @@ func Fail(msg string, callerSkip ...int) { skip += callerSkip[0] } log("FAIL", "%s\n\nFull Stack Trace\n%s", msg, PrunedStack(skip)) - fail(nowStamp()+": "+msg, skip) -} - -// FailurePanic is the value that will be panicked from Fail. -type FailurePanic struct { - Message string // The failure message passed to Fail - Filename string // The filename that is the source of the failure - Line int // The line number of the filename that is the source of the failure - FullStackTrace string // A full stack trace starting at the source of the failure -} - -const ginkgoFailurePanic = ` -Your test failed. -Ginkgo panics to prevent subsequent assertions from running. -Normally Ginkgo rescues this panic so you shouldn't see it. -But, if you make an assertion in a goroutine, Ginkgo can't capture the panic. -To circumvent this, you should call - defer GinkgoRecover() -at the top of the goroutine that caused this panic. -` - -// String makes FailurePanic look like the old Ginkgo panic when printed. -func (FailurePanic) String() string { return ginkgoFailurePanic } - -// fail wraps ginkgo.Fail so that it panics with more useful -// information about the failure. This function will panic with a -// FailurePanic. -func fail(message string, callerSkip ...int) { - skip := 1 - if len(callerSkip) > 0 { - skip += callerSkip[0] - } - - _, file, line, _ := runtime.Caller(skip) - fp := FailurePanic{ - Message: message, - Filename: file, - Line: line, - FullStackTrace: string(PrunedStack(skip)), - } - - defer func() { - e := recover() - if e != nil { - panic(fp) - } - }() - - ginkgo.Fail(message, skip) + ginkgo.Fail(nowStamp()+": "+msg, skip) } var codeFilterRE = regexp.MustCompile(`/github.com/onsi/ginkgo/v2/`) diff --git a/test/e2e/framework/skipper/skipper.go b/test/e2e/framework/skipper/skipper.go index 99d506e0a8f..abf6e3387a1 100644 --- a/test/e2e/framework/skipper/skipper.go +++ b/test/e2e/framework/skipper/skipper.go @@ -17,14 +17,8 @@ limitations under the License. package skipper import ( - "bufio" - "bytes" "context" "fmt" - "regexp" - "runtime" - "runtime/debug" - "strings" "github.com/onsi/ginkgo/v2" @@ -44,87 +38,14 @@ import ( func skipInternalf(caller int, format string, args ...interface{}) { msg := fmt.Sprintf(format, args...) + // Long term this should get replaced with https://github.com/onsi/ginkgo/issues/1069. framework.Logf(msg) - skip(msg, caller+1) -} - -// SkipPanic is the value that will be panicked from Skip. -type SkipPanic struct { - Message string // The failure message passed to Fail - Filename string // The filename that is the source of the failure - Line int // The line number of the filename that is the source of the failure - FullStackTrace string // A full stack trace starting at the source of the failure -} - -const ginkgoSkipPanic = ` -Your test was skipped. -Ginkgo panics to prevent subsequent assertions from running. -Normally Ginkgo rescues this panic so you shouldn't see it. -But, if you make an assertion in a goroutine, Ginkgo can't capture the panic. -To circumvent this, you should call - defer GinkgoRecover() -at the top of the goroutine that caused this panic. -` - -// String makes SkipPanic look like the old Ginkgo panic when printed. -func (SkipPanic) String() string { return ginkgoSkipPanic } - -// Skip wraps ginkgo.Skip so that it panics with more useful -// information about why the test is being skipped. This function will -// panic with a SkipPanic. -func skip(message string, callerSkip ...int) { - skip := 1 - if len(callerSkip) > 0 { - skip += callerSkip[0] - } - - _, file, line, _ := runtime.Caller(skip) - sp := SkipPanic{ - Message: message, - Filename: file, - Line: line, - FullStackTrace: pruneStack(skip), - } - - defer func() { - e := recover() - if e != nil { - panic(sp) - } - }() - - ginkgo.Skip(message, skip) -} - -// ginkgo adds a lot of test running infrastructure to the stack, so -// we filter those out -var stackSkipPattern = regexp.MustCompile(`onsi/ginkgo/v2`) - -func pruneStack(skip int) string { - skip += 2 // one for pruneStack and one for debug.Stack - stack := debug.Stack() - scanner := bufio.NewScanner(bytes.NewBuffer(stack)) - var prunedStack []string - - // skip the top of the stack - for i := 0; i < 2*skip+1; i++ { - scanner.Scan() - } - - for scanner.Scan() { - if stackSkipPattern.Match(scanner.Bytes()) { - scanner.Scan() // these come in pairs - } else { - prunedStack = append(prunedStack, scanner.Text()) - scanner.Scan() // these come in pairs - prunedStack = append(prunedStack, scanner.Text()) - } - } - - return strings.Join(prunedStack, "\n") + ginkgo.Skip(msg, caller+1) + panic("unreachable") } // Skipf skips with information about why the test is being skipped. +// The direct caller is recorded in the callstack. func Skipf(format string, args ...interface{}) { skipInternalf(1, format, args...) panic("unreachable") diff --git a/test/e2e/framework/skipper/skipper_test.go b/test/e2e/framework/skipper/skipper_test.go new file mode 100644 index 00000000000..c0c4cd44bcf --- /dev/null +++ b/test/e2e/framework/skipper/skipper_test.go @@ -0,0 +1,88 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package skipper_test + +import ( + "flag" + "testing" + + "github.com/onsi/ginkgo/v2" + + "k8s.io/kubernetes/test/e2e/framework" + "k8s.io/kubernetes/test/e2e/framework/internal/output" + e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper" +) + +// The line number of the following code is checked in TestFailureOutput below. +// Be careful when moving it around or changing the import statements above. +// Here are some intentionally blank lines that can be removed to compensate +// for future additional import statements. +// +// +// +// +// +// +// +// +// +// +// +// +// +// +// +// +// This must be line #50. + +var _ = ginkgo.Describe("e2e", func() { + ginkgo.It("skips", func() { + e2eskipper.Skipf("skipping %d, %d, %d", 1, 3, 4) + }) +}) + +func TestSkip(t *testing.T) { + // This simulates how test/e2e uses the framework and how users + // invoke test/e2e. + framework.RegisterCommonFlags(flag.CommandLine) + framework.RegisterClusterFlags(flag.CommandLine) + for flagname, value := range map[string]string{ + // This simplifies the text comparison. + "ginkgo.no-color": "true", + } { + if err := flag.Set(flagname, value); err != nil { + t.Fatalf("set %s: %v", flagname, err) + } + } + framework.AfterReadingAllFlags(&framework.TestContext) + suiteConfig, reporterConfig := framework.CreateGinkgoConfig() + + expected := output.SuiteResults{ + output.TestResult{ + Name: "e2e skips", + Output: `[It] skips + skipper_test.go:53 +INFO: skipping 1, 3, 4 +`, + Failure: `skipping 1, 3, 4`, + Stack: `k8s.io/kubernetes/test/e2e/framework/skipper_test.glob..func1.1() + skipper_test.go:54`, + }, + } + + output.TestGinkgoOutput(t, expected, suiteConfig, reporterConfig) +} diff --git a/test/e2e/upgrades/upgrade_suite.go b/test/e2e/upgrades/upgrade_suite.go index 149a242baf6..58b2eeb3cc7 100644 --- a/test/e2e/upgrades/upgrade_suite.go +++ b/test/e2e/upgrades/upgrade_suite.go @@ -28,7 +28,6 @@ import ( "k8s.io/kubernetes/test/e2e/chaosmonkey" "k8s.io/kubernetes/test/e2e/framework" - e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper" "k8s.io/kubernetes/test/utils/junit" admissionapi "k8s.io/pod-security-admission/api" @@ -37,25 +36,21 @@ import ( type chaosMonkeyAdapter struct { test Test - testReport *junit.TestCase framework *framework.Framework upgradeType UpgradeType upgCtx UpgradeContext } func (cma *chaosMonkeyAdapter) Test(sem *chaosmonkey.Semaphore) { - start := time.Now() var once sync.Once ready := func() { once.Do(func() { sem.Ready() }) } - defer FinalizeUpgradeTest(start, cma.testReport) defer ready() if skippable, ok := cma.test.(Skippable); ok && skippable.Skip(cma.upgCtx) { ginkgo.By("skipping test " + cma.test.Name()) - cma.testReport.Skipped = "skipping test " + cma.test.Name() return } @@ -65,36 +60,6 @@ func (cma *chaosMonkeyAdapter) Test(sem *chaosmonkey.Semaphore) { cma.test.Test(cma.framework, sem.StopCh, cma.upgradeType) } -// FinalizeUpgradeTest fills the necessary information about junit.TestCase. -func FinalizeUpgradeTest(start time.Time, tc *junit.TestCase) { - tc.Time = time.Since(start).Seconds() - r := recover() - if r == nil { - return - } - - switch r := r.(type) { - case framework.FailurePanic: - tc.Failures = []*junit.Failure{ - { - Message: r.Message, - Type: "Failure", - Value: fmt.Sprintf("%s\n\n%s", r.Message, r.FullStackTrace), - }, - } - case e2eskipper.SkipPanic: - tc.Skipped = fmt.Sprintf("%s:%d %q", r.Filename, r.Line, r.Message) - default: - tc.Errors = []*junit.Error{ - { - Message: fmt.Sprintf("%v", r), - Type: "Panic", - Value: fmt.Sprintf("%v", r), - }, - } - } -} - func CreateUpgradeFrameworks(tests []Test) map[string]*framework.Framework { nsFilter := regexp.MustCompile("[^[:word:]-]+") // match anything that's not a word character or hyphen testFrameworks := map[string]*framework.Framework{} @@ -126,7 +91,6 @@ func RunUpgradeSuite( testSuite.TestCases = append(testSuite.TestCases, testCase) cma := chaosMonkeyAdapter{ test: t, - testReport: testCase, framework: testFrameworks[t.Name()], upgradeType: upgradeType, upgCtx: *upgCtx,