Merge cockroachdb#66930

66930: roachtest: access r.buildVersion through Cluster r=erikgrinaker,stevendanna a=tbg roachtests now don't reach into the `*registry` to get the build version. The "meat" of a roachtest, the "Run" function, thus gets closer to not closing over the scope surrounding its definition. This will allow them to get configured as top-level functions without a need for a "trampoline closure". It will also allow refactoring `*registry` into a subpackage that can handle test registration. This in turn will allow registering a roachtest right where it is defined, rather than the awkward dance now where for every test we need to add a new line to a singleton `register()` function. Not saying this will be done soon, but it is now an option which is nice. Release note: None Co-authored-by: Tobias Grieger <[email protected]>
rytaft · Jun 28, 2021 · e05dfe0 · e05dfe0
2 parents d43d9fd + 8915df7
commit e05dfe0
Show file tree

Hide file tree

Showing 16 changed files with 18 additions and 20 deletions.
diff --git a/pkg/cmd/roachtest/acceptance.go b/pkg/cmd/roachtest/acceptance.go
@@ -47,7 +47,7 @@ func registerAcceptance(r *testRegistry) {
 			{
 				name: "version-upgrade",
 				fn: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-					runVersionUpgrade(ctx, t, c, r.buildVersion)
+					runVersionUpgrade(ctx, t, c)
 				},
 				// This test doesn't like running on old versions because it upgrades to
 				// the latest released version and then it tries to "head", where head is

diff --git a/pkg/cmd/roachtest/autoupgrade.go b/pkg/cmd/roachtest/autoupgrade.go
@@ -249,7 +249,7 @@ func registerAutoUpgrade(r *testRegistry) {
 		MinVersion: "v19.1.0",
 		Cluster:    r.makeClusterSpec(5),
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			pred, err := PredecessorVersion(r.buildVersion)
+			pred, err := PredecessorVersion(*t.BuildVersion())
 			if err != nil {
 				t.Fatal(err)
 			}

diff --git a/pkg/cmd/roachtest/backup.go b/pkg/cmd/roachtest/backup.go
@@ -359,7 +359,7 @@ func registerBackup(r *testRegistry) {
 
 			backupDir := "gs://cockroachdb-backup-testing/" + c.Name() + "?AUTH=implicit"
 			// Use inter-node file sharing on 20.1+.
-			if r.buildVersion.AtLeast(version.MustParse(`v20.1.0-0`)) {
+			if t.BuildVersion().AtLeast(version.MustParse(`v20.1.0-0`)) {
 				backupDir = "nodelocal://1/" + c.Name()
 			}
 			fullDir := backupDir + "/full"

diff --git a/pkg/cmd/roachtest/decommission.go b/pkg/cmd/roachtest/decommission.go
@@ -71,7 +71,7 @@ func registerDecommission(r *testRegistry) {
 			MinVersion: "v20.2.0",
 			Cluster:    r.makeClusterSpec(numNodes),
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-				runDecommissionMixedVersions(ctx, t, c, r.buildVersion)
+				runDecommissionMixedVersions(ctx, t, c, *t.BuildVersion())
 			},
 		})
 	}

diff --git a/pkg/cmd/roachtest/follower_reads.go b/pkg/cmd/roachtest/follower_reads.go
@@ -70,7 +70,7 @@ func registerFollowerReads(r *testRegistry) {
 			spec.CPU(2),
 		),
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			runFollowerReadsMixedVersionSingleRegionTest(ctx, t, c, r.buildVersion)
+			runFollowerReadsMixedVersionSingleRegionTest(ctx, t, c, *t.BuildVersion())
 		},
 	})
 }

diff --git a/pkg/cmd/roachtest/import.go b/pkg/cmd/roachtest/import.go
@@ -310,7 +310,7 @@ func registerImportMixedVersion(r *testRegistry) {
 		MinVersion: "v21.1.0",
 		Cluster:    r.makeClusterSpec(4),
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			predV, err := PredecessorVersion(r.buildVersion)
+			predV, err := PredecessorVersion(*t.BuildVersion())
 			if err != nil {
 				t.Fatal(err)
 			}

diff --git a/pkg/cmd/roachtest/mixed_version_jobs.go b/pkg/cmd/roachtest/mixed_version_jobs.go
@@ -337,7 +337,7 @@ func registerJobsMixedVersions(r *testRegistry) {
 		Skip:       "https://github.com/cockroachdb/cockroach/issues/57230",
 		Cluster:    r.makeClusterSpec(4),
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			predV, err := PredecessorVersion(r.buildVersion)
+			predV, err := PredecessorVersion(*t.BuildVersion())
 			if err != nil {
 				t.Fatal(err)
 			}

diff --git a/pkg/cmd/roachtest/mixed_version_schemachange.go b/pkg/cmd/roachtest/mixed_version_schemachange.go
@@ -35,7 +35,7 @@ func registerSchemaChangeMixedVersions(r *testRegistry) {
 				maxOps = 10
 				concurrency = 2
 			}
-			runSchemaChangeMixedVersions(ctx, t, c, maxOps, concurrency, r.buildVersion)
+			runSchemaChangeMixedVersions(ctx, t, c, maxOps, concurrency, *t.BuildVersion())
 		},
 	})
 }

diff --git a/pkg/cmd/roachtest/multitenant_upgrade.go b/pkg/cmd/roachtest/multitenant_upgrade.go
@@ -36,7 +36,7 @@ func registerMultiTenantUpgrade(r *testRegistry) {
 		Owner:             OwnerKV,
 		NonReleaseBlocker: false,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			runMultiTenantUpgrade(ctx, t, c, r.buildVersion)
+			runMultiTenantUpgrade(ctx, t, c, *t.BuildVersion())
 		},
 	})
 }

diff --git a/pkg/cmd/roachtest/schema_change_database_version_upgrade.go b/pkg/cmd/roachtest/schema_change_database_version_upgrade.go
@@ -38,7 +38,7 @@ func registerSchemaChangeDatabaseVersionUpgrade(r *testRegistry) {
 		MinVersion: "v20.2.0",
 		Cluster:    r.makeClusterSpec(3),
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			runSchemaChangeDatabaseVersionUpgrade(ctx, t, c, r.buildVersion)
+			runSchemaChangeDatabaseVersionUpgrade(ctx, t, c, *t.BuildVersion())
 		},
 	})
 }

diff --git a/pkg/cmd/roachtest/secondary_indexes.go b/pkg/cmd/roachtest/secondary_indexes.go
@@ -140,7 +140,7 @@ func registerSecondaryIndexesMultiVersionCluster(r *testRegistry) {
 		Cluster:    r.makeClusterSpec(3),
 		MinVersion: "v20.1.0",
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			predV, err := PredecessorVersion(r.buildVersion)
+			predV, err := PredecessorVersion(*t.BuildVersion())
 			if err != nil {
 				t.Fatal(err)
 			}

diff --git a/pkg/cmd/roachtest/test/BUILD.bazel b/pkg/cmd/roachtest/test/BUILD.bazel
@@ -2,7 +2,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
 go_library(
     name = "test",
-    srcs = ["test_interface.go.go"],
+    srcs = ["test_interface.go"],
     importpath = "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test",
     visibility = ["//visibility:public"],
     deps = [

diff --git a/pkg/cmd/roachtest/test/test_interface.go.go → pkg/cmd/roachtest/test/test_interface.go b/pkg/cmd/roachtest/test/test_interface.go.go → pkg/cmd/roachtest/test/test_interface.go
diff --git a/pkg/cmd/roachtest/tpcc.go b/pkg/cmd/roachtest/tpcc.go
@@ -307,7 +307,7 @@ func registerTPCC(r *testRegistry) {
 		Tags:       []string{`default`, `release_qualification`},
 		Cluster:    headroomSpec,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			maxWarehouses := maxSupportedTPCCWarehouses(r.buildVersion, cloud, t.Spec().(*TestSpec).Cluster)
+			maxWarehouses := maxSupportedTPCCWarehouses(*t.BuildVersion(), cloud, t.Spec().(*TestSpec).Cluster)
 			headroomWarehouses := int(float64(maxWarehouses) * 0.7)
 			t.L().Printf("computed headroom warehouses of %d\n", headroomWarehouses)
 			runTPCC(ctx, t, c, tpccOptions{
@@ -334,7 +334,7 @@ func registerTPCC(r *testRegistry) {
 			crdbNodes := c.Range(1, 4)
 			workloadNode := c.Node(5)
 
-			maxWarehouses := maxSupportedTPCCWarehouses(r.buildVersion, cloud, t.Spec().(*TestSpec).Cluster)
+			maxWarehouses := maxSupportedTPCCWarehouses(*t.BuildVersion(), cloud, t.Spec().(*TestSpec).Cluster)
 			headroomWarehouses := int(float64(maxWarehouses) * 0.7)
 			if local {
 				headroomWarehouses = 10
@@ -370,7 +370,7 @@ func registerTPCC(r *testRegistry) {
 				bankRows = 1000
 			}
 
-			oldV, err := PredecessorVersion(r.buildVersion)
+			oldV, err := PredecessorVersion(*t.BuildVersion())
 			if err != nil {
 				t.Fatal(err)
 			}

diff --git a/pkg/cmd/roachtest/version.go b/pkg/cmd/roachtest/version.go
@@ -217,7 +217,7 @@ func registerVersion(r *testRegistry) {
 			MinVersion: "v2.1.0",
 			Cluster:    r.makeClusterSpec(n + 1),
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-				pred, err := PredecessorVersion(r.buildVersion)
+				pred, err := PredecessorVersion(*t.BuildVersion())
 				if err != nil {
 					t.Fatal(err)
 				}

diff --git a/pkg/cmd/roachtest/versionupgrade.go b/pkg/cmd/roachtest/versionupgrade.go
@@ -80,10 +80,8 @@ DROP TABLE test.t;
 	`),
 }
 
-func runVersionUpgrade(
-	ctx context.Context, t test.Test, c cluster.Cluster, buildVersion version.Version,
-) {
-	predecessorVersion, err := PredecessorVersion(buildVersion)
+func runVersionUpgrade(ctx context.Context, t test.Test, c cluster.Cluster) {
+	predecessorVersion, err := PredecessorVersion(*t.BuildVersion())
 	if err != nil {
 		t.Fatal(err)
 	}