From 2723c5de0d66343f56884cf109e6d6fb4d02c5f0 Mon Sep 17 00:00:00 2001
From: Rebecca Stambler <rstambler@golang.org>
Date: Wed, 22 Apr 2020 23:44:20 -0400
Subject: [PATCH] go/packages/packagestest: dedupe based on note position

The "forTest" field is used for x_tests and for test variants, making
the deduplication too agressive. Instead, we deduplicate by the
position of the note.

Change-Id: I809364d6bba95be5ad95eced67fae645435f5592
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229597
Reviewed-by: Paul Jolly <paul@myitcv.org.uk>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
---
 go/packages/packagestest/expect.go            | 22 ++++++++-----------
 go/packages/packagestest/expect_test.go       |  2 +-
 go/packages/packagestest/export_test.go       |  5 +++--
 .../groups/two/primarymod/expect/yo.go        |  4 +++-
 .../groups/two/primarymod/expect/yo_test.go   | 10 +++++++++
 5 files changed, 26 insertions(+), 17 deletions(-)
 create mode 100644 go/packages/packagestest/testdata/groups/two/primarymod/expect/yo_test.go

diff --git a/go/packages/packagestest/expect.go b/go/packages/packagestest/expect.go
index e8c684738a..c1781e7b9a 100644
--- a/go/packages/packagestest/expect.go
+++ b/go/packages/packagestest/expect.go
@@ -16,7 +16,6 @@ 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"
 )
 
@@ -152,18 +151,8 @@ 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)
+	seen := make(map[token.Position]struct{})
 	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 {
@@ -173,7 +162,14 @@ func (e *Exported) getNotes() error {
 			if err != nil {
 				return fmt.Errorf("failed to extract expectations: %v", err)
 			}
-			notes = append(notes, l...)
+			for _, note := range l {
+				pos := e.ExpectFileSet.Position(note.Pos)
+				if _, ok := seen[pos]; ok {
+					continue
+				}
+				notes = append(notes, note)
+				seen[pos] = struct{}{}
+			}
 		}
 	}
 	if _, ok := e.written[e.primary]; !ok {
diff --git a/go/packages/packagestest/expect_test.go b/go/packages/packagestest/expect_test.go
index 098bdbe418..2587f580b0 100644
--- a/go/packages/packagestest/expect_test.go
+++ b/go/packages/packagestest/expect_test.go
@@ -65,7 +65,7 @@ func TestExpect(t *testing.T) {
 	// 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
+	wantCheck := 7
 	if wantCheck != checkCount {
 		t.Fatalf("Expected @check count of %v; got %v", wantCheck, checkCount)
 	}
diff --git a/go/packages/packagestest/export_test.go b/go/packages/packagestest/export_test.go
index e7bf124c4c..36f6397505 100644
--- a/go/packages/packagestest/export_test.go
+++ b/go/packages/packagestest/export_test.go
@@ -113,8 +113,9 @@ func TestGroupFilesByModules(t *testing.T) {
 				{
 					Name: "testdata/groups/two",
 					Files: map[string]interface{}{
-						"main.go":      true,
-						"expect/yo.go": true,
+						"main.go":           true,
+						"expect/yo.go":      true,
+						"expect/yo_test.go": true,
 					},
 				},
 				{
diff --git a/go/packages/packagestest/testdata/groups/two/primarymod/expect/yo.go b/go/packages/packagestest/testdata/groups/two/primarymod/expect/yo.go
index a7067786ed..bce2d30e09 100644
--- a/go/packages/packagestest/testdata/groups/two/primarymod/expect/yo.go
+++ b/go/packages/packagestest/testdata/groups/two/primarymod/expect/yo.go
@@ -1 +1,3 @@
-package expect
\ No newline at end of file
+package expect
+
+var X int //@check("X", "X")
diff --git a/go/packages/packagestest/testdata/groups/two/primarymod/expect/yo_test.go b/go/packages/packagestest/testdata/groups/two/primarymod/expect/yo_test.go
new file mode 100644
index 0000000000..a8b0612658
--- /dev/null
+++ b/go/packages/packagestest/testdata/groups/two/primarymod/expect/yo_test.go
@@ -0,0 +1,10 @@
+package expect_test
+
+import (
+	"testdata/groups/two/expect"
+	"testing"
+)
+
+func TestX(t *testing.T) {
+	_ = expect.X //@check("X", "X")
+}