From cfa8b22178d2faeacea202c63787cc6193a51a8c Mon Sep 17 00:00:00 2001
From: Paul Jolly <paul@myitcv.io>
Date: Fri, 17 Apr 2020 22:07:45 +0100
Subject: [PATCH] go/packages/packagestest: do not walk packages and their test
 variants

Currently, where a package has a test variant, we end up walking the
non-_test.go files twice: once when walking the package itself, the
second time when walking the test variant. In the gopls tests, this
means we end up double-counting the number of symbols (via the @symbol
annotation) in a package.

Extend the existing expect_test.go to demonstrate the problem is fixed
(a test which fails when the fix is not in place)

Change-Id: Ifd68b3d86e24f19d3f8efc3af71494b7d28b0acb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228761
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
---
 go/packages/packagestest/expect.go             | 12 ++++++++++++
 go/packages/packagestest/expect_test.go        | 13 +++++++++----
 go/packages/packagestest/testdata/test_test.go |  3 +++
 go/packages/packagestest/testdata/x_test.go    |  3 +++
 internal/lsp/testdata/lsp/summary.txt.golden   |  6 +++---
 5 files changed, 30 insertions(+), 7 deletions(-)
 create mode 100644 go/packages/packagestest/testdata/test_test.go
 create mode 100644 go/packages/packagestest/testdata/x_test.go

diff --git a/go/packages/packagestest/expect.go b/go/packages/packagestest/expect.go
index ddbec4448f..e8c684738a 100644
--- a/go/packages/packagestest/expect.go
+++ b/go/packages/packagestest/expect.go
@@ -16,6 +16,7 @@ import (
 
 	"golang.org/x/tools/go/expect"
 	"golang.org/x/tools/go/packages"
+	"golang.org/x/tools/internal/packagesinternal"
 	"golang.org/x/tools/internal/span"
 )
 
@@ -151,7 +152,18 @@ func (e *Exported) getNotes() error {
 	if err != nil {
 		return fmt.Errorf("unable to load packages for directories %s: %v", dirs, err)
 	}
+	// We need to avoid walking packages and their test variants, otherwise we
+	// double-count
+	pkgsWithTestVariants := make(map[string]bool)
 	for _, pkg := range pkgs {
+		if forTest := packagesinternal.GetForTest(pkg); forTest != "" {
+			pkgsWithTestVariants[forTest] = true
+		}
+	}
+	for _, pkg := range pkgs {
+		if pkgsWithTestVariants[pkg.ID] {
+			continue
+		}
 		for _, filename := range pkg.GoFiles {
 			content, err := e.FileContents(filename)
 			if err != nil {
diff --git a/go/packages/packagestest/expect_test.go b/go/packages/packagestest/expect_test.go
index 527c8d732d..098bdbe418 100644
--- a/go/packages/packagestest/expect_test.go
+++ b/go/packages/packagestest/expect_test.go
@@ -19,10 +19,10 @@ func TestExpect(t *testing.T) {
 		Files: packagestest.MustCopyFileTree("testdata"),
 	}})
 	defer exported.Cleanup()
-	count := 0
+	checkCount := 0
 	if err := exported.Expect(map[string]interface{}{
 		"check": func(src, target token.Position) {
-			count++
+			checkCount++
 		},
 		"boolArg": func(n *expect.Note, yes, no bool) {
 			if !yes {
@@ -61,7 +61,12 @@ func TestExpect(t *testing.T) {
 	}); err != nil {
 		t.Fatal(err)
 	}
-	if count == 0 {
-		t.Fatalf("No tests were run")
+	// We expect to have walked the @check annotations in all .go files,
+	// including _test.go files (XTest or otherwise). But to have walked the
+	// non-_test.go files only once. Hence wantCheck = 3 (testdata/test.go) + 1
+	// (testdata/test_test.go) + 1 (testdata/x_test.go)
+	wantCheck := 5
+	if wantCheck != checkCount {
+		t.Fatalf("Expected @check count of %v; got %v", wantCheck, checkCount)
 	}
 }
diff --git a/go/packages/packagestest/testdata/test_test.go b/go/packages/packagestest/testdata/test_test.go
new file mode 100644
index 0000000000..18b20805f9
--- /dev/null
+++ b/go/packages/packagestest/testdata/test_test.go
@@ -0,0 +1,3 @@
+package fake1
+
+type ATestType string //@check("ATestType","ATestType")
diff --git a/go/packages/packagestest/testdata/x_test.go b/go/packages/packagestest/testdata/x_test.go
new file mode 100644
index 0000000000..c8c4fa2534
--- /dev/null
+++ b/go/packages/packagestest/testdata/x_test.go
@@ -0,0 +1,3 @@
+package fake1_test
+
+type AnXTestType string //@check("AnXTestType","AnXTestType")
diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden
index 853af96281..5b902ce285 100644
--- a/internal/lsp/testdata/lsp/summary.txt.golden
+++ b/internal/lsp/testdata/lsp/summary.txt.golden
@@ -1,8 +1,8 @@
 -- summary --
 CodeLensCount = 2
-CompletionsCount = 238
-CompletionSnippetCount = 75
-UnimportedCompletionsCount = 11
+CompletionsCount = 237
+CompletionSnippetCount = 74
+UnimportedCompletionsCount = 6
 DeepCompletionsCount = 5
 FuzzyCompletionsCount = 8
 RankedCompletionsCount = 120