go/packages: refactor list driver

Many functions in the list driver took the Config and goInfo functions.
Consolidate into a state object and move most functions to methods on
it.

Rework asynchronous go command calls -- rather than always running them
and waiting for them to finish, run on demand and attach them all to a
Context that is cancelled when the function returns. This does risk
letting them run a tiny bit over, but it should be harmless, and it's a
lot easier to pass the right Context around than to try and wait for
everything.

Propagate errors that were previously swallowed. All the tests still
pass, and hopefully nobody is hitting the errors cases. If I'm wrong we
can start swallowing them again.

Tweak and reword various comments that I thought could be improved.

Change-Id: I78457dd5493de3e9cb97f17dfe222d7cc9a478e9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215938
Reviewed-by: Michael Matloob <matloob@golang.org>

3 files changed, 231 insertions, 204 deletions

diff --git a/go/packages/golist.go b/go/packages/golist.go
index 5f5f49796..31d669765 100644
--- a/go/packages/golist.go
+++ b/go/packages/golist.go
@@ -6,6 +6,7 @@ package packages
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"fmt"
 	"go/types"
@@ -40,16 +41,21 @@ type responseDeduper struct {
 	dr           *driverResponse
 }
 
-// init fills in r with a driverResponse.
-func (r *responseDeduper) init(dr *driverResponse) {
-	r.dr = dr
-	r.seenRoots = map[string]bool{}
-	r.seenPackages = map[string]*Package{}
+func newDeduper() *responseDeduper {
+	return &responseDeduper{
+		dr:           &driverResponse{},
+		seenRoots:    map[string]bool{},
+		seenPackages: map[string]*Package{},
+	}
+}
+
+// addAll fills in r with a driverResponse.
+func (r *responseDeduper) addAll(dr *driverResponse) {
 	for _, pkg := range dr.Packages {
-		r.seenPackages[pkg.ID] = pkg
+		r.addPackage(pkg)
 	}
 	for _, root := range dr.Roots {
-		r.seenRoots[root] = true
+		r.addRoot(root)
 	}
 }
 
@@ -69,25 +75,44 @@ func (r *responseDeduper) addRoot(id string) {
 	r.dr.Roots = append(r.dr.Roots, id)
 }
 
-// goInfo contains global information from the go tool.
-type goInfo struct {
-	rootDirs map[string]string
-	env      goEnv
+type golistState struct {
+	cfg *Config
+	ctx context.Context
+
+	envOnce    sync.Once
+	goEnvError error
+	goEnv      map[string]string
+
+	rootsOnce     sync.Once
+	rootDirsError error
+	rootDirs      map[string]string
 }
 
-type goEnv struct {
-	modulesOn bool
+// getEnv returns Go environment variables. Only specific variables are
+// populated -- computing all of them is slow.
+func (state *golistState) getEnv() (map[string]string, error) {
+	state.envOnce.Do(func() {
+		var b *bytes.Buffer
+		b, state.goEnvError = state.invokeGo("env", "-json", "GOMOD", "GOPATH")
+		if state.goEnvError != nil {
+			return
+		}
+
+		state.goEnv = make(map[string]string)
+		decoder := json.NewDecoder(b)
+		if state.goEnvError = decoder.Decode(&state.goEnv); state.goEnvError != nil {
+			return
+		}
+	})
+	return state.goEnv, state.goEnvError
 }
 
-func determineEnv(cfg *Config) goEnv {
-	buf, err := invokeGo(cfg, "env", "GOMOD")
+// mustGetEnv is a convenience function that can be used if getEnv has already succeeded.
+func (state *golistState) mustGetEnv() map[string]string {
+	env, err := state.getEnv()
 	if err != nil {
-		return goEnv{}
+		panic(fmt.Sprintf("mustGetEnv: %v", err))
 	}
-	gomod := bytes.TrimSpace(buf.Bytes())
-
-	env := goEnv{}
-	env.modulesOn = len(gomod) > 0
 	return env
 }
 
@@ -95,42 +120,33 @@ func determineEnv(cfg *Config) goEnv {
 // the build system package structure.
 // See driver for more details.
 func goListDriver(cfg *Config, patterns ...string) (*driverResponse, error) {
-	var sizes types.Sizes
+	// Make sure that any asynchronous go commands are killed when we return.
+	parentCtx := cfg.Context
+	if parentCtx == nil {
+		parentCtx = context.Background()
+	}
+	ctx, cancel := context.WithCancel(parentCtx)
+	defer cancel()
+
+	response := newDeduper()
+
+	// Fill in response.Sizes asynchronously if necessary.
 	var sizeserr error
 	var sizeswg sync.WaitGroup
 	if cfg.Mode&NeedTypesSizes != 0 || cfg.Mode&NeedTypes != 0 {
 		sizeswg.Add(1)
 		go func() {
-			sizes, sizeserr = getSizes(cfg)
+			var sizes types.Sizes
+			sizes, sizeserr = packagesdriver.GetSizesGolist(ctx, cfg.BuildFlags, cfg.Env, cfg.Dir, usesExportData(cfg))
+			// types.SizesFor always returns nil or a *types.StdSizes.
+			response.dr.Sizes, _ = sizes.(*types.StdSizes)
 			sizeswg.Done()
 		}()
 	}
-	defer sizeswg.Wait()
-
-	// start fetching rootDirs
-	var info goInfo
-	var rootDirsReady, envReady = make(chan struct{}), make(chan struct{})
-	go func() {
-		info.rootDirs = determineRootDirs(cfg)
-		close(rootDirsReady)
-	}()
-	go func() {
-		info.env = determineEnv(cfg)
-		close(envReady)
-	}()
-	getGoInfo := func() *goInfo {
-		<-rootDirsReady
-		<-envReady
-		return &info
-	}
-
-	// Ensure that we don't leak goroutines: Load is synchronous, so callers will
-	// not expect it to access the fields of cfg after the call returns.
-	defer getGoInfo()
 
-	// always pass getGoInfo to golistDriver
-	golistDriver := func(cfg *Config, patterns ...string) (*driverResponse, error) {
-		return golistDriver(cfg, getGoInfo, patterns...)
+	state := &golistState{
+		cfg: cfg,
+		ctx: ctx,
 	}
 
 	// Determine files requested in contains patterns
@@ -165,46 +181,34 @@ extractQueries:
 		}
 	}
 
-	response := &responseDeduper{}
-	var err error
-
 	// See if we have any patterns to pass through to go list. Zero initial
 	// patterns also requires a go list call, since it's the equivalent of
 	// ".".
 	if len(restPatterns) > 0 || len(patterns) == 0 {
-		dr, err := golistDriver(cfg, restPatterns...)
+		dr, err := state.createDriverResponse(restPatterns...)
 		if err != nil {
 			return nil, err
 		}
-		response.init(dr)
-	} else {
-		response.init(&driverResponse{})
+		response.addAll(dr)
 	}
 
-	sizeswg.Wait()
-	if sizeserr != nil {
-		return nil, sizeserr
-	}
-	// types.SizesFor always returns nil or a *types.StdSizes
-	response.dr.Sizes, _ = sizes.(*types.StdSizes)
-
-	var containsCandidates []string
-
 	if len(containFiles) != 0 {
-		if err := runContainsQueries(cfg, golistDriver, response, containFiles, getGoInfo); err != nil {
+		if err := state.runContainsQueries(response, containFiles); err != nil {
 			return nil, err
 		}
 	}
 
-	modifiedPkgs, needPkgs, err := processGolistOverlay(cfg, response, getGoInfo)
+	modifiedPkgs, needPkgs, err := state.processGolistOverlay(response)
 	if err != nil {
 		return nil, err
 	}
+
+	var containsCandidates []string
 	if len(containFiles) > 0 {
 		containsCandidates = append(containsCandidates, modifiedPkgs...)
 		containsCandidates = append(containsCandidates, needPkgs...)
 	}
-	if err := addNeededOverlayPackages(cfg, golistDriver, response, needPkgs, getGoInfo); err != nil {
+	if err := state.addNeededOverlayPackages(response, needPkgs); err != nil {
 		return nil, err
 	}
 	// Check candidate packages for containFiles.
@@ -233,28 +237,32 @@ extractQueries:
 		}
 	}
 
+	sizeswg.Wait()
+	if sizeserr != nil {
+		return nil, sizeserr
+	}
 	return response.dr, nil
 }
 
-func addNeededOverlayPackages(cfg *Config, driver driver, response *responseDeduper, pkgs []string, getGoInfo func() *goInfo) error {
+func (state *golistState) addNeededOverlayPackages(response *responseDeduper, pkgs []string) error {
 	if len(pkgs) == 0 {
 		return nil
 	}
-	dr, err := driver(cfg, pkgs...)
+	dr, err := state.createDriverResponse(pkgs...)
 	if err != nil {
 		return err
 	}
 	for _, pkg := range dr.Packages {
 		response.addPackage(pkg)
 	}
-	_, needPkgs, err := processGolistOverlay(cfg, response, getGoInfo)
+	_, needPkgs, err := state.processGolistOverlay(response)
 	if err != nil {
 		return err
 	}
-	return addNeededOverlayPackages(cfg, driver, response, needPkgs, getGoInfo)
+	return state.addNeededOverlayPackages(response, needPkgs)
 }
 
-func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, queries []string, goInfo func() *goInfo) error {
+func (state *golistState) runContainsQueries(response *responseDeduper, queries []string) error {
 	for _, query := range queries {
 		// TODO(matloob): Do only one query per directory.
 		fdir := filepath.Dir(query)
@@ -264,13 +272,13 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q
 		if err != nil {
 			return fmt.Errorf("could not determine absolute path of file= query path %q: %v", query, err)
 		}
-		dirResponse, err := driver(cfg, pattern)
+		dirResponse, err := state.createDriverResponse(pattern)
 		if err != nil || (len(dirResponse.Packages) == 1 && len(dirResponse.Packages[0].Errors) == 1) {
 			// There was an error loading the package. Try to load the file as an ad-hoc package.
 			// Usually the error will appear in a returned package, but may not if we're in modules mode
 			// and the ad-hoc is located outside a module.
 			var queryErr error
-			dirResponse, queryErr = driver(cfg, query)
+			dirResponse, queryErr = state.createDriverResponse(query)
 			if queryErr != nil {
 				// Return the original error if the attempt to fall back failed.
 				return err
@@ -294,7 +302,7 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q
 				if len(dirResponse.Packages[0].GoFiles) == 0 {
 					filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath
 					// TODO(matloob): check if the file is outside of a root dir?
-					for path := range cfg.Overlay {
+					for path := range state.cfg.Overlay {
 						if path == filename {
 							dirResponse.Packages[0].Errors = nil
 							dirResponse.Packages[0].GoFiles = []string{path}
@@ -329,10 +337,6 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q
 	return nil
 }
 
-func getSizes(cfg *Config) (types.Sizes, error) {
-	return packagesdriver.GetSizesGolist(cfg.Context, cfg.BuildFlags, cfg.Env, cfg.Dir, usesExportData(cfg))
-}
-
 // Fields must match go list;
 // see $GOROOT/src/cmd/go/internal/load/pkg.go.
 type jsonPackage struct {
@@ -375,10 +379,9 @@ func otherFiles(p *jsonPackage) [][]string {
 	return [][]string{p.CFiles, p.CXXFiles, p.MFiles, p.HFiles, p.FFiles, p.SFiles, p.SwigFiles, p.SwigCXXFiles, p.SysoFiles}
 }
 
-// golistDriver uses the "go list" command to expand the pattern
-// words and return metadata for the specified packages. dir may be
-// "" and env may be nil, as per os/exec.Command.
-func golistDriver(cfg *Config, rootsDirs func() *goInfo, words ...string) (*driverResponse, error) {
+// createDriverResponse uses the "go list" command to expand the pattern
+// words and return a response for the specified packages.
+func (state *golistState) createDriverResponse(words ...string) (*driverResponse, error) {
 	// go list uses the following identifiers in ImportPath and Imports:
 	//
 	// 	"p"			-- importable package or main (command)
@@ -392,7 +395,7 @@ func golistDriver(cfg *Config, rootsDirs func() *goInfo, words ...string) (*driv
 
 	// Run "go list" for complete
 	// information on the specified packages.
-	buf, err := invokeGo(cfg, "list", golistargs(cfg, words)...)
+	buf, err := state.invokeGo("list", golistargs(state.cfg, words)...)
 	if err != nil {
 		return nil, err
 	}
@@ -427,7 +430,10 @@ func golistDriver(cfg *Config, rootsDirs func() *goInfo, words ...string) (*driv
 		// contained in a known module or GOPATH entry. This will allow the package to be
 		// properly "reclaimed" when overlays are processed.
 		if filepath.IsAbs(p.ImportPath) && p.Error != nil {
-			pkgPath, ok := getPkgPath(cfg, p.ImportPath, rootsDirs)
+			pkgPath, ok, err := state.getPkgPath(p.ImportPath)
+			if err != nil {
+				return nil, err
+			}
 			if ok {
 				p.ImportPath = pkgPath
 			}
@@ -544,22 +550,20 @@ func golistDriver(cfg *Config, rootsDirs func() *goInfo, words ...string) (*driv
 }
 
 // getPkgPath finds the package path of a directory if it's relative to a root directory.
-func getPkgPath(cfg *Config, dir string, goInfo func() *goInfo) (string, bool) {
+func (state *golistState) getPkgPath(dir string) (string, bool, error) {
 	absDir, err := filepath.Abs(dir)
 	if err != nil {
-		cfg.Logf("error getting absolute path of %s: %v", dir, err)
-		return "", false
+		return "", false, err
 	}
-	for rdir, rpath := range goInfo().rootDirs {
-		absRdir, err := filepath.Abs(rdir)
-		if err != nil {
-			cfg.Logf("error getting absolute path of %s: %v", rdir, err)
-			continue
-		}
+	roots, err := state.determineRootDirs()
+	if err != nil {
+		return "", false, err
+	}
+
+	for rdir, rpath := range roots {
 		// Make sure that the directory is in the module,
 		// to avoid creating a path relative to another module.
-		if !strings.HasPrefix(absDir, absRdir) {
-			cfg.Logf("%s does not have prefix %s", absDir, absRdir)
+		if !strings.HasPrefix(absDir, rdir) {
 			continue
 		}
 		// TODO(matloob): This doesn't properly handle symlinks.
@@ -574,11 +578,11 @@ func getPkgPath(cfg *Config, dir string, goInfo func() *goInfo) (string, bool) {
 			// Once the file is saved, gopls, or the next invocation of the tool will get the correct
 			// result straight from golist.
 			// TODO(matloob): Implement module tiebreaking?
-			return path.Join(rpath, filepath.ToSlash(r)), true
+			return path.Join(rpath, filepath.ToSlash(r)), true, nil
 		}
-		return filepath.ToSlash(r), true
+		return filepath.ToSlash(r), true, nil
 	}
-	return "", false
+	return "", false, nil
 }
 
 // absJoin absolutizes and flattens the lists of files.
@@ -613,13 +617,15 @@ func golistargs(cfg *Config, words []string) []string {
 }
 
 // invokeGo returns the stdout of a go command invocation.
-func invokeGo(cfg *Config, verb string, args ...string) (*bytes.Buffer, error) {
+func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer, error) {
+	cfg := state.cfg
+
 	stdout := new(bytes.Buffer)
 	stderr := new(bytes.Buffer)
 	goArgs := []string{verb}
 	goArgs = append(goArgs, cfg.BuildFlags...)
 	goArgs = append(goArgs, args...)
-	cmd := exec.CommandContext(cfg.Context, "go", goArgs...)
+	cmd := exec.CommandContext(state.ctx, "go", goArgs...)
 	// On darwin the cwd gets resolved to the real path, which breaks anything that
 	// expects the working directory to keep the original path, including the
 	// go command when dealing with modules.
@@ -631,7 +637,7 @@ func invokeGo(cfg *Config, verb string, args ...string) (*bytes.Buffer, error) {
 	cmd.Stdout = stdout
 	cmd.Stderr = stderr
 	defer func(start time.Time) {
-		cfg.Logf("%s for %v, stderr: <<%s>> stdout: <<%s>>\n", time.Since(start), cmdDebugStr(cmd, args...), stderr, stdout)
+		cfg.Logf("%s for %v, stderr: <<%s>> stdout: <<%s>>\n", time.Since(start), cmdDebugStr(cmd, goArgs...), stderr, stdout)
 	}(time.Now())
 
 	if err := cmd.Run(); err != nil {
diff --git a/go/packages/golist_overlay.go b/go/packages/golist_overlay.go
index a7de62299..bc73fd69e 100644
--- a/go/packages/golist_overlay.go
+++ b/go/packages/golist_overlay.go
@@ -1,7 +1,6 @@
 package packages
 
 import (
-	"bytes"
 	"encoding/json"
 	"fmt"
 	"go/parser"
@@ -16,7 +15,7 @@ import (
 // sometimes incorrect.
 // TODO(matloob): Handle unsupported cases, including the following:
 // - determining the correct package to add given a new import path
-func processGolistOverlay(cfg *Config, response *responseDeduper, rootDirs func() *goInfo) (modifiedPkgs, needPkgs []string, err error) {
+func (state *golistState) processGolistOverlay(response *responseDeduper) (modifiedPkgs, needPkgs []string, err error) {
 	havePkgs := make(map[string]string) // importPath -> non-test package ID
 	needPkgsSet := make(map[string]bool)
 	modifiedPkgsSet := make(map[string]bool)
@@ -34,7 +33,7 @@ func processGolistOverlay(cfg *Config, response *responseDeduper, rootDirs func(
 	// potentially modifying the transitive set of dependencies).
 	var overlayAddsImports bool
 
-	for opath, contents := range cfg.Overlay {
+	for opath, contents := range state.cfg.Overlay {
 		base := filepath.Base(opath)
 		dir := filepath.Dir(opath)
 		var pkg *Package           // if opath belongs to both a package and its test variant, this will be the test variant
@@ -86,7 +85,10 @@ func processGolistOverlay(cfg *Config, response *responseDeduper, rootDirs func(
 		if pkg == nil {
 			// Try to find the module or gopath dir the file is contained in.
 			// Then for modules, add the module opath to the beginning.
-			pkgPath, ok := getPkgPath(cfg, dir, rootDirs)
+			pkgPath, ok, err := state.getPkgPath(dir)
+			if err != nil {
+				return nil, nil, err
+			}
 			if !ok {
 				break
 			}
@@ -130,23 +132,23 @@ func processGolistOverlay(cfg *Config, response *responseDeduper, rootDirs func(
 			continue
 		}
 		for _, imp := range imports {
-			_, found := pkg.Imports[imp]
-			if !found {
-				overlayAddsImports = true
-				// TODO(matloob): Handle cases when the following block isn't correct.
-				// These include imports of vendored packages, etc.
-				id, ok := havePkgs[imp]
-				if !ok {
-					id = imp
-				}
-				pkg.Imports[imp] = &Package{ID: id}
-				// Add dependencies to the non-test variant version of this package as wel.
-				if testVariantOf != nil {
-					testVariantOf.Imports[imp] = &Package{ID: id}
-				}
+			if _, found := pkg.Imports[imp]; found {
+				continue
+			}
+
+			// TODO(matloob): Handle cases when the following block isn't correct.
+			// These include imports of vendored packages, etc.
+			overlayAddsImports = true
+			id, ok := havePkgs[imp]
+			if !ok {
+				id = imp
+			}
+			pkg.Imports[imp] = &Package{ID: id}
+			// Add dependencies to the non-test variant version of this package as well.
+			if testVariantOf != nil {
+				testVariantOf.Imports[imp] = &Package{ID: id}
 			}
 		}
-		continue
 	}
 
 	// toPkgPath tries to guess the package path given the id.
@@ -161,8 +163,8 @@ func processGolistOverlay(cfg *Config, response *responseDeduper, rootDirs func(
 		return id
 	}
 
-	// Do another pass now that new packages have been created to determine the
-	// set of missing packages.
+	// Now that new packages have been created, do another pass to determine
+	// the new set of missing packages.
 	for _, pkg := range response.dr.Packages {
 		for _, imp := range pkg.Imports {
 			pkgPath := toPkgPath(imp.ID)
@@ -194,44 +196,59 @@ func hasTestFiles(p *Package) bool {
 	return false
 }
 
-// determineRootDirs returns a mapping from directories code can be contained in to the
-// corresponding import path prefixes of those directories.
-// Its result is used to try to determine the import path for a package containing
-// an overlay file.
-func determineRootDirs(cfg *Config) map[string]string {
-	// Assume modules first:
-	out, err := invokeGo(cfg, "list", "-m", "-json", "all")
+// determineRootDirs returns a mapping from absolute directories that could
+// contain code to their corresponding import path prefixes.
+func (state *golistState) determineRootDirs() (map[string]string, error) {
+	env, err := state.getEnv()
+	if err != nil {
+		return nil, err
+	}
+	if env["GOMOD"] != "" {
+		state.rootsOnce.Do(func() {
+			state.rootDirs, state.rootDirsError = state.determineRootDirsModules()
+		})
+	} else {
+		state.rootsOnce.Do(func() {
+			state.rootDirs, state.rootDirsError = state.determineRootDirsGOPATH()
+		})
+	}
+	return state.rootDirs, state.rootDirsError
+}
+
+func (state *golistState) determineRootDirsModules() (map[string]string, error) {
+	out, err := state.invokeGo("list", "-m", "-json", "all")
 	if err != nil {
-		return determineRootDirsGOPATH(cfg)
+		return nil, err
 	}
 	m := map[string]string{}
 	type jsonMod struct{ Path, Dir string }
 	for dec := json.NewDecoder(out); dec.More(); {
 		mod := new(jsonMod)
 		if err := dec.Decode(mod); err != nil {
-			return m // Give up and return an empty map. Package won't be found for overlay.
+			return nil, err
 		}
 		if mod.Dir != "" && mod.Path != "" {
 			// This is a valid module; add it to the map.
-			m[mod.Dir] = mod.Path
+			absDir, err := filepath.Abs(mod.Dir)
+			if err != nil {
+				return nil, err
+			}
+			m[absDir] = mod.Path
 		}
 	}
-	return m
+	return m, nil
 }
 
-func determineRootDirsGOPATH(cfg *Config) map[string]string {
+func (state *golistState) determineRootDirsGOPATH() (map[string]string, error) {
 	m := map[string]string{}
-	out, err := invokeGo(cfg, "env", "GOPATH")
-	if err != nil {
-		// Could not determine root dir mapping. Everything is best-effort, so just return an empty map.
-		// When we try to find the import path for a directory, there will be no root-dir match and
-		// we'll give up.
-		return m
-	}
-	for _, p := range filepath.SplitList(string(bytes.TrimSpace(out.Bytes()))) {
-		m[filepath.Join(p, "src")] = ""
+	for _, dir := range filepath.SplitList(state.mustGetEnv()["GOPATH"]) {
+		absDir, err := filepath.Abs(dir)
+		if err != nil {
+			return nil, err
+		}
+		m[filepath.Join(absDir, "src")] = ""
 	}
-	return m
+	return m, nil
 }
 
 func extractImports(filename string, contents []byte) ([]string, error) {
diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go
index 9e12c20a6..4e1efb69b 100644
--- a/go/packages/packages_test.go
+++ b/go/packages/packages_test.go
@@ -1025,78 +1025,82 @@ func testNewPackagesInOverlay(t *testing.T, exporter packagestest.Exporter) {
 
 	dir := filepath.Dir(filepath.Dir(exported.File("golang.org/fake", "a/a.go")))
 
-	for i, test := range []struct {
+	for _, test := range []struct {
+		name    string
 		overlay map[string][]byte
 		want    string // expected value of e.E
 	}{
-		// Overlay with one file.
-		{map[string][]byte{
-			filepath.Join(dir, "e", "e.go"): []byte(`package e; import "golang.org/fake/a"; const E = "e" + a.A`)},
+		{"one_file",
+			map[string][]byte{
+				filepath.Join(dir, "e", "e.go"): []byte(`package e; import "golang.org/fake/a"; const E = "e" + a.A`)},
 			`"eabc"`},
-		// Overlay with multiple files in the same package.
-		{map[string][]byte{
-			filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/a"; const E = "e" + a.A + underscore`),
-			filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
-		},
+		{"multiple_files_same_package",
+			map[string][]byte{
+				filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/a"; const E = "e" + a.A + underscore`),
+				filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
+			},
 			`"eabc_"`},
-		// Overlay with multiple files in different packages.
-		{map[string][]byte{
-			filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/f"; const E = "e" + f.F + underscore`),
-			filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
-			filepath.Join(dir, "f", "f.go"):      []byte(`package f; const F = "f"`),
-		},
+		{"multiple_files_two_packages",
+			map[string][]byte{
+				filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/f"; const E = "e" + f.F + underscore`),
+				filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
+				filepath.Join(dir, "f", "f.go"):      []byte(`package f; const F = "f"`),
+			},
 			`"ef_"`},
-		{map[string][]byte{
-			filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/f"; const E = "e" + f.F + underscore`),
-			filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
-			filepath.Join(dir, "f", "f.go"):      []byte(`package f; import "golang.org/fake/g"; const F = "f" + g.G`),
-			filepath.Join(dir, "g", "g.go"):      []byte(`package g; const G = "g"`),
-		},
+		{"multiple_files_three_packages",
+			map[string][]byte{
+				filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/f"; const E = "e" + f.F + underscore`),
+				filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
+				filepath.Join(dir, "f", "f.go"):      []byte(`package f; import "golang.org/fake/g"; const F = "f" + g.G`),
+				filepath.Join(dir, "g", "g.go"):      []byte(`package g; const G = "g"`),
+			},
 			`"efg_"`},
-		{map[string][]byte{
-			filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/f"; import "golang.org/fake/h"; const E = "e" + f.F + h.H + underscore`),
-			filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
-			filepath.Join(dir, "f", "f.go"):      []byte(`package f; import "golang.org/fake/g"; const F = "f" + g.G`),
-			filepath.Join(dir, "g", "g.go"):      []byte(`package g; const G = "g"`),
-			filepath.Join(dir, "h", "h.go"):      []byte(`package h; const H = "h"`),
-		},
+		{"multiple_files_four_packages",
+			map[string][]byte{
+				filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/f"; import "golang.org/fake/h"; const E = "e" + f.F + h.H + underscore`),
+				filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
+				filepath.Join(dir, "f", "f.go"):      []byte(`package f; import "golang.org/fake/g"; const F = "f" + g.G`),
+				filepath.Join(dir, "g", "g.go"):      []byte(`package g; const G = "g"`),
+				filepath.Join(dir, "h", "h.go"):      []byte(`package h; const H = "h"`),
+			},
 			`"efgh_"`},
-		{map[string][]byte{
-			filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/f"; const E = "e" + f.F + underscore`),
-			filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
-			filepath.Join(dir, "f", "f.go"):      []byte(`package f; import "golang.org/fake/g"; const F = "f" + g.G`),
-			filepath.Join(dir, "g", "g.go"):      []byte(`package g; import "golang.org/fake/h"; const G = "g" + h.H`),
-			filepath.Join(dir, "h", "h.go"):      []byte(`package h; const H = "h"`),
-		},
+		{"multiple_files_four_packages_again",
+			map[string][]byte{
+				filepath.Join(dir, "e", "e.go"):      []byte(`package e; import "golang.org/fake/f"; const E = "e" + f.F + underscore`),
+				filepath.Join(dir, "e", "e_util.go"): []byte(`package e; const underscore = "_"`),
+				filepath.Join(dir, "f", "f.go"):      []byte(`package f; import "golang.org/fake/g"; const F = "f" + g.G`),
+				filepath.Join(dir, "g", "g.go"):      []byte(`package g; import "golang.org/fake/h"; const G = "g" + h.H`),
+				filepath.Join(dir, "h", "h.go"):      []byte(`package h; const H = "h"`),
+			},
 			`"efgh_"`},
-		// Overlay with package main.
-		{map[string][]byte{
-			filepath.Join(dir, "e", "main.go"): []byte(`package main; import "golang.org/fake/a"; const E = "e" + a.A; func main(){}`)},
+		{"main_overlay",
+			map[string][]byte{
+				filepath.Join(dir, "e", "main.go"): []byte(`package main; import "golang.org/fake/a"; const E = "e" + a.A; func main(){}`)},
 			`"eabc"`},
 	} {
-		exported.Config.Overlay = test.overlay
-		exported.Config.Mode = packages.LoadAllSyntax
-		exported.Config.Logf = t.Logf
-
-		// With an overlay, we don't know the expected import path,
-		// so load with the absolute path of the directory.
-		initial, err := packages.Load(exported.Config, filepath.Join(dir, "e"))
-		if err != nil {
-			t.Error(err)
-			continue
-		}
+		t.Run(test.name, func(t *testing.T) {
+			exported.Config.Overlay = test.overlay
+			exported.Config.Mode = packages.LoadAllSyntax
+			exported.Config.Logf = t.Logf
+
+			// With an overlay, we don't know the expected import path,
+			// so load with the absolute path of the directory.
+			initial, err := packages.Load(exported.Config, filepath.Join(dir, "e"))
+			if err != nil {
+				t.Fatal(err)
+			}
 
-		// Check value of e.E.
-		e := initial[0]
-		eE := constant(e, "E")
-		if eE == nil {
-			t.Errorf("%d. e.E: got nil", i)
-			continue
-		}
-		got := eE.Val().String()
-		if got != test.want {
-			t.Errorf("%d. e.E: got %s, want %s", i, got, test.want)
-		}
+			// Check value of e.E.
+			e := initial[0]
+			eE := constant(e, "E")
+			if eE == nil {
+				t.Fatalf("e.E: was nil in %#v", e)
+			}
+			got := eE.Val().String()
+			if got != test.want {
+				t.Fatalf("e.E: got %s, want %s", got, test.want)
+			}
+		})
 	}
 }