dashboard, cmd/coordinator: add controls to skip subrepo dirs in GOPATH mode

This change adds controls to the dashboard package that makes it
possible to skip certain directories in subrepos from being tested
in GOPATH mode.

It uses this control mechanism to skip the gopls directory in x/tools
from being tested in GOPATH mode. That directory and everything inside
is developed with support for module mode only. All other packages
in x/tools continue to be supported both in module and GOPATH modes
for the current and previous releases, as per release policy¹.

¹ https://golang.org/doc/devel/release.html#policy

Fixes golang/go#34190

Change-Id: Icdb4d0e1419f69c84c9a3f9a33727a09a35599f4
Reviewed-on: https://go-review.googlesource.com/c/build/+/194397
Run-TryBot: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: dmitshur

cmd/coordinator/coordinator.go

@@ -2372,6 +2372,12 @@ func (st *buildStatus) runSubrepoTests() (remoteErr, err error) {
 		return nil, fmt.Errorf("failed to determine go env GOMOD value: %v", err)
 	}
 
+	// patterns defines import path patterns to provide to 'go test'.
+	// The starting default value is "golang.org/x/{repo}/...".
+	patterns := []string{
+		importPathOfRepo(st.SubName) + "/...",
+	}
+
 	// The next large step diverges into two code paths,
 	// one for module mode and another for GOPATH mode.
 	//
@@ -2419,7 +2425,7 @@ func (st *buildStatus) runSubrepoTests() (remoteErr, err error) {
 				return nil, fmt.Errorf("exec go list on buildlet: %v", err)
 			}
 			if rErr != nil {
-				fmt.Fprintf(st, "go list error:\n%s", &buf)
+				fmt.Fprintf(st, "go list error: %v\noutput:\n%s", rErr, &buf)
 				return rErr, nil
 			}
 			for _, p := range strings.Fields(buf.String()) {
@@ -2469,7 +2475,35 @@ func (st *buildStatus) runSubrepoTests() (remoteErr, err error) {
 			}
 		}
 
-		// TODO(dmitshur): Allow dashboard to filter out directories to test. See golang.org/issue/34190.
+		// The dashboard offers control over what packages to test in GOPATH mode.
+		// Compute a list of packages by calling 'go list'. See golang.org/issue/34190.
+		{
+			repoPath := importPathOfRepo(st.SubName)
+			var buf bytes.Buffer
+			sp := st.CreateSpan("listing_subrepo_packages", st.SubName)
+			rErr, err := st.bc.Exec(path.Join("go", "bin", "go"), buildlet.ExecOpts{
+				Output:   &buf,
+				Dir:      "gopath/src/" + repoPath,
+				ExtraEnv: append(st.conf.Env(), "GOROOT="+goroot, "GOPATH="+gopath),
+				Path:     []string{"$WORKDIR/go/bin", "$PATH"},
+				Args:     []string{"list", repoPath + "/..."},
+			})
+			sp.Done(firstNonNil(err, rErr))
+			if err != nil {
+				return nil, fmt.Errorf("exec go list on buildlet: %v", err)
+			}
+			if rErr != nil {
+				fmt.Fprintf(st, "go list error: %v\noutput:\n%s", rErr, &buf)
+				return rErr, nil
+			}
+			patterns = nil
+			for _, importPath := range strings.Fields(buf.String()) {
+				if !st.conf.ShouldTestPackageInGOPATHMode(importPath) {
+					continue
+				}
+				patterns = append(patterns, importPath)
+			}
+		}
 	}
 
 	// TODO(dmitshur): For some subrepos, test in both modes. See golang.org/issue/30233.
@@ -2491,7 +2525,7 @@ func (st *buildStatus) runSubrepoTests() (remoteErr, err error) {
 	if st.conf.IsRace() {
 		args = append(args, "-race")
 	}
-	args = append(args, importPathOfRepo(st.SubName)+"/...")
+	args = append(args, patterns...)
 
 	return st.bc.Exec(path.Join("go", "bin", "go"), buildlet.ExecOpts{
 		Debug:    true, // make buildlet print extra debug in output for failures
@@ -2527,6 +2561,16 @@ func (st *buildStatus) goMod(importPath, goroot, gopath string) (string, error)
 	return env.GoMod, err
 }
 
+// firstNonNil returns the first non-nil error, or nil otherwise.
+func firstNonNil(errs ...error) error {
+	for _, e := range errs {
+		if e != nil {
+			return e
+		}
+	}
+	return nil
+}
+
 // moduleProxy returns the GOPROXY environment value to use for module-enabled
 // tests.
 //

dashboard/builders.go

@@ -733,7 +733,30 @@ func (c *BuildConfig) ModulesEnv(repo string) (env []string) {
 	case "oauth2", "build", "perf", "website":
 		env = append(env, "GO111MODULE=on")
 	}
-	return
+	return env
+}
+
+// ShouldTestPackageInGOPATHMode is used to control whether the package
+// with the specified import path should be tested in GOPATH mode.
+//
+// When running tests for all golang.org/* repositories in GOPATH mode,
+// this method is called repeatedly with the full import path of each
+// package that is found and is being considered for testing in GOPATH
+// mode. It's not used and has no effect on import paths in the main
+// "go" repository. It has no effect on tests done in module mode.
+//
+// When considering making changes here, keep the release policy in mind:
+//
+// 	https://golang.org/doc/devel/release.html#policy
+//
+func (*BuildConfig) ShouldTestPackageInGOPATHMode(importPath string) bool {
+	if importPath == "golang.org/x/tools/gopls" ||
+		strings.HasPrefix(importPath, "golang.org/x/tools/gopls/") {
+		// Don't test golang.org/x/tools/gopls/... in GOPATH mode.
+		return false
+	}
+	// Test everything else in GOPATH mode as usual.
+	return true
 }
 
 func (c *BuildConfig) IsReverse() bool { return c.hostConf().IsReverse }

dashboard/builders_test.go

@@ -595,3 +595,30 @@ func TestShouldRunDistTest(t *testing.T) {
 		}
 	}
 }
+
+func TestShouldTestPackageInGOPATHMode(t *testing.T) {
+	// This function doesn't change behavior depending on the builder
+	// at this time, so just use a common one.
+	bc, ok := Builders["linux-amd64"]
+	if !ok {
+		t.Fatal("unknown builder")
+	}
+
+	tests := []struct {
+		importPath string
+		want       bool
+	}{
+		{"golang.org/x/image/bmp", true},
+		{"golang.org/x/tools/go/ast/astutil", true},
+		{"golang.org/x/tools/go/packages", true},
+		{"golang.org/x/tools", true}, // Three isn't a package there, but if there was, it should be tested.
+		{"golang.org/x/tools/gopls", false},
+		{"golang.org/x/tools/gopls/internal/foobar", false},
+	}
+	for _, tt := range tests {
+		got := bc.ShouldTestPackageInGOPATHMode(tt.importPath)
+		if got != tt.want {
+			t.Errorf("ShouldTestPackageInGOPATHMode(%q) = %v; want %v", tt.importPath, got, tt.want)
+		}
+	}
+}