diff --git a/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go b/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go index c29c3c8e99b6..5a5af126494f 100644 --- a/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go +++ b/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go @@ -187,7 +187,7 @@ func (tr *testRunner) run() (retErr error) { if event.Err == nil { tr.logger.Printf("background step finished: %s", event.Name) continue - } else if event.ExpectedCancel { + } else if event.TriggeredByTest { tr.logger.Printf("background step canceled by test: %s", event.Name) continue } diff --git a/pkg/cmd/roachtest/roachtestutil/task/manager.go b/pkg/cmd/roachtest/roachtestutil/task/manager.go index d15d783d5e37..de4bbd0e3c37 100644 --- a/pkg/cmd/roachtest/roachtestutil/task/manager.go +++ b/pkg/cmd/roachtest/roachtestutil/task/manager.go @@ -28,9 +28,9 @@ type ( // Event represents the result of a task execution. Event struct { - Name string - Err error - ExpectedCancel bool + Name string + Err error + TriggeredByTest bool } manager struct { @@ -59,7 +59,7 @@ func NewManager(ctx context.Context, l *logger.Logger) Manager { func (m *manager) defaultOptions() []Option { // The default panic handler simply returns the panic as an error. defaultPanicHandlerFn := func(_ context.Context, name string, l *logger.Logger, r interface{}) error { - return r.(error) + return fmt.Errorf("panic: %v", r) } // The default error handler simply returns the error as is. defaultErrorHandlerFn := func(_ context.Context, name string, l *logger.Logger, err error) error { @@ -98,17 +98,26 @@ func (m *manager) GoWithCancel(fn Func, opts ...Option) context.CancelFunc { } err = internalFunc(l) event := Event{ - Name: opt.Name, - Err: err, - ExpectedCancel: err != nil && IsContextCanceled(groupCtx) && expectedContextCancellation.Load(), + Name: opt.Name, + Err: err, + // TriggeredByTest is set to true if the task was canceled intentionally, + // by the test, and we encounter an error. The assumption is that we + // expect the error to have been caused by the cancelation, hence the + // error above was not caused by a failure. This ensures we don't register + // a test failure if the task was meant to be canceled. It's possible that + // `expectedContextCancellation` could be set before the context is + // canceled, thus we also ensure that the context is canceled. + TriggeredByTest: err != nil && IsContextCanceled(groupCtx) && expectedContextCancellation.Load(), } - select { - case m.events <- event: - // exit goroutine - case <-m.ctx.Done(): - // Parent context already finished, exit goroutine. + + // Do not send the event if the parent context is canceled. The test is + // already aware of the cancelation and sending an event would be redundant. + // For instance, a call to test.Fatal would already have captured the error + // and canceled the context. + if IsContextCanceled(m.ctx) { return nil } + m.events <- event return err }) @@ -129,7 +138,9 @@ func (m *manager) Go(fn Func, opts ...Option) { } // Terminate will call the stop functions for every task started during the -// test. Returns when all task functions have returned. +// test. Returns when all task functions have returned, or after a 5-minute +// timeout, whichever comes first. If the timeout is reached, the function logs +// a warning message and returns. func (m *manager) Terminate(l *logger.Logger) { func() { m.mu.Lock() diff --git a/pkg/cmd/roachtest/test_impl.go b/pkg/cmd/roachtest/test_impl.go index 0fade5bfa0d0..a58d4b726778 100644 --- a/pkg/cmd/roachtest/test_impl.go +++ b/pkg/cmd/roachtest/test_impl.go @@ -12,7 +12,6 @@ import ( "math/rand" "os" "regexp" - "runtime/debug" "strings" "sync" "time" @@ -654,8 +653,7 @@ func (t *testImpl) IsBuildVersion(minVersion string) bool { } func panicHandler(_ context.Context, name string, l *logger.Logger, r interface{}) error { - l.Printf("panic stack trace in task %s:\n%s", name, string(debug.Stack())) - return fmt.Errorf("panic (stack trace above): %v", r) + return fmt.Errorf("test task %s panicked: %v", name, r) } // GoWithCancel runs the given function in a goroutine and returns a diff --git a/pkg/cmd/roachtest/test_runner.go b/pkg/cmd/roachtest/test_runner.go index 69d43859c58a..a8bf62bf5b57 100644 --- a/pkg/cmd/roachtest/test_runner.go +++ b/pkg/cmd/roachtest/test_runner.go @@ -1299,7 +1299,7 @@ func (r *testRunner) runTest( if event.Err == nil { t.L().Printf("task finished: %s", event.Name) continue - } else if event.ExpectedCancel { + } else if event.TriggeredByTest { t.L().Printf("task canceled by test: %s", event.Name) continue }