Skip to content

Commit

Permalink
cmd/coordinator: find inner modules when testing repos
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@dmitshur @codebien
dmitshur authored and codebien committed Nov 13, 2019
1 parent cc2af8b commit 683a250
Show file tree
Hide file tree
Showing 2 changed files with 116 additions and 19 deletions.
14 changes: 13 additions & 1 deletion buildlet/buildletclient.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,7 @@ func (de DirEntry) String() string {
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 {
Expand All @@ -704,7 +705,7 @@ func (de DirEntry) Name() string {
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 {
Expand All @@ -713,6 +714,17 @@ func (de DirEntry) Perm() string {
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 {
Expand Down
121 changes: 103 additions & 18 deletions cmd/coordinator/coordinator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2372,11 +2372,17 @@ 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) + "/...",
}
// 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.
Expand All @@ -2389,7 +2395,34 @@ func (st *buildStatus) runSubrepoTests() (remoteErr, err error) {

// 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")
Expand Down Expand Up @@ -2496,12 +2529,12 @@ func (st *buildStatus) runSubrepoTests() (remoteErr, err error) {
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)
}
}
}
Expand All @@ -2525,16 +2558,41 @@ func (st *buildStatus) runSubrepoTests() (remoteErr, err error) {
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
Expand All @@ -2561,6 +2619,34 @@ func (st *buildStatus) goMod(importPath, goroot, gopath string) (string, error)
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 {
Expand Down Expand Up @@ -3414,7 +3500,6 @@ func (st *buildStatus) writeEventsLocked(w io.Writer, htmlMode bool) {
if st.isRunningLocked() {
fmt.Fprintf(w, " %7s (now)\n", fmt.Sprintf("+%0.1fs", time.Since(lastT).Seconds()))
}

}

func (st *buildStatus) logs() string {
Expand Down

0 comments on commit 683a250

Please sign in to comment.