From 0b24b358f4c7eaa92895f67a3f6cea2a0cf525d5 Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Mon, 17 Sep 2018 15:17:20 -0700 Subject: [PATCH] go/packages: add missing test variants to fallback loader For all packages p, q where there's an dependency path of p.test -> ... -> q -> ... -> p, Go creates test variants q [p.test] of each q and replaces the dependency on q with a dependency on q [p.test], and replaces p with the expanded test variant p[p.test]. Fix the fallback logic to add these missing test variants. (Before this change, it was only producing the variant of p: p [p.test].) Fixes golang/go#27670 Change-Id: Ic56ba35fadcdf8c5928ec76f5a7b0ebe650c9f02 Reviewed-on: https://go-review.googlesource.com/136176 Reviewed-by: Ian Cottrell --- go/packages/golist_fallback.go | 103 ++++++++++++++++++------ go/packages/golist_fallback_testmain.go | 60 ++++++++++++-- go/packages/packages_test.go | 4 - 3 files changed, 133 insertions(+), 34 deletions(-) diff --git a/go/packages/golist_fallback.go b/go/packages/golist_fallback.go index ea484706..ad5e003c 100644 --- a/go/packages/golist_fallback.go +++ b/go/packages/golist_fallback.go @@ -33,14 +33,20 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error) } var tmpdir string // used for generated cgo files + var needsTestVariant []struct { + pkg, xtestPkg *Package + } var response driverResponse + allPkgs := make(map[string]bool) addPackage := func(p *jsonPackage) { - if p.Name == "" { + id := p.ImportPath + + if p.Name == "" || allPkgs[id] { return } + allPkgs[id] = true - id := p.ImportPath isRoot := original[id] != nil pkgpath := id @@ -110,7 +116,7 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error) if isRoot { response.Roots = append(response.Roots, id) } - response.Packages = append(response.Packages, &Package{ + pkg := &Package{ ID: id, Name: p.Name, GoFiles: absJoin(p.Dir, p.GoFiles, p.CgoFiles), @@ -119,13 +125,12 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error) PkgPath: pkgpath, Imports: importMap(p.Imports), // TODO(matloob): set errors on the Package to cgoErrors - }) - if cfg.Tests { + } + response.Packages = append(response.Packages, pkg) + if cfg.Tests && isRoot { testID := fmt.Sprintf("%s [%s.test]", id, id) if len(p.TestGoFiles) > 0 || len(p.XTestGoFiles) > 0 { - if isRoot { - response.Roots = append(response.Roots, testID) - } + response.Roots = append(response.Roots, testID) testPkg := &Package{ ID: testID, Name: p.Name, @@ -140,32 +145,28 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error) var xtestPkg *Package if len(p.XTestGoFiles) > 0 { xtestID := fmt.Sprintf("%s_test [%s.test]", id, id) - if isRoot { - response.Roots = append(response.Roots, xtestID) - } - // Rewrite import to package under test to refer to test variant. - imports := importMap(p.XTestImports) - for imp := range imports { - if imp == p.ImportPath { - imports[imp] = &Package{ID: testID} - break - } - } + response.Roots = append(response.Roots, xtestID) + // Generate test variants for all packages q where a path exists + // such that xtestPkg -> ... -> q -> ... -> p (where p is the package under test) + // and rewrite all import map entries of p to point to testPkg (the test variant of + // p), and of each q to point to the test variant of that q. xtestPkg = &Package{ ID: xtestID, Name: p.Name + "_test", GoFiles: absJoin(p.Dir, p.XTestGoFiles), CompiledGoFiles: absJoin(p.Dir, p.XTestGoFiles), PkgPath: pkgpath + "_test", - Imports: imports, + Imports: importMap(p.XTestImports), } + // Add to list of packages we need to rewrite imports for to refer to test variants. + // We may need to create a test variant of a package that hasn't been loaded yet, so + // the test variants need to be created later. + needsTestVariant = append(needsTestVariant, struct{ pkg, xtestPkg *Package }{pkg, xtestPkg}) response.Packages = append(response.Packages, xtestPkg) } // testmain package testmainID := id + ".test" - if isRoot { - response.Roots = append(response.Roots, testmainID) - } + response.Roots = append(response.Roots, testmainID) imports := map[string]*Package{} imports[testPkg.PkgPath] = &Package{ID: testPkg.ID} if xtestPkg != nil { @@ -185,13 +186,13 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error) return } testmain := filepath.Join(outdir, "testmain.go") - extradeps, err := generateTestmain(testmain, testPkg, xtestPkg) + extraimports, extradeps, err := generateTestmain(testmain, testPkg, xtestPkg) if err != nil { testmainPkg.Errors = append(testmainPkg.Errors, Error{"-", fmt.Sprintf("failed to generate testmain: %v", err)}) } deps = append(deps, extradeps...) - for _, imp := range extradeps { // testing, testing/internal/testdeps, and maybe os + for _, imp := range extraimports { // testing, testing/internal/testdeps, and maybe os imports[imp] = &Package{ID: imp} } testmainPkg.GoFiles = []string{testmain} @@ -222,6 +223,10 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error) addPackage(p) } + for _, v := range needsTestVariant { + createTestVariants(&response, v.pkg, v.xtestPkg) + } + // TODO(matloob): Is this the right ordering? sort.SliceStable(response.Packages, func(i, j int) bool { return response.Packages[i].PkgPath < response.Packages[j].PkgPath @@ -230,6 +235,54 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error) return &response, nil } +func createTestVariants(response *driverResponse, pkgUnderTest, xtestPkg *Package) { + allPkgs := make(map[string]*Package) + for _, pkg := range response.Packages { + allPkgs[pkg.ID] = pkg + } + needsTestVariant := make(map[string]bool) + needsTestVariant[pkgUnderTest.ID] = true + var needsVariantRec func(p *Package) bool + needsVariantRec = func(p *Package) bool { + if needsTestVariant[p.ID] { + return true + } + for _, imp := range p.Imports { + if needsVariantRec(allPkgs[imp.ID]) { + // Don't break because we want to make sure all dependencies + // have been processed, and all required test variants of our dependencies + // exist. + needsTestVariant[p.ID] = true + } + } + if !needsTestVariant[p.ID] { + return false + } + // Create a clone of the package. It will share the same strings and lists of source files, + // but that's okay. It's only necessary for the Imports map to have a separate identity. + testVariant := *p + testVariant.ID = fmt.Sprintf("%s [%s.test]", p.ID, pkgUnderTest.ID) + testVariant.Imports = make(map[string]*Package) + for imp, pkg := range p.Imports { + testVariant.Imports[imp] = pkg + if needsTestVariant[pkg.ID] { + testVariant.Imports[imp] = &Package{ID: fmt.Sprintf("%s [%s.test]", pkg.ID, pkgUnderTest.ID)} + } + } + response.Packages = append(response.Packages, &testVariant) + return needsTestVariant[p.ID] + } + // finally, update the xtest package's imports + for imp, pkg := range xtestPkg.Imports { + if allPkgs[pkg.ID] == nil { + fmt.Printf("for %s: package %s doesn't exist\n", xtestPkg.ID, pkg.ID) + } + if needsVariantRec(allPkgs[pkg.ID]) { + xtestPkg.Imports[imp] = &Package{ID: fmt.Sprintf("%s [%s.test]", pkg.ID, pkgUnderTest.ID)} + } + } +} + // vendorlessPath returns the devendorized version of the import path ipath. // For example, VendorlessPath("foo/bar/vendor/a/b") returns "a/b". // Copied from golang.org/x/tools/imports/fix.go. diff --git a/go/packages/golist_fallback_testmain.go b/go/packages/golist_fallback_testmain.go index 37541707..128e00e2 100644 --- a/go/packages/golist_fallback_testmain.go +++ b/go/packages/golist_fallback_testmain.go @@ -27,16 +27,66 @@ import ( // This file complements golist_fallback.go by providing // support for generating testmains. -func generateTestmain(out string, testPkg, xtestPkg *Package) (extradeps []string, err error) { +func generateTestmain(out string, testPkg, xtestPkg *Package) (extraimports, extradeps []string, err error) { testFuncs, err := loadTestFuncs(testPkg, xtestPkg) if err != nil { - return nil, err + return nil, nil, err } - extradeps = []string{"testing/internal/testdeps", "testing"} + extraimports = []string{"testing", "testing/internal/testdeps"} if testFuncs.TestMain == nil { - extradeps = append(extradeps, "os") + extraimports = append(extraimports, "os") } - return extradeps, writeTestmain(out, testFuncs) + // Transitive dependencies of ("testing", "testing/internal/testdeps"). + // os is part of the transitive closure so it and its transitive dependencies are + // included regardless of whether it's imported in the template below. + extradeps = []string{ + "errors", + "internal/cpu", + "unsafe", + "internal/bytealg", + "internal/race", + "runtime/internal/atomic", + "runtime/internal/sys", + "runtime", + "sync/atomic", + "sync", + "io", + "unicode", + "unicode/utf8", + "bytes", + "math", + "syscall", + "time", + "internal/poll", + "internal/syscall/unix", + "internal/testlog", + "os", + "math/bits", + "strconv", + "reflect", + "fmt", + "sort", + "strings", + "flag", + "runtime/debug", + "context", + "runtime/trace", + "testing", + "bufio", + "regexp/syntax", + "regexp", + "compress/flate", + "encoding/binary", + "hash", + "hash/crc32", + "compress/gzip", + "path/filepath", + "io/ioutil", + "text/tabwriter", + "runtime/pprof", + "testing/internal/testdeps", + } + return extraimports, extradeps, writeTestmain(out, testFuncs) } // The following is adapted from the cmd/go testmain generation code. diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index b39e77a5..4babf2c1 100644 --- a/go/packages/packages_test.go +++ b/go/packages/packages_test.go @@ -240,10 +240,6 @@ func TestLoadImportsGraph(t *testing.T) { } func TestLoadImportsTestVariants(t *testing.T) { - if usesOldGolist { - t.Skip("not yet supported in pre-Go 1.10.4 golist fallback implementation") - } - tmp, cleanup := makeTree(t, map[string]string{ "src/a/a.go": `package a; import _ "b"`, "src/b/b.go": `package b`,