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

@@ -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{})

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) {

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']",

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)

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,