gopls/internal/regtest: fix synchronization for TestUseGoplsMod

Jumping to definition in a regtest can indirectly lead to a didOpen
call, so the awaits added to TestUseGoplsMod to synchronize metadata
were ineffectual. On Android, this can lead to the race described in
golang/go#43652.

But in any case, all this bookkeeping of notifications is fragile. Avoid
it entirely by having the fake editor keep track of notification
statistics. In the future, we should use this to clean up many existing
regtests.

For golang/go#43554
For golang/go#39384

Change-Id: Icd1619bd5cdd2f646d1a0050f5beaf2ab1c27f37
Reviewed-on: https://go-review.googlesource.com/c/tools/+/283512
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
Trust: Robert Findley <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>

Author: findleyr

gopls/internal/regtest/expectation.go

@@ -185,6 +185,27 @@ func ShowMessageRequest(title string) SimpleExpectation {
 	}
 }
 
+// DoneWithOpen expects all didOpen notifications currently sent by the editor
+// to be completely processed.
+func (e *Env) DoneWithOpen() Expectation {
+	opens := e.Editor.Stats().DidOpen
+	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), opens)
+}
+
+// DoneWithChange expects all didChange notifications currently sent by the
+// editor to be completely processed.
+func (e *Env) DoneWithChange() Expectation {
+	changes := e.Editor.Stats().DidChange
+	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), changes)
+}
+
+// DoneWithChangeWatchedFiles expects all didChangeWatchedFiles notifications
+// currently sent by the editor to be completely processed.
+func (e *Env) DoneWithChangeWatchedFiles() Expectation {
+	changes := e.Editor.Stats().DidChangeWatchedFiles
+	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), changes)
+}
+
 // CompletedWork expects a work item to have been completed >= atLeast times.
 //
 // Since the Progress API doesn't include any hidden metadata, we must use the

gopls/internal/regtest/reg_test.go

@@ -98,8 +98,8 @@ func TestMain(m *testing.M) {
 		// Regtest cleanup is broken in go1.12 and earlier, and sometimes flakes on
 		// Windows due to file locking, but this is OK for our CI.
 		//
-		// Fail on non-windows go1.13+.
-		if testenv.Go1Point() >= 13 && runtime.GOOS != "windows" {
+		// Fail on go1.13+, except for windows and android which have shutdown problems.
+		if testenv.Go1Point() >= 13 && runtime.GOOS != "windows" && runtime.GOOS != "android" {
 			os.Exit(1)
 		}
 	}

gopls/internal/regtest/workspace_test.go

@@ -449,7 +449,7 @@ replace a.com => $SANDBOX_WORKDIR/moda/a
 		// loaded. Validate this by jumping to a definition in b.com and ensuring
 		// that we go to the module cache.
 		env.OpenFile("moda/a/a.go")
-		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1))
+		env.Await(env.DoneWithOpen())
 
 		// To verify which modules are loaded, we'll jump to the definition of
 		// b.Hello.
@@ -479,11 +479,12 @@ require (
 replace a.com => %s/moda/a
 replace b.com => %s/modb
 `, workdir, workdir))
-		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 1))
+		env.Await(env.DoneWithChangeWatchedFiles())
 		// Check that go.mod diagnostics picked up the newly active mod file.
 		// The local version of modb has an extra dependency we need to download.
 		env.OpenFile("modb/go.mod")
-		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 2))
+		env.Await(env.DoneWithOpen())
+
 		var d protocol.PublishDiagnosticsParams
 		env.Await(
 			OnceMet(
@@ -501,7 +502,7 @@ replace b.com => %s/modb
 		// Now, let's modify the gopls.mod *overlay* (not on disk), and verify that
 		// this change is only picked up once it is saved.
 		env.OpenFile("gopls.mod")
-		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 3))
+		env.Await(env.DoneWithOpen())
 		env.SetBufferContent("gopls.mod", fmt.Sprintf(`module gopls-workspace
 
 require (
@@ -514,7 +515,7 @@ replace a.com => %s/moda/a
 		// Editing the gopls.mod removes modb from the workspace modules, and so
 		// should clear outstanding diagnostics...
 		env.Await(OnceMet(
-			CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 2),
+			env.DoneWithChange(),
 			EmptyDiagnostics("modb/go.mod"),
 		))
 		// ...but does not yet cause a workspace reload, so we should still jump to modb.

internal/lsp/fake/editor.go

@@ -41,6 +41,14 @@ type Editor struct {
 	buffers map[string]buffer
 	// Capabilities / Options
 	serverCapabilities protocol.ServerCapabilities
+	// Call metrics for the purpose of expectations. This is done in an ad-hoc
+	// manner for now. Perhaps in the future we should do something more
+	// systematic.
+	calls CallCounts
+}
+
+type CallCounts struct {
+	DidOpen, DidChange, DidChangeWatchedFiles int
 }
 
 type buffer struct {
@@ -131,6 +139,12 @@ func (e *Editor) Connect(ctx context.Context, conn jsonrpc2.Conn, hooks ClientHo
 	return e, nil
 }
 
+func (e *Editor) Stats() CallCounts {
+	e.mu.Lock()
+	defer e.mu.Unlock()
+	return e.calls
+}
+
 // Shutdown issues the 'shutdown' LSP notification.
 func (e *Editor) Shutdown(ctx context.Context) error {
 	if e.Server != nil {
@@ -282,6 +296,7 @@ func (e *Editor) onFileChanges(ctx context.Context, evts []FileEvent) {
 		return
 	}
 	e.mu.Lock()
+	defer e.mu.Unlock()
 	var lspevts []protocol.FileEvent
 	for _, evt := range evts {
 		// Always send an on-disk change, even for events that seem useless
@@ -302,10 +317,10 @@ func (e *Editor) onFileChanges(ctx context.Context, evts []FileEvent) {
 			_ = e.setBufferContentLocked(ctx, evt.Path, false, strings.Split(content, "\n"), nil)
 		}
 	}
-	e.mu.Unlock()
 	e.Server.DidChangeWatchedFiles(ctx, &protocol.DidChangeWatchedFilesParams{
 		Changes: lspevts,
 	})
+	e.calls.DidChangeWatchedFiles++
 }
 
 // OpenFile creates a buffer for the given workdir-relative file.
@@ -346,16 +361,17 @@ func (e *Editor) createBuffer(ctx context.Context, path string, dirty bool, cont
 		dirty:   dirty,
 	}
 	e.mu.Lock()
+	defer e.mu.Unlock()
 	e.buffers[path] = buf
 	item := textDocumentItem(e.sandbox.Workdir, buf)
-	e.mu.Unlock()
 
 	if e.Server != nil {
 		if err := e.Server.DidOpen(ctx, &protocol.DidOpenTextDocumentParams{
 			TextDocument: item,
 		}); err != nil {
 			return errors.Errorf("DidOpen: %w", err)
 		}
+		e.calls.DidOpen++
 	}
 	return nil
 }
@@ -570,6 +586,14 @@ func (e *Editor) SetBufferContent(ctx context.Context, path, content string) err
 	return e.setBufferContentLocked(ctx, path, true, lines, nil)
 }
 
+// HasBuffer reports whether the file name is open in the editor.
+func (e *Editor) HasBuffer(name string) bool {
+	e.mu.Lock()
+	defer e.mu.Unlock()
+	_, ok := e.buffers[name]
+	return ok
+}
+
 // BufferText returns the content of the buffer with the given name.
 func (e *Editor) BufferText(name string) string {
 	e.mu.Lock()
@@ -629,6 +653,7 @@ func (e *Editor) setBufferContentLocked(ctx context.Context, path string, dirty
 		if err := e.Server.DidChange(ctx, params); err != nil {
 			return errors.Errorf("DidChange: %w", err)
 		}
+		e.calls.DidChange++
 	}
 	return nil
 }
@@ -652,8 +677,10 @@ func (e *Editor) GoToDefinition(ctx context.Context, path string, pos Pos) (stri
 	}
 	newPath := e.sandbox.Workdir.URIToPath(resp[0].URI)
 	newPos := fromProtocolPosition(resp[0].Range.Start)
-	if err := e.OpenFile(ctx, newPath); err != nil {
-		return "", Pos{}, errors.Errorf("OpenFile: %w", err)
+	if !e.HasBuffer(newPath) {
+		if err := e.OpenFile(ctx, newPath); err != nil {
+			return "", Pos{}, errors.Errorf("OpenFile: %w", err)
+		}
 	}
 	return newPath, newPos, nil
 }