From 520188d60f5082c80be05bfde8f4681614a6a418 Mon Sep 17 00:00:00 2001
From: Heschi Kreinick <heschi@google.com>
Date: Tue, 28 Jan 2020 15:57:23 -0500
Subject: [PATCH] go/packages: fix flaky failure creating non-test package from
 overlay

The core of processGolistOverlay was a map iteration, which is
nondeterministic. When creating both a non-test and test package, we
would sometimes encounter the test file before the non-test file. In
that case, for reasons I didn't bother tracking down, we would never
create the non-test package.

Rather than doing complicated bookkeeping to fix the problem, simply
process non-test before test, and make the loop deterministic to save
all of our sanity.

Updates golang/go#36661.

Change-Id: I1f76869fa52794ac8ae96e22ad06a2b1e1861995
Reviewed-on: https://go-review.googlesource.com/c/tools/+/216717
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
---
 go/packages/golist_overlay.go | 20 +++++++++++++++++++-
 go/packages/packages_test.go  | 27 +++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/go/packages/golist_overlay.go b/go/packages/golist_overlay.go
index c6925c87bee..3760744e512 100644
--- a/go/packages/golist_overlay.go
+++ b/go/packages/golist_overlay.go
@@ -7,6 +7,7 @@ import (
 	"go/token"
 	"os"
 	"path/filepath"
+	"sort"
 	"strconv"
 	"strings"
 )
@@ -34,7 +35,24 @@ func (state *golistState) processGolistOverlay(response *responseDeduper) (modif
 	// potentially modifying the transitive set of dependencies).
 	var overlayAddsImports bool
 
-	for opath, contents := range state.cfg.Overlay {
+	// If both a package and its test package are created by the overlay, we
+	// need the real package first. Process all non-test files before test
+	// files, and make the whole process deterministic while we're at it.
+	var overlayFiles []string
+	for opath := range state.cfg.Overlay {
+		overlayFiles = append(overlayFiles, opath)
+	}
+	sort.Slice(overlayFiles, func(i, j int) bool {
+		iTest := strings.HasSuffix(overlayFiles[i], "_test.go")
+		jTest := strings.HasSuffix(overlayFiles[j], "_test.go")
+		if iTest != jTest {
+			return !iTest // non-tests are before tests.
+		}
+		return overlayFiles[i] < overlayFiles[j]
+	})
+
+	for _, opath := range overlayFiles {
+		contents := state.cfg.Overlay[opath]
 		base := filepath.Base(opath)
 		dir := filepath.Dir(opath)
 		var pkg *Package           // if opath belongs to both a package and its test variant, this will be the test variant
diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go
index 9edce09c2b8..81e0d43746c 100644
--- a/go/packages/packages_test.go
+++ b/go/packages/packages_test.go
@@ -1104,6 +1104,33 @@ func testNewPackagesInOverlay(t *testing.T, exporter packagestest.Exporter) {
 	}
 }
 
+// Test that we can create a package and its test package in an overlay.
+func TestOverlayNewPackageAndTest(t *testing.T) { packagestest.TestAll(t, testOverlayNewPackageAndTest) }
+func testOverlayNewPackageAndTest(t *testing.T, exporter packagestest.Exporter) {
+	exported := packagestest.Export(t, exporter, []packagestest.Module{
+		{
+			Name: "golang.org/fake",
+			Files: map[string]interface{}{
+				"foo.txt": "placeholder",
+			},
+		},
+	})
+	defer exported.Cleanup()
+
+	dir := filepath.Dir(exported.File("golang.org/fake", "foo.txt"))
+	exported.Config.Overlay = map[string][]byte{
+		filepath.Join(dir, "a.go"):      []byte(`package a;`),
+		filepath.Join(dir, "a_test.go"): []byte(`package a; import "testing";`),
+	}
+	initial, err := packages.Load(exported.Config, "file="+filepath.Join(dir, "a.go"), "file="+filepath.Join(dir, "a_test.go"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(initial) != 2 {
+		t.Errorf("got %v packages, wanted %v", len(initial), 2)
+	}
+}
+
 func TestAdHocPackagesBadImport(t *testing.T) {
 	// This test doesn't use packagestest because we are testing ad-hoc packages,
 	// which are outside of $GOPATH and outside of a module.