From fda49386e73a8fb0808f1a9545382f5dbec68961 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Fri, 10 Dec 2021 19:55:42 -0500 Subject: [PATCH] *: address cases of improper error wrapping Release note: None --- pkg/cli/clisqlclient/conn_test.go | 4 ++-- .../roachtest/tests/schema_change_database_version_upgrade.go | 3 ++- pkg/col/coldatatestutils/random_testutils.go | 2 +- pkg/geo/geos/geos.go | 4 ++-- pkg/jobs/job_scheduler_test.go | 2 +- pkg/kv/kvserver/reports/critical_localities_report.go | 2 +- pkg/kv/kvserver/reports/replication_stats_report.go | 2 +- pkg/kv/kvserver/reports/reporter.go | 2 +- pkg/rpc/context_test.go | 4 ++-- pkg/security/securitytest/securitytest.go | 3 ++- pkg/server/drain_test.go | 4 +++- pkg/util/log/logcrash/crash_reporting_test.go | 2 +- pkg/util/log/redact_test.go | 2 +- 13 files changed, 20 insertions(+), 16 deletions(-) diff --git a/pkg/cli/clisqlclient/conn_test.go b/pkg/cli/clisqlclient/conn_test.go index 5fe07d75c7ad..fabd6d5c2910 100644 --- a/pkg/cli/clisqlclient/conn_test.go +++ b/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 }) diff --git a/pkg/cmd/roachtest/tests/schema_change_database_version_upgrade.go b/pkg/cmd/roachtest/tests/schema_change_database_version_upgrade.go index 38ac9e730c89..43d7862a0491 100644 --- a/pkg/cmd/roachtest/tests/schema_change_database_version_upgrade.go +++ b/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 } diff --git a/pkg/col/coldatatestutils/random_testutils.go b/pkg/col/coldatatestutils/random_testutils.go index ae7b0562b546..f906f5d66260 100644 --- a/pkg/col/coldatatestutils/random_testutils.go +++ b/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()) diff --git a/pkg/geo/geos/geos.go b/pkg/geo/geos/geos.go index 7176892f17aa..c931955e7700 100644 --- a/pkg/geo/geos/geos.go +++ b/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")) } } diff --git a/pkg/jobs/job_scheduler_test.go b/pkg/jobs/job_scheduler_test.go index b8603e04ce49..04b2d475c525 100644 --- a/pkg/jobs/job_scheduler_test.go +++ b/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 }) diff --git a/pkg/kv/kvserver/reports/critical_localities_report.go b/pkg/kv/kvserver/reports/critical_localities_report.go index 3c5a8acd1770..6ca36535ee09 100644 --- a/pkg/kv/kvserver/reports/critical_localities_report.go +++ b/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) diff --git a/pkg/kv/kvserver/reports/replication_stats_report.go b/pkg/kv/kvserver/reports/replication_stats_report.go index dccc723d8e6f..87a8f5e15070 100644 --- a/pkg/kv/kvserver/reports/replication_stats_report.go +++ b/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 diff --git a/pkg/kv/kvserver/reports/reporter.go b/pkg/kv/kvserver/reports/reporter.go index c65f61399a08..2c60a61a4ff6 100644 --- a/pkg/kv/kvserver/reports/reporter.go +++ b/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:]...) diff --git a/pkg/rpc/context_test.go b/pkg/rpc/context_test.go index a3b6646e565e..fd3ad4401c44 100644 --- a/pkg/rpc/context_test.go +++ b/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 } diff --git a/pkg/security/securitytest/securitytest.go b/pkg/security/securitytest/securitytest.go index 18731a1f50f8..e83dca39af55 100644 --- a/pkg/security/securitytest/securitytest.go +++ b/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 } diff --git a/pkg/server/drain_test.go b/pkg/server/drain_test.go index 11bd604f352f..4b88374ccafc 100644 --- a/pkg/server/drain_test.go +++ b/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 */) }) } diff --git a/pkg/util/log/logcrash/crash_reporting_test.go b/pkg/util/log/logcrash/crash_reporting_test.go index 120f70acbdbd..59b053270d47 100644 --- a/pkg/util/log/logcrash/crash_reporting_test.go +++ b/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"), diff --git a/pkg/util/log/redact_test.go b/pkg/util/log/redact_test.go index e21dfef2bd28..7a463b268c69 100644 --- a/pkg/util/log/redact_test.go +++ b/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()) }