cmd/dist: introduce test variants

This introduces the concept of test variants in dist, which are
different configurations of the same package. The variant of a test is
a short string summarizing the configuration.

The "variant name" of a test is either the package name if the variant
is empty, or package:variant if not. Currently this isn't used for
anything, but soon we'll use this as the Package field of the test
JSON output so that we can disambiguate output from differently
configured runs of the same test package, and naturally flow this
through to any test result viewer.

The long-term plan is to use variant names as dist's own test names
and eliminate the ad hoc names it has right now. Unfortunately, the
build coordinator is aware of many of the ad hoc dist test names, so
some more work is needed to get to that point. This CL keeps almost
all test names the same, with the exception of tests registered by
registerCgoTests, where we regularize test names a bit using variants
to avoid some unnecessary complexity (I believe nothing depends on the
names of these tests).

For #37486.

Change-Id: I119fec2872e40b12c1973cf2cddc7f413d62a48c
Reviewed-on: https://go-review.googlesource.com/c/go/+/495016
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

1 files changed, 79 insertions, 33 deletions

diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go
index a8635bed8a..81a4973b19 100644
--- a/src/cmd/dist/test.go
+++ b/src/cmd/dist/test.go
@@ -74,6 +74,8 @@ type tester struct {
 	testNames    map[string]bool
 	timeoutScale int
 
+	variantNames map[string]bool // check that pkg[:variant] names are unique
+
 	worklist []*work
 }
 
@@ -298,6 +300,13 @@ type goTest struct {
 
 	runOnHost bool // When cross-compiling, run this test on the host instead of guest
 
+	// variant, if non-empty, is a name used to distinguish different
+	// configurations of the same test package(s).
+	variant string
+	// sharded indicates that variant is used solely for sharding and that
+	// the set of test names run by each variant of a package is non-overlapping.
+	sharded bool
+
 	// We have both pkg and pkgs as a convenience. Both may be set, in which
 	// case they will be combined. At least one must be set.
 	pkgs []string // Multiple packages to test
@@ -405,13 +414,7 @@ func (opts *goTest) buildArgs(t *tester) (goCmd string, build, run, pkgs, testFl
 		build = append(build, "-buildmode="+opts.buildmode)
 	}
 
-	pkgs = opts.pkgs
-	if opts.pkg != "" {
-		pkgs = append(pkgs[:len(pkgs):len(pkgs)], opts.pkg)
-	}
-	if len(pkgs) == 0 {
-		panic("no packages")
-	}
+	pkgs = opts.packages()
 
 	runOnHost := opts.runOnHost && (goarch != gohostarch || goos != gohostos)
 	needTestFlags := len(opts.testFlags) > 0 || runOnHost
@@ -448,6 +451,19 @@ func (opts *goTest) buildArgs(t *tester) (goCmd string, build, run, pkgs, testFl
 	return
 }
 
+// packages returns the full list of packages to be run by this goTest. This
+// will always include at least one package.
+func (opts *goTest) packages() []string {
+	pkgs := opts.pkgs
+	if opts.pkg != "" {
+		pkgs = append(pkgs[:len(pkgs):len(pkgs)], opts.pkg)
+	}
+	if len(pkgs) == 0 {
+		panic("no packages")
+	}
+	return pkgs
+}
+
 // ranGoTest and stdMatches are state closed over by the stdlib
 // testing func in registerStdTest below. The tests are run
 // sequentially, so there's no need for locks.
@@ -592,12 +608,14 @@ func (t *tester) registerTests() {
 	if !t.compileOnly {
 		t.registerTest("osusergo", "os/user with tag osusergo",
 			&goTest{
+				variant: "osusergo",
 				timeout: 300 * time.Second,
 				tags:    []string{"osusergo"},
 				pkg:     "os/user",
 			})
 		t.registerTest("purego:hash/maphash", "hash/maphash purego implementation",
 			&goTest{
+				variant: "purego",
 				timeout: 300 * time.Second,
 				tags:    []string{"purego"},
 				pkg:     "hash/maphash",
@@ -608,6 +626,7 @@ func (t *tester) registerTests() {
 	if goos == "darwin" && goarch == "amd64" && t.cgoEnabled {
 		t.registerTest("amd64ios", "GOOS=ios on darwin/amd64",
 			&goTest{
+				variant:  "amd64ios",
 				timeout:  300 * time.Second,
 				runTests: "SystemRoots",
 				env:      []string{"GOOS=ios", "CGO_ENABLED=1"},
@@ -619,6 +638,7 @@ func (t *tester) registerTests() {
 	if !t.compileOnly && t.hasParallelism() {
 		t.registerTest("runtime:cpu124", "GOMAXPROCS=2 runtime -cpu=1,2,4 -quick",
 			&goTest{
+				variant:   "cpu124",
 				timeout:   300 * time.Second,
 				cpu:       "1,2,4",
 				short:     true,
@@ -666,6 +686,7 @@ func (t *tester) registerTests() {
 			for _, pkg := range pkgs {
 				t.registerTest(hook+":"+pkg, "maymorestack="+hook,
 					&goTest{
+						variant: hook,
 						timeout: 600 * time.Second,
 						short:   true,
 						env:     []string{"GOFLAGS=" + goFlags},
@@ -704,8 +725,9 @@ func (t *tester) registerTests() {
 			// Run `go test fmt` in the moved GOROOT, without explicitly setting
 			// GOROOT in the environment. The 'go' command should find itself.
 			cmd := (&goTest{
-				goroot: moved,
-				pkg:    "fmt",
+				variant: "moved_goroot",
+				goroot:  moved,
+				pkg:     "fmt",
 			}).command(t)
 			unsetEnv(cmd, "GOROOT")
 			err := cmd.Run()
@@ -739,6 +761,7 @@ func (t *tester) registerTests() {
 		}
 		t.registerTest("nolibgcc:"+pkg, "Testing without libgcc.",
 			&goTest{
+				variant:  "nolibgcc",
 				ldflags:  "-linkmode=internal -libgcc=none",
 				runTests: run,
 				pkg:      pkg,
@@ -753,6 +776,7 @@ func (t *tester) registerTests() {
 	if t.internalLinkPIE() && !disablePIE {
 		t.registerTest("pie_internal", "internal linking of -buildmode=pie",
 			&goTest{
+				variant:   "pie_internal",
 				timeout:   60 * time.Second,
 				buildmode: "pie",
 				ldflags:   "-linkmode=internal",
@@ -763,6 +787,7 @@ func (t *tester) registerTests() {
 		if t.cgoEnabled && t.internalLink() && !disablePIE {
 			t.registerTest("pie_internal_cgo", "internal linking of -buildmode=pie",
 				&goTest{
+					variant:   "pie_internal",
 					timeout:   60 * time.Second,
 					buildmode: "pie",
 					ldflags:   "-linkmode=internal",
@@ -775,6 +800,7 @@ func (t *tester) registerTests() {
 	if t.hasParallelism() {
 		t.registerTest("sync_cpu", "sync -cpu=10",
 			&goTest{
+				variant: "cpu10",
 				timeout: 120 * time.Second,
 				cpu:     "10",
 				pkg:     "sync",
@@ -837,10 +863,13 @@ func (t *tester) registerTests() {
 			nShards = n
 		}
 		for shard := 0; shard < nShards; shard++ {
+			id := fmt.Sprintf("%d_%d", shard, nShards)
 			t.registerTest(
-				fmt.Sprintf("test:%d_%d", shard, nShards),
+				"test:"+id,
 				"../test",
 				&goTest{
+					variant:   id,
+					sharded:   true,
 					pkg:       "cmd/internal/testdir",
 					testFlags: []string{fmt.Sprintf("-shard=%d", shard), fmt.Sprintf("-shards=%d", nShards)},
 					runOnHost: true,
@@ -854,7 +883,7 @@ func (t *tester) registerTests() {
 	// To help developers avoid trybot-only failures, we try to run on typical developer machines
 	// which is darwin,linux,windows/amd64 and darwin/arm64.
 	if goos == "darwin" || ((goos == "linux" || goos == "windows") && goarch == "amd64") {
-		t.registerTest("api", "API check", &goTest{pkg: "cmd/api", timeout: 5 * time.Minute, testFlags: []string{"-check"}})
+		t.registerTest("api", "API check", &goTest{variant: "check", pkg: "cmd/api", timeout: 5 * time.Minute, testFlags: []string{"-check"}})
 	}
 }
 
@@ -895,6 +924,19 @@ func (rtPreFunc) isRegisterTestOpt() {}
 //
 // name must uniquely identify the test and heading must be non-empty.
 func (t *tester) registerTest(name, heading string, test *goTest, opts ...registerTestOpt) {
+	if t.variantNames == nil {
+		t.variantNames = make(map[string]bool)
+	}
+	for _, pkg := range test.packages() {
+		variantName := pkg
+		if test.variant != "" {
+			variantName += ":" + test.variant
+		}
+		if t.variantNames[variantName] {
+			panic("duplicate variant name " + variantName)
+		}
+		t.variantNames[variantName] = true
+	}
 	var preFunc func(*distTest) bool
 	for _, opt := range opts {
 		switch opt := opt.(type) {
@@ -1049,8 +1091,9 @@ func (t *tester) supportedBuildmode(mode string) bool {
 }
 
 func (t *tester) registerCgoTests(heading string) {
-	cgoTest := func(name string, subdir, linkmode, buildmode string, opts ...registerTestOpt) *goTest {
+	cgoTest := func(variant string, subdir, linkmode, buildmode string, opts ...registerTestOpt) *goTest {
 		gt := &goTest{
+			variant:   variant,
 			pkg:       "cmd/cgo/internal/" + subdir,
 			buildmode: buildmode,
 			ldflags:   "-linkmode=" + linkmode,
@@ -1076,18 +1119,18 @@ func (t *tester) registerCgoTests(heading string) {
 			gt.tags = append(gt.tags, "static")
 		}
 
-		t.registerTest("cgo:"+name, heading, gt, opts...)
+		t.registerTest("cgo:"+subdir+":"+variant, heading, gt, opts...)
 		return gt
 	}
 
-	cgoTest("test-auto", "test", "auto", "")
+	cgoTest("auto", "test", "auto", "")
 
 	// Stub out various buildmode=pie tests  on alpine until 54354 resolved.
 	builderName := os.Getenv("GO_BUILDER_NAME")
 	disablePIE := strings.HasSuffix(builderName, "-alpine")
 
 	if t.internalLink() {
-		cgoTest("test-internal", "test", "internal", "")
+		cgoTest("internal", "test", "internal", "")
 	}
 
 	os := gohostos
@@ -1098,24 +1141,24 @@ func (t *tester) registerCgoTests(heading string) {
 			break
 		}
 		// test linkmode=external, but __thread not supported, so skip testtls.
-		cgoTest("test-external", "test", "external", "")
+		cgoTest("external", "test", "external", "")
 
-		gt := cgoTest("test-external-s", "test", "external", "")
+		gt := cgoTest("external-s", "test", "external", "")
 		gt.ldflags += " -s"
 
 		if t.supportedBuildmode("pie") && !disablePIE {
-			cgoTest("test-auto-pie", "test", "auto", "pie")
+			cgoTest("auto-pie", "test", "auto", "pie")
 			if t.internalLink() && t.internalLinkPIE() {
-				cgoTest("test-internal-pie", "test", "internal", "pie")
+				cgoTest("internal-pie", "test", "internal", "pie")
 			}
 		}
 
 	case os == "aix", os == "android", os == "dragonfly", os == "freebsd", os == "linux", os == "netbsd", os == "openbsd":
-		gt := cgoTest("test-external-g0", "test", "external", "")
+		gt := cgoTest("external-g0", "test", "external", "")
 		gt.env = append(gt.env, "CGO_CFLAGS=-g0 -fdiagnostics-color")
 
-		cgoTest("testtls-auto", "testtls", "auto", "")
-		cgoTest("testtls-external", "testtls", "external", "")
+		cgoTest("auto", "testtls", "auto", "")
+		cgoTest("external", "testtls", "external", "")
 		switch {
 		case os == "aix":
 			// no static linking
@@ -1162,30 +1205,30 @@ func (t *tester) registerCgoTests(heading string) {
 			// Static linking tests
 			if goos != "android" && p != "netbsd/arm" {
 				// TODO(#56629): Why does this fail on netbsd-arm?
-				cgoTest("testtls-static", "testtls", "external", "static", staticCheck)
+				cgoTest("static", "testtls", "external", "static", staticCheck)
 			}
-			cgoTest("nocgo-auto", "testnocgo", "auto", "", staticCheck)
-			cgoTest("nocgo-external", "testnocgo", "external", "", staticCheck)
+			cgoTest("auto", "testnocgo", "auto", "", staticCheck)
+			cgoTest("external", "testnocgo", "external", "", staticCheck)
 			if goos != "android" {
-				cgoTest("nocgo-static", "testnocgo", "external", "static", staticCheck)
-				cgoTest("test-static", "test", "external", "static", staticCheck)
+				cgoTest("static", "testnocgo", "external", "static", staticCheck)
+				cgoTest("static", "test", "external", "static", staticCheck)
 				// -static in CGO_LDFLAGS triggers a different code path
 				// than -static in -extldflags, so test both.
 				// See issue #16651.
 				if goarch != "loong64" {
 					// TODO(#56623): Why does this fail on loong64?
-					cgoTest("test-static-env", "test", "auto", "static", staticCheck)
+					cgoTest("auto-static", "test", "auto", "static", staticCheck)
 				}
 			}
 
 			// PIE linking tests
 			if t.supportedBuildmode("pie") && !disablePIE {
-				cgoTest("test-pie", "test", "auto", "pie")
+				cgoTest("auto-pie", "test", "auto", "pie")
 				if t.internalLink() && t.internalLinkPIE() {
-					cgoTest("test-pie-internal", "test", "internal", "pie")
+					cgoTest("internal-pie", "test", "internal", "pie")
 				}
-				cgoTest("testtls-pie", "testtls", "auto", "pie")
-				cgoTest("nocgo-pie", "testnocgo", "auto", "pie")
+				cgoTest("auto-pie", "testtls", "auto", "pie")
+				cgoTest("auto-pie", "testnocgo", "auto", "pie")
 			}
 		}
 	}
@@ -1325,12 +1368,14 @@ func (t *tester) registerRaceTests() {
 	hdr := "Testing race detector"
 	t.registerTest("race:runtime/race", hdr,
 		&goTest{
+			variant:  "race",
 			race:     true,
 			runTests: "Output",
 			pkg:      "runtime/race",
 		})
 	t.registerTest("race", hdr,
 		&goTest{
+			variant:  "race",
 			race:     true,
 			runTests: "TestParse|TestEcho|TestStdinCloseRace|TestClosedPipeRace|TestTypeRace|TestFdRace|TestFdReadRace|TestFileCloseRace",
 			pkgs:     []string{"flag", "net", "os", "os/exec", "encoding/gob"},
@@ -1345,12 +1390,13 @@ func (t *tester) registerRaceTests() {
 		// There are already cgo-enabled packages being tested with the race detector.
 		// We shouldn't need to redo all of cmd/cgo/internal/test too.
 		// The race buildler will take care of this.
-		// t.registerTest("race:cmd/cgo/internal/test", hdr, &goTest{dir: "cmd/cgo/internal/test", race: true, env: []string{"GOTRACEBACK=2"}})
+		// t.registerTest("race:cmd/cgo/internal/test", hdr, &goTest{variant:"race", dir: "cmd/cgo/internal/test", race: true, env: []string{"GOTRACEBACK=2"}})
 	}
 	if t.extLink() {
 		// Test with external linking; see issue 9133.
 		t.registerTest("race:external", hdr,
 			&goTest{
+				variant:  "race-external",
 				race:     true,
 				ldflags:  "-linkmode=external",
 				runTests: "TestParse|TestEcho|TestStdinCloseRace",