From 2622def375a2b0d54d9528fdcf6d4775c5ddc3c1 Mon Sep 17 00:00:00 2001 From: Miral Gadani Date: Tue, 27 Sep 2022 19:13:35 -0400 Subject: [PATCH] roachtest: - Added t.Error for consistency with t.Fatal, t.Skip - Updated t.Fatalf to use errors.Newf - previously, format was ignored - Github issue message body updated to `t.FailureMsg()` - Removed unused `t.m.timeout` - Add secondary function definition to fmtsafe/functions.go to satisfy linter. `testImpl.Errorf` requires both `(*main.testImpl).Errorf` and `(*github.com/cockroachdb/cockroach/pkg/cmd/roachtest.testImpl).Errorf` to be added to `fmtsafe/functions.go` otherwise the fmtsafe will fail. Release note: None --- pkg/cmd/roachtest/test/test_interface.go | 1 + pkg/cmd/roachtest/test_impl.go | 13 ++++++++----- pkg/cmd/roachtest/test_runner.go | 4 +--- pkg/testutils/lint/passes/fmtsafe/fmtsafe.go | 3 ++- pkg/testutils/lint/passes/fmtsafe/functions.go | 16 ++++++++++++++-- 5 files changed, 26 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/roachtest/test/test_interface.go b/pkg/cmd/roachtest/test/test_interface.go index 6ccbc94adbb9..7da9b1c030d5 100644 --- a/pkg/cmd/roachtest/test/test_interface.go +++ b/pkg/cmd/roachtest/test/test_interface.go @@ -37,6 +37,7 @@ type Test interface { VersionsBinaryOverride() map[string]string Skip(args ...interface{}) Skipf(format string, args ...interface{}) + Error(args ...interface{}) Errorf(string, ...interface{}) FailNow() Fatal(args ...interface{}) diff --git a/pkg/cmd/roachtest/test_impl.go b/pkg/cmd/roachtest/test_impl.go index 6e403ca621b7..713cb04ca156 100644 --- a/pkg/cmd/roachtest/test_impl.go +++ b/pkg/cmd/roachtest/test_impl.go @@ -88,9 +88,6 @@ type testImpl struct { // test is being marked as failed (i.e. when the failed field above is also // set). This is used to cancel the context passed to t.spec.Run(), so async // test goroutines can be notified. - // - // TODO(test-eng): use a structured error instead. - timeout bool // status is a map from goroutine id to status set by that goroutine. A // special goroutine is indicated by runnerID; that one provides the test's @@ -257,6 +254,8 @@ func (t *testImpl) Skipf(format string, args ...interface{}) { panic(errTestFatal) } +// This creates an error from the first arg, and adds each subsequent arg +// as error detail func argsToErr(depth int, args ...interface{}) error { // NB: we'd probably not allow multiple arguments here and we'd want // the one remaining arg to be an `error`, but we are trying to be @@ -287,7 +286,7 @@ func (t *testImpl) Fatal(args ...interface{}) { // Fatalf is like Fatal, but takes a format string. func (t *testImpl) Fatalf(format string, args ...interface{}) { - t.addFailure(argsToErr(1, args...)) + t.addFailure(errors.Newf(format, args...)) panic(errTestFatal) } @@ -297,9 +296,13 @@ func (t *testImpl) FailNow() { panic(errTestFatal) } +func (t *testImpl) Error(args ...interface{}) { + t.addFailure(argsToErr(1, args...)) +} + // Errorf implements the TestingT interface. func (t *testImpl) Errorf(format string, args ...interface{}) { - t.addFailure(errors.NewWithDepthf(1, format, args...)) + t.addFailure(errors.Newf(format, args...)) } func formatFailure(b *strings.Builder, errs ...error) { diff --git a/pkg/cmd/roachtest/test_runner.go b/pkg/cmd/roachtest/test_runner.go index 2a81892c4e82..2e325dd98924 100644 --- a/pkg/cmd/roachtest/test_runner.go +++ b/pkg/cmd/roachtest/test_runner.go @@ -818,9 +818,7 @@ func (r *testRunner) runTest( durationStr := fmt.Sprintf("%.2fs", t.duration().Seconds()) if t.Failed() { - t.mu.Lock() - output := fmt.Sprintf("test artifacts and logs in: %s\n", t.ArtifactsDir()) + string(t.mu.output) - t.mu.Unlock() + output := fmt.Sprintf("test artifacts and logs in: %s\n%s", t.ArtifactsDir(), t.FailureMsg()) if teamCity { shout(ctx, l, stdout, "##teamcity[testFailed name='%s' details='%s' flowId='%s']", diff --git a/pkg/testutils/lint/passes/fmtsafe/fmtsafe.go b/pkg/testutils/lint/passes/fmtsafe/fmtsafe.go index f55046ac48ee..f7f4029f3695 100644 --- a/pkg/testutils/lint/passes/fmtsafe/fmtsafe.go +++ b/pkg/testutils/lint/passes/fmtsafe/fmtsafe.go @@ -251,7 +251,8 @@ func checkCallExpr(pass *analysis.Pass, enclosingFnName string, call *ast.CallEx // Tip is exported for use in tests. var Tip = ` -Tip: use YourFuncf("descriptive prefix %%s", ...) or list new formatting wrappers in pkg/testutils/lint/passes/fmtsafe/functions.go.` +Tip: use YourFuncf("descriptive prefix %%s", ...) or list new formatting wrappers in pkg/testutils/lint/passes/fmtsafe/functions.go. +N.B. additional entry is required functions in the main package. See functions.go#requireConstFmt` func hasNoLintComment(pass *analysis.Pass, call *ast.CallExpr, idx int) bool { fPos, f := findContainingFile(pass, call) diff --git a/pkg/testutils/lint/passes/fmtsafe/functions.go b/pkg/testutils/lint/passes/fmtsafe/functions.go index 726ba6ad15ec..5bd4fe3ff1b0 100644 --- a/pkg/testutils/lint/passes/fmtsafe/functions.go +++ b/pkg/testutils/lint/passes/fmtsafe/functions.go @@ -32,8 +32,14 @@ var requireConstMsg = map[string]bool{ "(*github.com/cockroachdb/cockroach/pkg/sql.optPlanningCtx).log": true, } -// requireConstFmt records functions for which the string arg -// before the final ellipsis must be a constant string. +/* +requireConstFmt records functions for which the string arg +before the final ellipsis must be a constant string. + +Definitions surrounded in parentheses are functions attached to a struct. +For functions defined in the main package, a *second* entry is required +in the form (main.yourStruct).yourFuncF +*/ var requireConstFmt = map[string]bool{ // Logging things. "log.Printf": true, @@ -85,8 +91,14 @@ var requireConstFmt = map[string]bool{ "(*github.com/cockroachdb/cockroach/pkg/util/grpcutil.grpcLogger).Errorf": true, "(*github.com/cockroachdb/cockroach/pkg/util/grpcutil.grpcLogger).Fatalf": true, + // Both of these signatures need to be included for the linter to not flag + // roachtest testImpl.Errorf since it is in the main package + "(*main.testImpl).Errorf": true, "(*github.com/cockroachdb/cockroach/pkg/cmd/roachtest.testImpl).Errorf": true, + "(*main.testImpl).Fatalf": true, + "(*github.com/cockroachdb/cockroach/pkg/cmd/roachtest.testImpl).Fatalf": true, + "(*github.com/cockroachdb/cockroach/pkg/kv/kvserver.raftLogger).Debugf": true, "(*github.com/cockroachdb/cockroach/pkg/kv/kvserver.raftLogger).Infof": true, "(*github.com/cockroachdb/cockroach/pkg/kv/kvserver.raftLogger).Warningf": true,