go/packages: correct package IDs for overlaid x tests

Bad go/packages results were being cached, causing unexpected behavior
when new x tests are created. This fix also allows us to enable a
regression test that had been previously disabled.

Change-Id: I93c05df2f4d32c6c8a43e9f5aaeeb30bc4a32f3a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/238058
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

go/packages/golist_overlay.go

@@ -113,13 +113,19 @@ func (state *golistState) processGolistOverlay(response *responseDeduper) (modif
 			if !ok {
 				break
 			}
+			var forTest string // only set for x tests
 			isXTest := strings.HasSuffix(pkgName, "_test")
 			if isXTest {
+				forTest = pkgPath
 				pkgPath += "_test"
 			}
 			id := pkgPath
-			if isTestFile && !isXTest {
-				id = fmt.Sprintf("%s [%s.test]", pkgPath, pkgPath)
+			if isTestFile {
+				if isXTest {
+					id = fmt.Sprintf("%s [%s.test]", pkgPath, forTest)
+				} else {
+					id = fmt.Sprintf("%s [%s.test]", pkgPath, pkgPath)
+				}
 			}
 			// Try to reclaim a package with the same ID, if it exists in the response.
 			for _, p := range response.dr.Packages {
@@ -148,7 +154,9 @@ func (state *golistState) processGolistOverlay(response *responseDeduper) (modif
 						pkg.Imports[k] = &Package{ID: v.ID}
 					}
 				}
-				// TODO(rstambler): Handle forTest for x_tests.
+				if isXTest {
+					pkg.forTest = forTest
+				}
 			}
 		}
 		if !fileExists {

go/packages/overlay_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"log"
 	"path/filepath"
+	"reflect"
 	"testing"
 
 	"golang.org/x/tools/go/packages"
@@ -129,12 +130,13 @@ func TestOverlayChangesTestPackage(t *testing.T) {
 func TestOverlayXTests(t *testing.T) {
 	packagestest.TestAll(t, testOverlayXTests)
 }
+
+// This test checks the behavior of go/packages.Load with an overlaid
+// x test. The source of truth is the go/packages.Load results for the
+// exact same package, just on-disk.
 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
+	const aFile = `package a; const C = "C"; func Hello() {}`
+	const aTestVariant = `package a
 
 import "testing"
 
@@ -142,11 +144,8 @@ 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
+}`
+	const aXTest = `package a_test
 
 import (
 	"testing"
@@ -158,24 +157,50 @@ const xTestC = "x" + a.C
 
 func TestHello(t *testing.T) {
 	a.Hello()
-}
-`),
+}`
+
+	// First, get the source of truth by loading the package, all on disk.
+	onDisk := packagestest.Export(t, exporter, []packagestest.Module{{
+		Name: "golang.org/fake",
+		Files: map[string]interface{}{
+			"a/a.go":        aFile,
+			"a/a_test.go":   aTestVariant,
+			"a/a_x_test.go": aXTest,
+		},
+	}})
+	defer onDisk.Cleanup()
+
+	onDisk.Config.Mode = commonMode
+	onDisk.Config.Tests = true
+	onDisk.Config.Mode = packages.LoadTypes
+	initial, err := packages.Load(onDisk.Config, fmt.Sprintf("file=%s", onDisk.File("golang.org/fake", "a/a_x_test.go")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	wantPkg := initial[0]
+
+	exported := packagestest.Export(t, exporter, []packagestest.Module{{
+		Name: "golang.org/fake",
+		Files: map[string]interface{}{
+			"a/a.go":        aFile,
+			"a/a_test.go":   aTestVariant,
+			"a/a_x_test.go": ``, // empty x test on disk
+		},
+		Overlay: map[string][]byte{
+			"a/a_x_test.go": []byte(aXTest),
 		},
 	}})
 	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")
+	// Confirm that the overlaid package is identical to the on-disk version.
+	pkg := initial[0]
+	if !reflect.DeepEqual(wantPkg, pkg) {
+		t.Fatalf("mismatched packages: want %#v, got %#v", wantPkg, pkg)
+	}
+	xTestC := constant(pkg, "xTestC")
 	if xTestC == nil {
 		t.Fatalf("no value for xTestC")
 	}
@@ -185,7 +210,6 @@ func TestHello(t *testing.T) {
 	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 {

go/packages/packages_test.go

@@ -1560,6 +1560,7 @@ func testContainsOverlayXTest(t *testing.T, exporter packagestest.Exporter) {
 			"c/c.go": `package c`,
 		}}})
 	defer exported.Cleanup()
+
 	bOverlayXTestFile := filepath.Join(filepath.Dir(exported.File("golang.org/fake", "b/b.go")), "b_overlay_x_test.go")
 	exported.Config.Mode = packages.NeedName | packages.NeedFiles | packages.NeedImports
 	exported.Config.Overlay = map[string][]byte{bOverlayXTestFile: []byte(`package b_test; import "golang.org/fake/b"`)}
@@ -1571,15 +1572,14 @@ func testContainsOverlayXTest(t *testing.T, exporter packagestest.Exporter) {
 	graph, _ := importGraph(initial)
 	wantGraph := `
   golang.org/fake/b
-* golang.org/fake/b_test
+* golang.org/fake/b_test [golang.org/fake/b.test]
   golang.org/fake/c
   golang.org/fake/b -> golang.org/fake/c
-  golang.org/fake/b_test -> golang.org/fake/b
+  golang.org/fake/b_test [golang.org/fake/b.test] -> golang.org/fake/b
 `[1:]
 	if graph != wantGraph {
 		t.Errorf("wrong import graph: got <<%s>>, want <<%s>>", graph, wantGraph)
 	}
-
 }
 
 // This test ensures that the effective GOARCH variable in the

internal/lsp/regtest/diagnostics_test.go

@@ -734,8 +734,6 @@ func _() {
 // This test tries to replicate the workflow of a user creating a new x test.
 // It also tests golang/go#39315.
 func TestManuallyCreatingXTest(t *testing.T) {
-	t.Skipf("See golang/go#39315")
-
 	// Only for 1.15 because of golang/go#37971.
 	testenv.NeedsGo1Point(t, 15)