cmd/coordinator: find inner modules when testing repos
Previously, we were invoking a single 'go test' run at the repository
root with the import path pattern of 'golang.org/x/{repo}/...'. This
pattern does not match packages that are located in nested modules
in the repository.
Look for go.mod files in all subdirectories of the repository to find
all inner modules. Then, run 'go test' inside each module root, thus
testing all packages in all modules of the repository. If one of the
test invocations fails, keep testing others, and report all failures.
When looking for inner modules, consider only those that have module
path that would not be ignored by the go tool and aren't vendored.
This way, go.mod files inside testdata directories aren't treated as
if they're proper modules.
This is being done only when the tests are running in module mode,
since module boundaries don't exist in GOPATH mode.
Fixes golang/go#32528
Change-Id: I9f8558982885c9955d3b34127c80c485d713b380
Reviewed-on: https://go-review.googlesource.com/c/build/+/194559
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/buildlet/buildletclient.go b/buildlet/buildletclient.go
index ce5a59b..02b8000 100644
--- a/buildlet/buildletclient.go
+++ b/buildlet/buildletclient.go
@@ -696,6 +696,7 @@
return de.line
}
+// Name returns the relative path to the file, such as "src/net/http/" or "src/net/http/jar.go".
func (de DirEntry) Name() string {
f := strings.Split(de.line, "\t")
if len(f) < 2 {
@@ -704,7 +705,7 @@
return f[1]
}
-// Perm returns the permission bits in string form, e.g., "drw-rw-rw".
+// Perm returns the permission bits in string form, such as "-rw-r--r--" or "drwxr-xr-x".
func (de DirEntry) Perm() string {
i := strings.IndexByte(de.line, '\t')
if i == -1 {
@@ -713,6 +714,17 @@
return de.line[:i]
}
+// IsDir reports whether de describes a directory. That is,
+// it tests for the os.ModeDir bit being set in de.Perm().
+func (de DirEntry) IsDir() bool {
+ if len(de.line) == 0 {
+ return false
+ }
+ return de.line[0] == 'd'
+}
+
+// Digest returns the SHA-1 digest of the file, such as "da39a3ee5e6b4b0d3255bfef95601890afd80709".
+// It returns the empty string if the digest isn't included.
func (de DirEntry) Digest() string {
f := strings.Split(de.line, "\t")
if len(f) < 5 {
diff --git a/cmd/coordinator/coordinator.go b/cmd/coordinator/coordinator.go
index d7bd72e..70fc8b0 100644
--- a/cmd/coordinator/coordinator.go
+++ b/cmd/coordinator/coordinator.go
@@ -2372,11 +2372,17 @@
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) + "/...",
+ // A goTestRun represents a single invocation of the 'go test' command.
+ type goTestRun struct {
+ Dir string // Directory where 'go test' should be executed.
+ Patterns []string // Import path patterns to provide to 'go test'.
}
+ // The default behavior is to test the pattern "golang.org/x/{repo}/..."
+ // in the repository root.
+ testRuns := []goTestRun{{
+ Dir: "gopath/src/" + importPathOfRepo(st.SubName),
+ Patterns: []string{importPathOfRepo(st.SubName) + "/..."},
+ }}
// The next large step diverges into two code paths,
// one for module mode and another for GOPATH mode.
@@ -2389,7 +2395,34 @@
// No need to place the repo's dependencies into a GOPATH workspace.
- // TODO(dmitshur): Look for inner modules, test them too. See golang.org/issue/32528.
+ // Look for inner modules, in order to test them too. See golang.org/issue/32528.
+ repoPath := importPathOfRepo(st.SubName)
+ sp := st.CreateSpan("listing_subrepo_modules", st.SubName)
+ err = st.bc.ListDir("gopath/src/"+repoPath, buildlet.ListDirOpts{Recursive: true}, func(e buildlet.DirEntry) {
+ goModFile := path.Base(e.Name()) == "go.mod" && !e.IsDir()
+ if !goModFile {
+ return
+ }
+ // Found a go.mod file in a subdirectory, which indicates the root of a module.
+ modulePath := path.Join(repoPath, path.Dir(e.Name()))
+ if modulePath == repoPath {
+ // This is the go.mod file at the repository root.
+ // It's already a part of testRuns, so skip it.
+ return
+ } else if ignoredByGoTool(modulePath) || isVendored(modulePath) {
+ // go.mod file is in a directory we're not looking to support, so skip it.
+ return
+ }
+ // Add an additional test run entry that will test this entire module.
+ testRuns = append(testRuns, goTestRun{
+ Dir: "gopath/src/" + modulePath,
+ Patterns: []string{modulePath + "/..."},
+ })
+ })
+ sp.Done(err)
+ if err != nil {
+ return nil, err
+ }
} else {
fmt.Fprintf(st, "testing in GOPATH mode\n\n")
@@ -2496,12 +2529,12 @@
fmt.Fprintf(st, "go list error: %v\noutput:\n%s", rErr, &buf)
return rErr, nil
}
- patterns = nil
+ testRuns[0].Patterns = nil
for _, importPath := range strings.Fields(buf.String()) {
if !st.conf.ShouldTestPackageInGOPATHMode(importPath) {
continue
}
- patterns = append(patterns, importPath)
+ testRuns[0].Patterns = append(testRuns[0].Patterns, importPath)
}
}
}
@@ -2525,16 +2558,41 @@
if st.conf.IsRace() {
args = append(args, "-race")
}
- 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
- Output: st,
- Dir: "gopath/src/" + importPathOfRepo(st.SubName),
- ExtraEnv: env,
- Path: []string{"$WORKDIR/go/bin", "$PATH"},
- Args: args,
- })
+ var remoteErrors []error
+ for _, tr := range testRuns {
+ rErr, err := st.bc.Exec(path.Join("go", "bin", "go"), buildlet.ExecOpts{
+ Debug: true, // make buildlet print extra debug in output for failures
+ Output: st,
+ Dir: tr.Dir,
+ ExtraEnv: env,
+ Path: []string{"$WORKDIR/go/bin", "$PATH"},
+ Args: append(args, tr.Patterns...),
+ })
+ if err != nil {
+ // A network/communication error. Give up here;
+ // the caller can retry as it sees fit.
+ return nil, err
+ } else if rErr != nil {
+ // An error occurred remotely and is terminal, but we want to
+ // keep testing other packages and report their failures too,
+ // rather than stopping short.
+ remoteErrors = append(remoteErrors, rErr)
+ }
+ }
+ if len(remoteErrors) == 1 {
+ return remoteErrors[0], nil
+ } else if len(remoteErrors) > 1 {
+ var b strings.Builder
+ for i, e := range remoteErrors {
+ if i != 0 {
+ b.WriteString("; ")
+ }
+ b.WriteString(e.Error())
+ }
+ return errors.New(b.String()), nil
+ }
+ return nil, nil
}
// goMod determines and reports the value of go env GOMOD
@@ -2561,6 +2619,34 @@
return env.GoMod, err
}
+// ignoredByGoTool reports whether the given import path corresponds
+// to a directory that would be ignored by the go tool.
+//
+// The logic of the go tool for ignoring directories is documented at
+// https://golang.org/cmd/go/#hdr-Package_lists_and_patterns:
+//
+// Directory and file names that begin with "." or "_" are ignored
+// by the go tool, as are directories named "testdata".
+//
+func ignoredByGoTool(importPath string) bool {
+ for _, el := range strings.Split(importPath, "/") {
+ if strings.HasPrefix(el, ".") || strings.HasPrefix(el, "_") || el == "testdata" {
+ return true
+ }
+ }
+ return false
+}
+
+// isVendored reports whether the given import path corresponds
+// to a Go package that is inside a vendor directory.
+//
+// The logic for what is considered a vendor directory is documented at
+// https://golang.org/cmd/go/#hdr-Vendor_Directories.
+func isVendored(importPath string) bool {
+ return strings.HasPrefix(importPath, "vendor/") ||
+ strings.Contains(importPath, "/vendor/")
+}
+
// firstNonNil returns the first non-nil error, or nil otherwise.
func firstNonNil(errs ...error) error {
for _, e := range errs {
@@ -3414,7 +3500,6 @@
if st.isRunningLocked() {
fmt.Fprintf(w, " %7s (now)\n", fmt.Sprintf("+%0.1fs", time.Since(lastT).Seconds()))
}
-
}
func (st *buildStatus) logs() string {
