diff --git a/pkg/cmd/roachtest/roachtestutil/mixedversion/BUILD.bazel b/pkg/cmd/roachtest/roachtestutil/mixedversion/BUILD.bazel index 31346f06cf84..a91372831bea 100644 --- a/pkg/cmd/roachtest/roachtestutil/mixedversion/BUILD.bazel +++ b/pkg/cmd/roachtest/roachtestutil/mixedversion/BUILD.bazel @@ -22,13 +22,15 @@ go_library( "//pkg/util/syncutil", "//pkg/util/timeutil", "//pkg/util/version", - "@com_github_cockroachdb_errors//:errors", ], ) go_test( name = "mixedversion_test", - srcs = ["planner_test.go"], + srcs = [ + "planner_test.go", + "runner_test.go", + ], args = ["-test.timeout=295s"], embed = [":mixedversion"], deps = [ diff --git a/pkg/cmd/roachtest/roachtestutil/mixedversion/planner_test.go b/pkg/cmd/roachtest/roachtestutil/mixedversion/planner_test.go index 56833b4f84e2..5ed1b42346f3 100644 --- a/pkg/cmd/roachtest/roachtestutil/mixedversion/planner_test.go +++ b/pkg/cmd/roachtest/roachtestutil/mixedversion/planner_test.go @@ -32,7 +32,7 @@ var ( Stdout: io.Discard, Stderr: io.Discard, } - l, err := cfg.NewLogger("" /* path */) + l, err := cfg.NewLogger("/dev/null" /* path */) if err != nil { panic(err) } diff --git a/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go b/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go index a82b2e1e74f9..f108de6055a0 100644 --- a/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go +++ b/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go @@ -19,6 +19,7 @@ import ( "path" "path/filepath" "regexp" + "runtime/debug" "strconv" "strings" "sync/atomic" @@ -32,7 +33,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/ctxgroup" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" - "github.com/cockroachdb/errors" ) type ( @@ -249,7 +249,16 @@ func (tr *testRunner) runSingleStep(ctx context.Context, ss singleStep, l *logge prefix := fmt.Sprintf("FINISHED [%s]", timeutil.Since(start)) tr.logStep(prefix, ss, l) }() - if err := ss.Run(ctx, l, tr.cluster, tr.newHelper(ctx, l)); err != nil { + + if err := func() (retErr error) { + defer func() { + if r := recover(); r != nil { + l.Printf("panic stack trace:\n%s", string(debug.Stack())) + retErr = fmt.Errorf("panic (stack trace above): %v", r) + } + }() + return ss.Run(ctx, l, tr.cluster, tr.newHelper(ctx, l)) + }(); err != nil { if isContextCanceled(err) { l.Printf("step terminated (context canceled)") // Avoid creating a `stepError` (which involves querying binary @@ -331,7 +340,7 @@ func (tr *testRunner) testFailure(desc string, l *logger.Logger) error { tr.logger.Printf("could not rename failed step logger: %v", err) } - return errors.WithStack(tf) + return tf } func (tr *testRunner) logStep(prefix string, step singleStep, l *logger.Logger) { diff --git a/pkg/cmd/roachtest/roachtestutil/mixedversion/runner_test.go b/pkg/cmd/roachtest/roachtestutil/mixedversion/runner_test.go new file mode 100644 index 000000000000..62662f30c359 --- /dev/null +++ b/pkg/cmd/roachtest/roachtestutil/mixedversion/runner_test.go @@ -0,0 +1,85 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package mixedversion + +import ( + "context" + "fmt" + "testing" + + "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster" + "github.com/cockroachdb/cockroach/pkg/roachprod/logger" + "github.com/stretchr/testify/require" +) + +func Test_runSingleStep(t *testing.T) { + tr := testTestRunner() + + // steps that run without errors do not return errors + successStep := newTestStep(func() error { + return nil + }) + err := tr.runSingleStep(ctx, successStep, nilLogger) + require.NoError(t, err) + + // steps that return an error have that error surfaced + errorStep := newTestStep(func() error { + return fmt.Errorf("oops") + }) + err = tr.runSingleStep(ctx, errorStep, nilLogger) + require.Error(t, err) + require.Contains(t, err.Error(), "oops") + + // steps that panic cause an error to be returned + panicStep := newTestStep(func() error { + var ids []int + if ids[0] > 42 { + return nil + } + return fmt.Errorf("unreachable") + }) + err = nil + require.NotPanics(t, func() { + err = tr.runSingleStep(ctx, panicStep, nilLogger) + }) + require.Error(t, err) + require.Contains(t, err.Error(), "panic (stack trace above): runtime error: index out of range [0] with length 0") +} + +func testTestRunner() *testRunner { + runnerCtx, cancel := context.WithCancel(ctx) + return &testRunner{ + ctx: runnerCtx, + cancel: cancel, + logger: nilLogger, + crdbNodes: nodes, + background: newBackgroundRunner(runnerCtx), + seed: seed, + } +} + +type testSingleStep struct { + runFunc func() error +} + +func (testSingleStep) ID() int { return 42 } +func (testSingleStep) Description() string { return "testSingleStep" } +func (testSingleStep) Background() shouldStop { return nil } + +func (tss testSingleStep) Run( + _ context.Context, _ *logger.Logger, _ cluster.Cluster, _ *Helper, +) error { + return tss.runFunc() +} + +func newTestStep(f func() error) singleStep { + return testSingleStep{runFunc: f} +} diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index 9022c8ff7b48..0e9434dacde9 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/pkg/testutils/lint/lint_test.go @@ -2306,6 +2306,8 @@ func TestLint(t *testing.T) { // pooling, etc, then test code needs to adhere as well. stream.GrepNot(nakedGoroutineExceptions + `:.*Use of go keyword not allowed`), stream.GrepNot(nakedGoroutineExceptions + `:.*Illegal call to Group\.Go\(\)`), + // We purposefully dereference nil in this file to test panic handling + stream.GrepNot(`pkg/cmd/roachtest/roachtestutil/mixedversion/runner_test\.go:.*nil dereference`), } const vetTool = "roachvet"