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 ae323a35d491..d09f81be2e07 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" @@ -679,8 +678,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 175c53478cbc..c0212db1e127 100644 --- a/pkg/cmd/roachtest/test_runner.go +++ b/pkg/cmd/roachtest/test_runner.go @@ -1316,7 +1316,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 } diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index 273cdd06e574..f2a2c7a3cc03 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/pkg/testutils/lint/lint_test.go @@ -198,6 +198,31 @@ func TestLint(t *testing.T) { t.Error(err) } + // Things that are package scoped are below here. + pkgScope := pkgVar + if !pkgSpecified { + pkgScope = "./pkg/..." + } + + // Load packages for top-level forbidden import tests. + pkgPath := filepath.Join(cockroachDB, pkgScope) + pkgs, err := packages.Load( + &packages.Config{ + Mode: packages.NeedImports | packages.NeedName, + Dir: crdbDir, + }, + pkgPath, + ) + if err != nil { + t.Fatal(errors.Wrapf(err, "error loading package %s", pkgPath)) + } + // NB: if no packages were found, this API confusingly + // returns no error, so we need to explicitly check that + // something was returned. + if len(pkgs) == 0 { + t.Fatalf("could not list packages under %s", pkgPath) + } + t.Run("TestLowercaseFunctionNames", func(t *testing.T) { skip.UnderShort(t) t.Parallel() @@ -1804,12 +1829,6 @@ func TestLint(t *testing.T) { } }) - // Things that are packaged scoped are below here. - pkgScope := pkgVar - if !pkgSpecified { - pkgScope = "./pkg/..." - } - t.Run("TestForbiddenImports", func(t *testing.T) { t.Parallel() @@ -1850,23 +1869,6 @@ func TestLint(t *testing.T) { grepBuf.WriteString(")$") filter := stream.FilterFunc(func(arg stream.Arg) error { - pkgPath := filepath.Join(cockroachDB, pkgScope) - pkgs, err := packages.Load( - &packages.Config{ - Mode: packages.NeedImports | packages.NeedName, - Dir: crdbDir, - }, - pkgPath, - ) - if err != nil { - return errors.Wrapf(err, "error loading package %s", pkgPath) - } - // NB: if no packages were found, this API confusingly - // returns no error, so we need to explicitly check that - // something was returned. - if len(pkgs) == 0 { - return errors.Newf("could not list packages under %s", pkgPath) - } for _, pkg := range pkgs { for _, s := range pkg.Imports { arg.Out <- pkg.PkgPath + ": " + s.PkgPath @@ -2794,4 +2796,56 @@ func TestLint(t *testing.T) { } } }) + + // Test forbidden roachtest imports. + t.Run("TestRoachtestForbiddenImports", func(t *testing.T) { + t.Parallel() + + roachprodLoggerPkg := "github.com/cockroachdb/cockroach/pkg/roachprod/logger" + // forbiddenImportPkg -> permittedReplacementPkg + forbiddenImports := map[string]string{ + "github.com/cockroachdb/cockroach/pkg/util/log": roachprodLoggerPkg, + "log": roachprodLoggerPkg, + } + + // grepBuf creates a grep string that matches any forbidden import pkgs. + var grepBuf bytes.Buffer + grepBuf.WriteByte('(') + for forbiddenPkg := range forbiddenImports { + grepBuf.WriteByte('|') + grepBuf.WriteString(regexp.QuoteMeta(forbiddenPkg)) + } + grepBuf.WriteString(")$") + + filter := stream.FilterFunc(func(arg stream.Arg) error { + for _, pkg := range pkgs { + for _, s := range pkg.Imports { + arg.Out <- pkg.PkgPath + ": " + s.PkgPath + } + } + return nil + }) + numAnalyzed := 0 + if err := stream.ForEach(stream.Sequence( + filter, + stream.Sort(), + stream.Uniq(), + stream.Grep(`cockroach/pkg/cmd/roachtest/(tests|operations): `), + ), func(s string) { + pkgStr := strings.Split(s, ": ") + _, importedPkg := pkgStr[0], pkgStr[1] + numAnalyzed++ + + // Test that a disallowed package is not imported. + if replPkg, ok := forbiddenImports[importedPkg]; ok { + t.Errorf("\n%s <- please use %q instead of %q", s, replPkg, importedPkg) + } + }); err != nil { + t.Error(err) + } + if numAnalyzed == 0 { + t.Errorf("Empty input! Please check the linter.") + } + }) + }