go/packages: handle x test overlays with no content
This change handles the case when an empty x test is provided in an
overlay. Previously, the package name was being changed back to the name
of the rest of the files in the package, but in reality, we don't want
that behavior for x tests.
Change-Id: I4c7d938e9c26b92d4a688d3cfd42755b35e2c1c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/233657
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
diff --git a/go/packages/golist_overlay.go b/go/packages/golist_overlay.go
index 3c99b6e..b82c90d 100644
--- a/go/packages/golist_overlay.go
+++ b/go/packages/golist_overlay.go
@@ -70,9 +70,9 @@
// to the overlay.
continue
}
- // if all the overlay files belong to a different package, change the package
- // name to that package. Otherwise leave it alone; there will be an error message.
- maybeFixPackageName(pkgName, pkgOfDir, dir)
+ // If all the overlay files belong to a different package, change the
+ // package name to that package.
+ maybeFixPackageName(pkgName, isTestFile, pkgOfDir[dir])
nextPackage:
for _, p := range response.dr.Packages {
if pkgName != p.Name && p.ID != "command-line-arguments" {
@@ -121,16 +121,21 @@
if isTestFile && !isXTest {
id = fmt.Sprintf("%s [%s.test]", pkgPath, pkgPath)
}
- // Try to reclaim a package with the same id if it exists in the response.
+ // Try to reclaim a package with the same ID, if it exists in the response.
for _, p := range response.dr.Packages {
if reclaimPackage(p, id, opath, contents) {
pkg = p
break
}
}
- // Otherwise, create a new package
+ // Otherwise, create a new package.
if pkg == nil {
- pkg = &Package{PkgPath: pkgPath, ID: id, Name: pkgName, Imports: make(map[string]*Package)}
+ pkg = &Package{
+ PkgPath: pkgPath,
+ ID: id,
+ Name: pkgName,
+ Imports: make(map[string]*Package),
+ }
response.addPackage(pkg)
havePkgs[pkg.PkgPath] = id
// Add the production package's sources for a test variant.
@@ -143,6 +148,7 @@
pkg.Imports[k] = &Package{ID: v.ID}
}
}
+ // TODO(rstambler): Handle forTest for x_tests.
}
}
if !fileExists {
@@ -158,6 +164,8 @@
continue
}
for _, imp := range imports {
+ // TODO(rstambler): If the package is an x test and the import has
+ // a test variant, make sure to replace it.
if _, found := pkg.Imports[imp]; found {
continue
}
@@ -415,24 +423,35 @@
// package name, and they all have the same package name, then that name becomes
// the package name.
// It returns true if it changes the package name, false otherwise.
-func maybeFixPackageName(newName string, pkgOfDir map[string][]*Package, dir string) bool {
+func maybeFixPackageName(newName string, isTestFile bool, pkgsOfDir []*Package) {
names := make(map[string]int)
- for _, p := range pkgOfDir[dir] {
+ for _, p := range pkgsOfDir {
names[p.Name]++
}
if len(names) != 1 {
// some files are in different packages
- return false
+ return
}
- oldName := ""
+ var oldName string
for k := range names {
oldName = k
}
if newName == oldName {
- return false
+ return
}
- for _, p := range pkgOfDir[dir] {
+ // We might have a case where all of the package names in the directory are
+ // the same, but the overlay file is for an x test, which belongs to its
+ // own package. If the x test does not yet exist on disk, we may not yet
+ // have its package name on disk, but we should not rename the packages.
+ //
+ // We use a heuristic to determine if this file belongs to an x test:
+ // The test file should have a package name whose package name has a _test
+ // suffix or looks like "newName_test".
+ maybeXTest := strings.HasPrefix(oldName+"_test", newName) || strings.HasSuffix(newName, "_test")
+ if isTestFile && maybeXTest {
+ return
+ }
+ for _, p := range pkgsOfDir {
p.Name = newName
}
- return true
}
diff --git a/go/packages/overlay_test.go b/go/packages/overlay_test.go
index 7459f2b..3eb5b97 100644
--- a/go/packages/overlay_test.go
+++ b/go/packages/overlay_test.go
@@ -1,6 +1,7 @@
package packages_test
import (
+ "fmt"
"log"
"path/filepath"
"testing"
@@ -125,6 +126,68 @@
log.SetFlags(0)
}
+func TestOverlayXTests(t *testing.T) {
+ packagestest.TestAll(t, testOverlayXTests)
+}
+func testOverlayXTests(t *testing.T, exporter packagestest.Exporter) {
+ exported := packagestest.Export(t, exporter, []packagestest.Module{{
+ Name: "golang.org/fake",
+ Files: map[string]interface{}{
+ "a/a.go": `package a; const C = "C"; func Hello() {}`,
+ "a/a_test.go": `package a
+
+import "testing"
+
+const TestC = "test" + C
+
+func TestHello(){
+ Hello()
+}`,
+ "a/a_x_test.go": "",
+ },
+ Overlay: map[string][]byte{
+ "a/a_x_test.go": []byte(`package a_test
+
+import (
+ "testing"
+
+ "golang.org/fake/a"
+)
+
+const xTestC = "x" + a.C
+
+func TestHello(t *testing.T) {
+ a.Hello()
+}
+`),
+ },
+ }})
+ defer exported.Cleanup()
+
+ exported.Config.Mode = commonMode
+ exported.Config.Tests = true
+ exported.Config.Mode = packages.LoadTypes
+
+ initial, err := packages.Load(exported.Config, fmt.Sprintf("file=%s", exported.File("golang.org/fake", "a/a_x_test.go")))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(initial) != 1 {
+ t.Fatalf("expected 1 package, got %d", len(initial))
+ }
+ xTestC := constant(initial[0], "xTestC")
+ if xTestC == nil {
+ t.Fatalf("no value for xTestC")
+ }
+ got := xTestC.Val().String()
+ // TODO(rstambler): Ideally, this test would check that the test variant
+ // was imported, but that's pretty complicated.
+ if want := `"xC"`; got != want {
+ t.Errorf("got: %q, want %q", got, want)
+ }
+
+}
+
func checkPkg(t *testing.T, p *packages.Package, id, name string, syntax int) bool {
t.Helper()
if p.ID == id && p.Name == name && len(p.Syntax) == syntax {
