Merge cockroachdb#51801

51801: *: use skip.* methods, not t.Skip r=petermattis,nvanbenschoten a=jordanlewis

This commit introduces a new family of methods in the testutils/skip
package:

- WithIssue, WithIssuef: skip a test temporarily, linking to a GitHub
  issue.
- UnderRace, UnderShort, UnderStress, UnderStressRace: skip a test when
  run under the race detector, -short flag, stress build, or stressrace
  build, respectively.
- IgnoreLint, IgnoreLintf: skip a test for another reason, without
  bothering the linter.

It also removes all naked t.Skip and t.Skipf calls, replacing them with
one of the above skip package methods, and adds a linter to ensure that
naked t.Skip is never used.

This opens the door to performing more analytics on our skipped tests,
because of the extra structured data these methods provide.

Release note: None

Co-authored-by: Jordan Lewis <[email protected]>

pkg/acceptance/cli_test.go

@@ -19,6 +19,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/acceptance/cluster"
 	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 )
 
@@ -42,7 +43,7 @@ func TestDockerCLI(t *testing.T) {
 	containerConfig.Env = []string{fmt.Sprintf("PGUSER=%s", security.RootUser)}
 	ctx := context.Background()
 	if err := testDockerOneShot(ctx, t, "cli_test", containerConfig); err != nil {
-		t.Skipf(`TODO(dt): No binary in one-shot container, see #6086: %s`, err)
+		skip.IgnoreLintf(t, `TODO(dt): No binary in one-shot container, see #6086: %s`, err)
 	}
 
 	paths, err := filepath.Glob(testGlob)
@@ -102,7 +103,7 @@ func TestDockerUnixSocket(t *testing.T) {
 	ctx := context.Background()
 
 	if err := testDockerOneShot(ctx, t, "cli_test", containerConfig); err != nil {
-		t.Skipf(`TODO(dt): No binary in one-shot container, see #6086: %s`, err)
+		skip.IgnoreLintf(t, `TODO(dt): No binary in one-shot container, see #6086: %s`, err)
 	}
 
 	containerConfig.Env = []string{fmt.Sprintf("PGUSER=%s", security.RootUser)}

pkg/acceptance/compose/gss/psql/gss_test.go

@@ -150,7 +150,7 @@ func TestGSSFileDescriptorCount(t *testing.T) {
 	// track the open file count in the cockroach container, but that seems
 	// brittle and probably not worth the effort. However this test is
 	// useful when doing manual tracking of file descriptor count.
-	t.Skip("skip")
+	t.Skip("#51791")
 
 	rootConnector, err := pq.NewConnector("user=root sslmode=require")
 	if err != nil {

pkg/bench/bench_test.go

@@ -22,6 +22,7 @@ import (
 	"sync/atomic"
 	"testing"
 
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
@@ -105,9 +106,7 @@ func BenchmarkSelect3(b *testing.B) {
 }
 
 func BenchmarkCount(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
 		defer func() {
@@ -141,9 +140,7 @@ func BenchmarkCount(b *testing.B) {
 }
 
 func BenchmarkCountTwoCF(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
 		defer func() {
@@ -177,9 +174,7 @@ func BenchmarkCountTwoCF(b *testing.B) {
 }
 
 func BenchmarkSort(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
 		defer func() {
@@ -215,9 +210,7 @@ func BenchmarkSort(b *testing.B) {
 // BenchmarkTableResolution benchmarks table name resolution
 // for a variety of different naming schemes.
 func BenchmarkTableResolution(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 
 	for _, createTempTables := range []bool{false, true} {
@@ -379,9 +372,7 @@ func runBenchmarkInsertSecondaryIndex(b *testing.B, db *sqlutils.SQLRunner, coun
 }
 
 func BenchmarkSQL(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
 		for _, runFn := range []func(*testing.B, *sqlutils.SQLRunner, int){
@@ -561,9 +552,7 @@ func runBenchmarkScan(b *testing.B, db *sqlutils.SQLRunner, count int, limit int
 }
 
 func BenchmarkScan(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
 		for _, count := range []int{1, 10, 100, 1000, 10000} {
@@ -775,9 +764,7 @@ func runBenchmarkOrderBy(
 }
 
 func BenchmarkOrderBy(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 	const count = 100000
 	const limit = 10
@@ -1038,9 +1025,7 @@ CREATE TABLE bench.scan(
 }
 
 func BenchmarkWideTable(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 	const count = 10
 	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
@@ -1055,9 +1040,7 @@ func BenchmarkWideTable(b *testing.B) {
 }
 
 func BenchmarkWideTableIgnoreColumns(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
 		db.Exec(b, wideTableSchema)
@@ -1076,9 +1059,7 @@ func BenchmarkWideTableIgnoreColumns(b *testing.B) {
 // BenchmarkPlanning runs some queries on an empty table. The purpose is to
 // benchmark (and get memory allocation statistics for) the planning process.
 func BenchmarkPlanning(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
 		db.Exec(b, `CREATE TABLE abc (a INT PRIMARY KEY, b INT, c INT, INDEX(b), UNIQUE INDEX(c))`)
 
@@ -1182,9 +1163,7 @@ func BenchmarkSortJoinAggregation(b *testing.B) {
 }
 
 func BenchmarkNameResolution(b *testing.B) {
-	if testing.Short() {
-		b.Skip("short flag")
-	}
+	skip.UnderShort(b)
 	defer log.Scope(b).Close(b)
 	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
 		db.Exec(b, `CREATE TABLE namespace (k INT PRIMARY KEY, v INT)`)

pkg/bench/foreachdb.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	_ "github.com/go-sql-driver/mysql" // registers the MySQL driver to gosql
@@ -91,7 +92,7 @@ func benchmarkPostgres(b *testing.B, f BenchmarkFn) {
 		RawQuery: "sslmode=require&dbname=postgres",
 	}
 	if conn, err := net.Dial("tcp", pgURL.Host); err != nil {
-		b.Skipf("unable to connect to postgres server on %s: %s", pgURL.Host, err)
+		skip.IgnoreLintf(b, "unable to connect to postgres server on %s: %s", pgURL.Host, err)
 	} else {
 		conn.Close()
 	}
@@ -111,7 +112,7 @@ func benchmarkPostgres(b *testing.B, f BenchmarkFn) {
 func benchmarkMySQL(b *testing.B, f BenchmarkFn) {
 	const addr = "localhost:3306"
 	if conn, err := net.Dial("tcp", addr); err != nil {
-		b.Skipf("unable to connect to mysql server on %s: %s", addr, err)
+		skip.IgnoreLintf(b, "unable to connect to mysql server on %s: %s", addr, err)
 	} else {
 		conn.Close()
 	}

pkg/bench/pgbench_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -86,7 +87,7 @@ func BenchmarkPgbenchQueryParallel(b *testing.B) {
 
 func execPgbench(b *testing.B, pgURL url.URL) {
 	if _, err := exec.LookPath("pgbench"); err != nil {
-		b.Skip("pgbench is not available on PATH")
+		skip.IgnoreLint(b, "pgbench is not available on PATH")
 	}
 	c, err := SetupExec(pgURL, "bench", 20000, b.N)
 	if err != nil {
@@ -126,7 +127,7 @@ func BenchmarkPgbenchExec(b *testing.B) {
 			RawQuery: "sslmode=disable&dbname=postgres",
 		}
 		if conn, err := net.Dial("tcp", pgURL.Host); err != nil {
-			b.Skipf("unable to connect to postgres server on %s: %s", pgURL.Host, err)
+			skip.IgnoreLintf(b, "unable to connect to postgres server on %s: %s", pgURL.Host, err)
 		} else {
 			conn.Close()
 		}

pkg/ccl/backupccl/backup_cloud_test.go

@@ -17,6 +17,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws/credentials"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
 	"github.com/cockroachdb/cockroach/pkg/storage/cloudimpl"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -39,11 +40,11 @@ func TestCloudBackupRestoreS3(t *testing.T) {
 	defer log.Scope(t).Close(t)
 	creds, err := credentials.NewEnvCredentials().Get()
 	if err != nil {
-		t.Skipf("No AWS env keys (%v)", err)
+		skip.IgnoreLintf(t, "No AWS env keys (%v)", err)
 	}
 	bucket := os.Getenv("AWS_S3_BUCKET")
 	if bucket == "" {
-		t.Skip("AWS_S3_BUCKET env var must be set")
+		skip.IgnoreLint(t, "AWS_S3_BUCKET env var must be set")
 	}
 
 	// TODO(dan): Actually invalidate the descriptor cache and delete this line.
@@ -69,7 +70,7 @@ func TestCloudBackupRestoreGoogleCloudStorage(t *testing.T) {
 	defer log.Scope(t).Close(t)
 	bucket := os.Getenv("GS_BUCKET")
 	if bucket == "" {
-		t.Skip("GS_BUCKET env var must be set")
+		skip.IgnoreLint(t, "GS_BUCKET env var must be set")
 	}
 
 	// TODO(dan): Actually invalidate the descriptor cache and delete this line.
@@ -92,11 +93,11 @@ func TestCloudBackupRestoreAzure(t *testing.T) {
 	accountName := os.Getenv("AZURE_ACCOUNT_NAME")
 	accountKey := os.Getenv("AZURE_ACCOUNT_KEY")
 	if accountName == "" || accountKey == "" {
-		t.Skip("AZURE_ACCOUNT_NAME and AZURE_ACCOUNT_KEY env vars must be set")
+		skip.IgnoreLint(t, "AZURE_ACCOUNT_NAME and AZURE_ACCOUNT_KEY env vars must be set")
 	}
 	bucket := os.Getenv("AZURE_CONTAINER")
 	if bucket == "" {
-		t.Skip("AZURE_CONTAINER env var must be set")
+		skip.IgnoreLint(t, "AZURE_CONTAINER env var must be set")
 	}
 
 	// TODO(dan): Actually invalidate the descriptor cache and delete this line.

pkg/ccl/backupccl/backup_test.go

@@ -52,6 +52,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/jobutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -780,7 +781,7 @@ func TestBackupRestoreCheckpointing(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	t.Skip("https://github.com/cockroachdb/cockroach/issues/33357")
+	skip.WithIssue(t, 33357)
 
 	defer func(oldInterval time.Duration) {
 		BackupCheckpointInterval = oldInterval
@@ -1018,7 +1019,7 @@ func getHighWaterMark(jobID int64, sqlDB *gosql.DB) (roachpb.Key, error) {
 func TestBackupRestoreControlJob(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-	t.Skip("#24136")
+	skip.WithIssue(t, 24136)
 
 	// force every call to update
 	defer jobs.TestingSetProgressThresholds()()

pkg/ccl/backupccl/bench_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/importccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl/sampledataccl"
 	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/workload/bank"
 	"github.com/cockroachdb/cockroach/pkg/workload/workloadsql"
@@ -40,9 +41,7 @@ func bankBuf(numAccounts int) *bytes.Buffer {
 }
 
 func BenchmarkClusterBackup(b *testing.B) {
-	if testing.Short() {
-		b.Skip("TODO: fix benchmark")
-	}
+	skip.UnderShort(b, "TODO: fix benchmark")
 	// NB: This benchmark takes liberties in how b.N is used compared to the go
 	// documentation's description. We're getting useful information out of it,
 	// but this is not a pattern to cargo-cult.
@@ -97,9 +96,7 @@ func BenchmarkClusterRestore(b *testing.B) {
 }
 
 func BenchmarkLoadRestore(b *testing.B) {
-	if testing.Short() {
-		b.Skip("TODO: fix benchmark")
-	}
+	skip.UnderShort(b, "TODO: fix benchmark")
 	// NB: This benchmark takes liberties in how b.N is used compared to the go
 	// documentation's description. We're getting useful information out of it,
 	// but this is not a pattern to cargo-cult.
@@ -149,9 +146,7 @@ func BenchmarkLoadSQL(b *testing.B) {
 }
 
 func BenchmarkClusterEmptyIncrementalBackup(b *testing.B) {
-	if testing.Short() {
-		b.Skip("TODO: fix benchmark")
-	}
+	skip.UnderShort(b, "TODO: fix benchmark")
 	const numStatements = 100000
 
 	_, _, sqlDB, _, cleanupFn := backupccl.BackupRestoreTestSetup(b, backupccl.MultiNode, 0, backupccl.InitNone)

pkg/ccl/changefeedccl/bench_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -54,7 +55,7 @@ func BenchmarkChangefeedTicks(b *testing.B) {
 	// work with RangeFeed without a rewrite, but it's not being used for anything
 	// right now, so the rewrite isn't worth it. We should fix this if we need to
 	// start doing changefeed perf work at some point.
-	b.Skip(`broken in #38211`)
+	skip.WithIssue(b, 51842, `broken in #38211`)
 
 	ctx := context.Background()
 	s, sqlDBRaw, _ := serverutils.StartServer(b, base.TestServerArgs{UseDatabase: "d"})