*: address cases of improper error wrapping

Release note: None

pkg/cli/clisqlclient/conn_test.go

@@ -69,7 +69,7 @@ func TestConnRecover(t *testing.T) {
 				t.Fatal(closeErr)
 			}
 		} else if !errors.Is(err, driver.ErrBadConn) {
-			return errors.Newf("expected ErrBadConn, got %v", err)
+			return errors.Newf("expected ErrBadConn, got %v", err /* nolint:errwrap */)
 		}
 		return nil
 	})
@@ -89,7 +89,7 @@ func TestConnRecover(t *testing.T) {
 	// Ditto from Query().
 	testutils.SucceedsSoon(t, func() error {
 		if err := conn.Exec(`SELECT 1`, nil); !errors.Is(err, driver.ErrBadConn) {
-			return errors.Newf("expected ErrBadConn, got %v", err)
+			return errors.Newf("expected ErrBadConn, got %v", err /* nolint:errwrap */)
 		}
 		return nil
 	})

pkg/cmd/roachtest/tests/schema_change_database_version_upgrade.go

@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util/version"
 	"github.com/cockroachdb/errors"
@@ -91,7 +92,7 @@ func runSchemaChangeDatabaseVersionUpgrade(
 	assertDatabaseNotResolvable := func(ctx context.Context, db *gosql.DB, dbName string) error {
 		_, err = db.ExecContext(ctx, fmt.Sprintf(`SELECT table_name FROM [SHOW TABLES FROM %s]`, dbName))
 		if err == nil || err.Error() != "pq: target database or schema does not exist" {
-			return errors.AssertionFailedf("unexpected error: %s", err)
+			return errors.Newf("unexpected error: %s", pgerror.FullError(err))
 		}
 		return nil
 	}

pkg/col/coldatatestutils/random_testutils.go

@@ -216,7 +216,7 @@ func setNull(rng *rand.Rand, vec coldata.Vec, i int) {
 	case types.DecimalFamily:
 		_, err := vec.Decimal()[i].SetFloat64(rng.Float64())
 		if err != nil {
-			colexecerror.InternalError(errors.AssertionFailedf("%v", err))
+			colexecerror.InternalError(errors.NewAssertionErrorWithWrappedErrf(err, "could not set decimal"))
 		}
 	case types.IntervalFamily:
 		vec.Interval()[i] = duration.MakeDuration(rng.Int63(), rng.Int63(), rng.Int63())

pkg/geo/geos/geos.go

@@ -654,11 +654,11 @@ func PrepareGeometry(a geopb.EWKB) (PreparedGeometry, error) {
 func PreparedGeomDestroy(a PreparedGeometry) {
 	g, err := ensureInitInternal()
 	if err != nil {
-		panic(errors.AssertionFailedf("trying to destroy PreparedGeometry with no GEOS: %v", err))
+		panic(errors.NewAssertionErrorWithWrappedErrf(err, "trying to destroy PreparedGeometry with no GEOS"))
 	}
 	ap := (*C.CR_GEOS_PreparedGeometry)(unsafe.Pointer(a))
 	if err := statusToError(C.CR_GEOS_PreparedGeometryDestroy(g, ap)); err != nil {
-		panic(errors.AssertionFailedf("PreparedGeometryDestroy returned an error: %v", err))
+		panic(errors.NewAssertionErrorWithWrappedErrf(err, "PreparedGeometryDestroy returned an error"))
 	}
 }
 

pkg/jobs/job_scheduler_test.go

@@ -616,7 +616,7 @@ func TestJobSchedulerDaemonUsesSystemTables(t *testing.T) {
 		var count int
 		if err := db.QueryRow(
 			"SELECT count(*) FROM defaultdb.foo").Scan(&count); err != nil || count != 3 {
-			return errors.Newf("expected 3 rows, got %d (err=%+v)", count, err)
+			return errors.Newf("expected 3 rows, got %d (err=%+v)", count, err /* nolint:errwrap */)
 		}
 		return nil
 	})

pkg/kv/kvserver/reports/critical_localities_report.go

@@ -353,7 +353,7 @@ func (v *criticalLocalitiesVisitor) visitNewZone(
 			return true
 		})
 	if err != nil {
-		return errors.AssertionFailedf("unexpected error visiting zones: %s", err)
+		return errors.NewAssertionErrorWithWrappedErrf(err, "unexpected error visiting zones")
 	}
 	if !found {
 		return errors.AssertionFailedf("no suitable zone config found for range: %s", r)

pkg/kv/kvserver/reports/replication_stats_report.go

@@ -366,7 +366,7 @@ func (v *replicationStatsVisitor) visitNewZone(
 			return false
 		})
 	if err != nil {
-		return errors.AssertionFailedf("unexpected error visiting zones for range %s: %s", r, err)
+		return errors.NewAssertionErrorWithWrappedErrf(err, "unexpected error visiting zones for range %s", r)
 	}
 	v.prevZoneKey = zKey
 	v.prevNumReplicas = numReplicas

pkg/kv/kvserver/reports/reporter.go

@@ -610,7 +610,7 @@ func visitRanges(
 			if err != nil {
 				// Sanity check - v.failed() should return an error now (the same as err above).
 				if !v.failed() {
-					return errors.AssertionFailedf("expected visitor %T to have failed() after error: %s", v, err)
+					return errors.NewAssertionErrorWithWrappedErrf(err, "expected visitor %T to have failed() after error", v)
 				}
 				// Remove this visitor; it shouldn't be called any more.
 				visitors = append(visitors[:i], visitors[i+1:]...)

pkg/rpc/context_test.go

@@ -1307,13 +1307,13 @@ func grpcRunKeepaliveTestCase(testCtx context.Context, c grpcKeepaliveTestCase)
 	}
 	if c.expClose {
 		if sendErr == nil || !grpcutil.IsClosedConnection(sendErr) {
-			newErr := errors.Newf("expected closed connection, found %v", sendErr)
+			newErr := errors.Newf("expected closed connection, found %v", sendErr /* nolint:errwrap */)
 			log.Infof(ctx, "%+v", newErr)
 			return newErr
 		}
 	} else {
 		if sendErr != nil {
-			newErr := errors.Newf("expected unclosed connection, found %v", sendErr)
+			newErr := errors.Newf("expected unclosed connection, found %v", sendErr /* nolint:errwrap */)
 			log.Infof(ctx, "%+v", newErr)
 			return newErr
 		}

pkg/security/securitytest/securitytest.go

@@ -76,7 +76,8 @@ func AssetReadDir(name string) ([]os.FileInfo, error) {
 		info, err := AssetInfo(joined)
 		if err != nil {
 			if _, dirErr := AssetDir(joined); dirErr != nil {
-				return nil, errors.Wrapf(err, "missing directory (%s)", dirErr)
+				errString := dirErr.Error()
+				return nil, errors.Wrapf(err, "missing directory (%s)", errString)
 			}
 			continue // skip subdirectory
 		}

pkg/server/drain_test.go

@@ -87,7 +87,9 @@ func doTestDrain(tt *testing.T) {
 		if grpcutil.IsClosedConnection(err) {
 			return nil
 		}
-		return errors.Newf("server not yet refusing RPC, got %v", err)
+		// It is incorrect to use errors.Wrap since that will result in a nil
+		// return value if err is nil, which is not desired.
+		return errors.Newf("server not yet refusing RPC, got %v", err /* nolint:errwrap */)
 	})
 }
 

pkg/util/log/logcrash/crash_reporting_test.go

@@ -45,7 +45,7 @@ var safeErrorTestCases = func() []safeErrorTestCase {
 	var errWrapped1 = errors.Wrap(errFundamental, "this is reportable")
 	var errWrapped2 = errors.Wrapf(errWrapped1, "this is reportable too")
 	var errWrapped3 = errors.Wrap(errWrapped2, "this is reportable as well")
-	var errFormatted = errors.Newf("this embed an error: %v", errWrapped2)
+	var errFormatted = errors.Newf("this embed an error: %v", errWrapped2 /* nolint:errwrap */)
 	var errWrappedSentinel = errors.Wrap(
 		errors.Wrapf(errSentinel,
 			"this is reportable"),

pkg/util/log/redact_test.go

@@ -71,7 +71,7 @@ func TestRedactedLogOutput(t *testing.T) {
 	resetCaptured()
 	Errorf(context.Background(), "test3e %v end",
 		errors.AssertionFailedf("hello %v",
-			errors.Newf("error-in-error %s", "world")))
+			errors.Newf("error-in-error %s", "world") /* nolint:errwrap */))
 	if !contains(redactableIndicator+" [-] 4  test3e", t) {
 		t.Errorf("expected marker indicator, got %q", contents())
 	}