From 8e32e9f144bc0fca3fc5c250127f11a540c06345 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 9 Jul 2021 12:14:13 -0400 Subject: [PATCH] internal/lsp/regtest: fix a panic TestResolveImportCycle CL 333289 introduced a panic, which was subsequently suppressed in test error output due to the deferred t.Fatal (an interesting gotcha that I honestly wasn't aware of). Fix both the panic, and the suppression of regtest panics. Also fix the regtest editor shutdown to run on a detached context, so that shutdown doesn't fail for tests that have timed out. For golang/go#46773 Change-Id: I080a713ae4cd4651476d8b4aab1d2291754a4f5a Reviewed-on: https://go-review.googlesource.com/c/tools/+/333510 Trust: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- gopls/internal/regtest/misc/shared_test.go | 4 +++- internal/lsp/regtest/expectation.go | 2 +- internal/lsp/regtest/runner.go | 9 ++++++++- internal/lsp/regtest/wrappers.go | 15 --------------- 4 files changed, 12 insertions(+), 18 deletions(-) diff --git a/gopls/internal/regtest/misc/shared_test.go b/gopls/internal/regtest/misc/shared_test.go index e6dc0ac41f..6861743ff4 100644 --- a/gopls/internal/regtest/misc/shared_test.go +++ b/gopls/internal/regtest/misc/shared_test.go @@ -53,7 +53,9 @@ func TestSimultaneousEdits(t *testing.T) { func TestShutdown(t *testing.T) { runShared(t, func(env1 *Env, env2 *Env) { - env1.CloseEditor() + if err := env1.Editor.Close(env1.Ctx); err != nil { + t.Errorf("closing first editor: %v", err) + } // Now make an edit in editor #2 to trigger diagnostics. env2.OpenFile("main.go") env2.RegexpReplace("main.go", "\\)\n(})", "") diff --git a/internal/lsp/regtest/expectation.go b/internal/lsp/regtest/expectation.go index eb2495525c..8fb6afbf40 100644 --- a/internal/lsp/regtest/expectation.go +++ b/internal/lsp/regtest/expectation.go @@ -539,7 +539,7 @@ func EmptyDiagnostics(name string) Expectation { // send at least one diagnostic set for open files. func EmptyOrNoDiagnostics(name string) Expectation { check := func(s State) Verdict { - if diags := s.diagnostics[name]; len(diags.Diagnostics) == 0 { + if diags := s.diagnostics[name]; diags == nil || len(diags.Diagnostics) == 0 { return Met } return Unmet diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go index c245fa77b3..fb17b49283 100644 --- a/internal/lsp/regtest/runner.go +++ b/internal/lsp/regtest/runner.go @@ -29,6 +29,7 @@ import ( "golang.org/x/tools/internal/lsp/lsprpc" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" + "golang.org/x/tools/internal/xcontext" ) // Mode is a bitmask that defines for which execution modes a test should run. @@ -307,7 +308,13 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio if t.Failed() || testing.Verbose() { ls.printBuffers(t.Name(), os.Stderr) } - env.CloseEditor() + // For tests that failed due to a timeout, don't fail to shutdown + // because ctx is done. + closeCtx, cancel := context.WithTimeout(xcontext.Detach(ctx), 5*time.Second) + defer cancel() + if err := env.Editor.Close(closeCtx); err != nil { + t.Errorf("closing editor: %v", err) + } }() // Always await the initial workspace load. env.Await(InitialWorkspaceLoad) diff --git a/internal/lsp/regtest/wrappers.go b/internal/lsp/regtest/wrappers.go index 68e1b742c7..5677ab0416 100644 --- a/internal/lsp/regtest/wrappers.go +++ b/internal/lsp/regtest/wrappers.go @@ -6,14 +6,12 @@ package regtest import ( "encoding/json" - "io" "path" "testing" "golang.org/x/tools/internal/lsp/command" "golang.org/x/tools/internal/lsp/fake" "golang.org/x/tools/internal/lsp/protocol" - errors "golang.org/x/xerrors" ) func (e *Env) ChangeFilesOnDisk(events []fake.FileEvent) { @@ -247,19 +245,6 @@ func (e *Env) DocumentHighlight(name string, pos fake.Pos) []protocol.DocumentHi return highlights } -func checkIsFatal(t testing.TB, err error) { - t.Helper() - if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrClosedPipe) { - t.Fatal(err) - } -} - -// CloseEditor shuts down the editor, calling t.Fatal on any error. -func (e *Env) CloseEditor() { - e.T.Helper() - checkIsFatal(e.T, e.Editor.Close(e.Ctx)) -} - // RunGenerate runs go:generate on the given dir, calling t.Fatal on any error. // It waits for the generate command to complete and checks for file changes // before returning.