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 <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Trust: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/gopls/internal/regtest/expectation.go b/gopls/internal/regtest/expectation.go
index b518984..6c479c3 100644
--- a/gopls/internal/regtest/expectation.go
+++ b/gopls/internal/regtest/expectation.go
@@ -185,6 +185,27 @@
}
}
+// 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
diff --git a/gopls/internal/regtest/reg_test.go b/gopls/internal/regtest/reg_test.go
index f8aae7f..25accb1 100644
--- a/gopls/internal/regtest/reg_test.go
+++ b/gopls/internal/regtest/reg_test.go
@@ -98,8 +98,8 @@
// 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)
}
}
diff --git a/gopls/internal/regtest/workspace_test.go b/gopls/internal/regtest/workspace_test.go
index 7e74b53..f4c21f7 100644
--- a/gopls/internal/regtest/workspace_test.go
+++ b/gopls/internal/regtest/workspace_test.go
@@ -449,7 +449,7 @@
// 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 @@
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 @@
// 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 @@
// 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.
diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go
index a5fbf2b..758b64b 100644
--- a/internal/lsp/fake/editor.go
+++ b/internal/lsp/fake/editor.go
@@ -41,6 +41,14 @@
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 @@
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 @@
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 @@
_ = 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,9 +361,9 @@
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{
@@ -356,6 +371,7 @@
}); err != nil {
return errors.Errorf("DidOpen: %w", err)
}
+ e.calls.DidOpen++
}
return nil
}
@@ -570,6 +586,14 @@
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 @@
if err := e.Server.DidChange(ctx, params); err != nil {
return errors.Errorf("DidChange: %w", err)
}
+ e.calls.DidChange++
}
return nil
}
@@ -652,8 +677,10 @@
}
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
}
