From 090b14e8501f6c8fe1b5c6e8d039defbe8716cc1 Mon Sep 17 00:00:00 2001
From: Robert Findley <rfindley@google.com>
Date: Fri, 13 May 2022 17:39:10 -0400
Subject: [PATCH] internal/lsp/regtest: make TestResolveImportCycle robust to
 error order

In JSON output, the Go command was only setting the Error field for one
package involved in an import cycle. As a result, TestResolveImportCycle
was dependent on the chosen package, causing flakes when the chosen
package is not deterministic.

Arguably this behavior should be fixed, both in the go command and in
gopls, but for now make the test resilient to choice by asserting on any
of the possible errors.

Add a new AnyOf expectation to support this type of assertion and tweak the
test output formatting. Also update the test to eagerly fail once the
didOpen notification has been fully processed, so that we don't have to
wait for the assertion timeout.

Updates golang/go#52904

Change-Id: Ic209d8fdcb7308c041b287a8f122c47e96d29a96
Reviewed-on: https://go-review.googlesource.com/c/tools/+/406274
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
---
 .../regtest/diagnostics/diagnostics_test.go   | 14 +++++++-
 internal/lsp/regtest/env.go                   |  2 +-
 internal/lsp/regtest/expectation.go           | 34 ++++++++++++++++---
 3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index 433b462349..a2707efdb5 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -1650,7 +1650,19 @@ const B = a.B
 	Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("a/a.go")
 		env.OpenFile("b/b.go")
-		env.Await(env.DiagnosticAtRegexp("a/a.go", `"mod.test/b"`))
+		env.Await(
+			OnceMet(
+				env.DoneWithOpen(),
+				// The Go command sometimes tells us about only one of the import cycle
+				// errors below. For robustness of this test, succeed if we get either.
+				//
+				// TODO(golang/go#52904): we should get *both* of these errors.
+				AnyOf(
+					env.DiagnosticAtRegexpWithMessage("a/a.go", `"mod.test/b"`, "import cycle"),
+					env.DiagnosticAtRegexpWithMessage("b/b.go", `"mod.test/a"`, "import cycle"),
+				),
+			),
+		)
 		env.RegexpReplace("b/b.go", `const B = a\.B`, "")
 		env.SaveBuffer("b/b.go")
 		env.Await(
diff --git a/internal/lsp/regtest/env.go b/internal/lsp/regtest/env.go
index b6b163a87b..f095c38f28 100644
--- a/internal/lsp/regtest/env.go
+++ b/internal/lsp/regtest/env.go
@@ -258,7 +258,7 @@ func checkExpectations(s State, expectations []Expectation) (Verdict, string) {
 		if v > finalVerdict {
 			finalVerdict = v
 		}
-		summary.WriteString(fmt.Sprintf("\t%v: %s\n", v, e.Description()))
+		summary.WriteString(fmt.Sprintf("%v: %s\n", v, e.Description()))
 	}
 	return finalVerdict, summary.String()
 }
diff --git a/internal/lsp/regtest/expectation.go b/internal/lsp/regtest/expectation.go
index 15de33f349..ab808f9e8c 100644
--- a/internal/lsp/regtest/expectation.go
+++ b/internal/lsp/regtest/expectation.go
@@ -96,13 +96,37 @@ func OnceMet(precondition Expectation, mustMeets ...Expectation) *SimpleExpectat
 			return Unmet
 		}
 	}
-	var descriptions []string
-	for _, mustMeet := range mustMeets {
-		descriptions = append(descriptions, mustMeet.Description())
-	}
+	description := describeExpectations(mustMeets...)
 	return &SimpleExpectation{
 		check:       check,
-		description: fmt.Sprintf("once %q is met, must have %q", precondition.Description(), strings.Join(descriptions, "\n")),
+		description: fmt.Sprintf("once %q is met, must have:\n%s", precondition.Description(), description),
+	}
+}
+
+func describeExpectations(expectations ...Expectation) string {
+	var descriptions []string
+	for _, e := range expectations {
+		descriptions = append(descriptions, e.Description())
+	}
+	return strings.Join(descriptions, "\n")
+}
+
+// AnyOf returns an expectation that is satisfied when any of the given
+// expectations is met.
+func AnyOf(anyOf ...Expectation) *SimpleExpectation {
+	check := func(s State) Verdict {
+		for _, e := range anyOf {
+			verdict := e.Check(s)
+			if verdict == Met {
+				return Met
+			}
+		}
+		return Unmet
+	}
+	description := describeExpectations(anyOf...)
+	return &SimpleExpectation{
+		check:       check,
+		description: fmt.Sprintf("Any of:\n%s", description),
 	}
 }