Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: add regression testing #36879

Closed
stamblerre opened this issue Jan 29, 2020 · 17 comments
Closed

x/tools/gopls: add regression testing #36879

stamblerre opened this issue Jan 29, 2020 · 17 comments
Assignees
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jan 29, 2020

We've been using the govim tests for regression testing, which creates unnecessary load on @myitcv in filing and reproducing issues. It also places too much reliance on one specific editor client. We should develop our own regression tests that use a fake client.

/cc @findleyr

@gopherbot gopherbot added this to the Unreleased milestone Jan 29, 2020
@stamblerre stamblerre removed this from the Unreleased milestone Jan 29, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jan 29, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 4, 2020

Change https://golang.org/cl/217598 mentions this issue: internal/lsp/fake: add fakes for testing editor interaction

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 6, 2020

Change https://golang.org/cl/218278 mentions this issue: internal/lsp/lsprpc: add test for empty diagnostics in deleted files

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 6, 2020

Change https://golang.org/cl/218322 mentions this issue: internal/lsp/lsprpc: add test for definition outside of workspace

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 9, 2020

Change https://golang.org/cl/218698 mentions this issue: internal/lsp/lsprpc: add an LSP forwarder and regtest environment

gopherbot pushed a commit to golang/tools that referenced this issue Feb 10, 2020
A lot of bug reports originating from LSP clients are related to either
the timing or sequence of editor interactions with gopls (or at least
they're originally reported this way). For example: "when I open a
package and then create a new file, I lose diagnostics for existing
files".  These conditions are often hard to reproduce, and to isolate as
either a gopls bug or a bug in the editor.

Right now we're relying on govim integration tests to catch these
regressions, but it's important to also have a testing framework that
can exercise this functionality in-process.  As a starting point this CL
adds test fakes that implement a high level API for scripting editor
interactions. A fake workspace can be used to sandbox file operations; a
fake editor provides an interface for text editing operations; a fake
LSP client can be used to connect the fake editor to a gopls instance.
Some tests are added to the lsprpc package to demonstrate the API.

The primary goal of these fakes should be to simulate an client that
complies to the LSP spec. Put another way: if we have a bug report that
we can't reproduce with our regression tests, it should either be a bug
in our test fakes or a bug in the LSP client originating the report.

I did my best to comply with the spec in this implementation, but it
will certainly develop as we write more tests. We will also need to add
to the editor API in the future for testing more language features.

Updates golang/go#36879
Updates golang/go#34111

Change-Id: Ib81188683a7066184b8a254275ed5525191a2d68
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217598
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 10, 2020
Add a test for the bug reported in golang/go#37049: we are missing empty
diagnostics for deleted files. Doing this involved added a missing
RemoveFile method on the fake.Watcher type.

Skip the test for now, as it is failing.

Updates golang/go#37049
Updates golang/go#36879

Change-Id: Ib3b6907455cc44a2e6af00c2254aa444e9480749
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218278
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 10, 2020
Add regression tests for GoToDefinition. In particular, exercise the
panic from golang/go#37045.

Updates golang/go#37045
Updates golang/go#36879

Change-Id: I67b562acd293f47907de0435c14b62c1a22cf2ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218322
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 10, 2020

Change https://golang.org/cl/218777 mentions this issue: internal/jsonrpc2/servertest: support both TCP and pipe connection

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 10, 2020

Change https://golang.org/cl/218778 mentions this issue: internal/lsp/lsprpc: add a forwarder handler

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 10, 2020

Change https://golang.org/cl/218839 mentions this issue: internal/jsonrpc2: support serving over unix domain sockets

gopherbot pushed a commit to golang/tools that referenced this issue Feb 14, 2020
Add a new Forwarder type to the lsprpc package, which implements the
jsonrpc2.StreamServer interface. This will be used to establish some
parity in the implementation of shared and singleton gopls servers.

Much more testing is needed, as is handling for the many edge cases
around forwarding the LSP, but since this is functionally equivalent to
TCP forwarding (and the -remote flag was already broken), I went ahead
and used the Forwarder to replace the forward method in the serve
command. This means that we can now use the combination of -listen and
-remote to chain together gopls servers... not that there's any reason
to do this.

Also, wrap the new regression tests with a focus on expressiveness when
testing the happy path, as well as parameterizing them so that they can
be run against different client/server execution environments. This
started to be sizable enough to warrant moving them to a separate
regtest package. The lsprpc package tests will instead focus on unit
testing the client-server binding logic.

Updates golang/go#36879
Updates golang/go#34111

Change-Id: Ib98131a58aabc69299845d2ecefceccfc1199574
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218698
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 16, 2020
Update the servertest package to support connecting to a jsonrpc2 server
using either TCP or io.Pipes. The latter is provided so that regtests
can more accurately mimic the current gopls execution mode, where gopls
is run as a sidecar and communicated with via a pipe.

Updates golang/go#36879

Change-Id: I0e14ed0e628333ba2cc7b088009f1887fcaa82a5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218777
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 19, 2020
Add a forwarder handler that alters messages before forwarding, for now,
it just intercepts the "exit" message.

Also, make it easier to write regression tests for a shared gopls
instance, by adding a helper that instantiates two connected
environments, and only runs in the shared execution modes.

Updates golang/go#36879
Updates golang/go#34111

Change-Id: I7673f72ab71b5c7fd6ad65d274c15132a942e06a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218778
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 19, 2020
For tests (and perhaps later, for daemon discovery), unix domain sockets
offer advantages over TCP: we can know the exact socket address that will be
used when starting a server subprocess. They also offer performance and
security advantages over TCP, and were specifically requested on
golang.org/issues/34111.

This CL adds support for listening on UDS, and uses this to implement an
additional regtest environment mode that starts up an external process.
This mode is disabled by default, but may be enabled by the
-enable_gopls_subprocess_tests.

The regtest TestMain may be hijacked to instead run as gopls, if a
special environment variable is set. This allows the the test runner to
start a separate process by using os.Argv[0]. The -gopls_test_binary
flag may be used to point tests at a separate gopls binary.

Updates golang/go#36879
Updates golang/go#34111

Change-Id: I1cfdf55040e81ffa69a6726878a96529e5522e82
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218839
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 25, 2020

Change https://golang.org/cl/220902 mentions this issue: internal/lsp/regtest: clean-up and more error handling

gopherbot pushed a commit to golang/tools that referenced this issue Feb 25, 2020
Fix various things related to regtest execution:
 + Check the error from OpenFile in fake.Editor.GoToDefinition.
 + Add an error-checked wrapper to env for CloseBuffer.
 + Use env wrappers in TestDiagnosticClearingOnClose.
 + Use os.Executable to get the test binary path.
 + Add a -listen.timeout to the remote gopls process, so that it is
   cleaned up.

Updates golang/go#36879

Change-Id: I056ff50bdb611a78ad78e4f5e2a94a4f155cc9de
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220902
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2020

Change https://golang.org/cl/221539 mentions this issue: internal/lsp/regtest: implement formatting and organizeImports

gopherbot pushed a commit to golang/tools that referenced this issue Mar 2, 2020
Add two new fake editor commands: Formatting and OrganizeImports, which
delegate to textDocument/formatting and textDocument/codeAction
respectively. Use this in simple regtests, as well as on save.

Implementing this required fixing a broken assumption about text edits
in the editor: previously these edits were incrementally mutating the
buffer, but the correct implementation should simultaneously mutate the
buffer (i.e., all positions in an edit set refer to the starting buffer
state). This never mattered before because we were only operating on one
edit at a time.

Updates golang/go#36879

Change-Id: I6dec343c4e202288fa20c26df2fbafe9340a1bce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221539
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 24, 2020

Change https://golang.org/cl/224757 mentions this issue: internal/lsp/fake: add regexp search and replace

gopherbot pushed a commit to golang/tools that referenced this issue Mar 24, 2020
Expressing regtests in terms of textual coordinates is hard to read: the
reader ends up counting lines and characters to understand the text edit
or assertion.

To address, this, add two new functions for fake.Editor: RegexpSearch
and RegexpReplace, as well as a symmetric RegexpSearch function for
workspace files and wrappers for regtext.Env.

This allows expressing edits as well as buffer locations in terms of
easily scannable regexps.

An alternative solution to this problem is to integrate markers ala
packagestest. I tried this, but it ended up being cumbersome to
implement and less usable than regexps, due to the static nature of
markers: after the buffer has been edited all markers must be
updated.

Updates golang/go#36879

Change-Id: Iad087cf0d529737034197beef7b729816a159c69
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224757
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Mar 25, 2020

A couple thoughts as I work on https://golang.org/cl/225317:

  • It would be helpful to be able to get the gopls logs for a test after it runs.
  • One useful application for generating tests from logs would be the ability to automatically generate a sequence of calls to the appropriate functions for a sequence of text synchronization events. In my case, the repro is a bit complicated, and I still haven't been able to get the right ordering--the issue reproduces differently in VS Code and in my test. It would be great if I could just pass my VS Code log into a generator and have it produce the correct series of calls. (cc @pjweinb)

Update: Turns out that the cause of my difficulties reproducing this was #35230, not the actual sequence of file opens/changes/etc.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 10, 2020

Change https://golang.org/cl/227657 mentions this issue: internal/lsp/regtest: add support for custom proxy data

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 14, 2020

Change https://golang.org/cl/228235 mentions this issue: internal/lsp/regtest: add support for custom test proxy data

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 15, 2020

Change https://golang.org/cl/228399 mentions this issue: internal/lsp/regtest: generalize expecations beyond diagnostics

gopherbot pushed a commit to golang/tools that referenced this issue Apr 16, 2020
Certain regtests require referencing external data. To support this, add
the ability to use a file-based proxy populated with testdata.

To expose this configuration, augment the regtest runner with variadic
options. Also use this to replace the Runner.RunInMode function.

Add a simple regtest that uses this functionality.

Updates golang/go#36879

Change-Id: I7e6314430abcd127dbb7bca12574ef9935bf1f83
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228235
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Apr 16, 2020
Due to the asynchronous and non-transactional nature of the LSP, writing
regtests requires waiting for certain conditions to be met in the client
editor. To this point, the only type of condition for which we supported
waiting was the presence of diagnostic messages, but we can in principle
wait for anything that is triggered by a server notification.

This change generalizes the notion of expectations to also encompass log
messages. Doing this required expanding the value returned from checking
expectations to include a new "Unmeetable" verdict, to account for cases
where we know that a condition will never be met (for example if it is a
negative assertion). This may be useful for diagnostics as well.

A test is added to demonstrate these new expectations, where the initial
workspace load fails due to unfetchable dependencies.

Additionally, some helper flags are added to help debug regtests without
a code change, by allowing changing the test timeout, printing logs, and
printing goroutine profiles.

Updates golang/go#36879

Change-Id: I4cc194e10a4f181ad36a1a7abbb08ff41954b642
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228399
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 23, 2020

Change https://golang.org/cl/229777 mentions this issue: internal/lsp: instrument work done reporting to use in regtests

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 23, 2020

Change https://golang.org/cl/229782 mentions this issue: internal/lsp/regtest: add expository package documentation

gopherbot pushed a commit to golang/tools that referenced this issue Apr 28, 2020
In order for regtests to wait until file diagnostics are complete,
instrument diagnostics with verbose WorkDone reporting. In order for
this to be granular enough for use, the modification source needed to be
threaded through to the didModifyFiles function (which is where the
diagnostic goroutine is spun off).

A new expectation is added: CompletedWork, to allow specifying that a
specific work item has been completed. The problem with using
NoOutstandingWork was that it required a continuous chain of work to
prevent the regtest from succeeding when the bug was present, meaning
that by the time we have sent the didChange notification successfully
the server must have started work on its behalf. This was inherently
racy, and too tricky to get right.

Additionally, a couple bugs are fixed:
 - EmptyDiagnostics is corrected to account for the case where we have
   received zero diagnostics for a given file.
 - A deadlock is fixed in Await when expectations are immediately met.

Updates golang/go#36879
Fixes golang/go#32149

Change-Id: I49ee011860351eed96a3b4f6795804b57a10dc60
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229777
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 28, 2020

Closing this now that the regtest framework is somewhat mature and documented, and we have decent test coverage. We will continue to add more tests and improve the framework, but I believe this tracking issue has served its purpose. In the future, we should open new issues for any specific regtest bugs or feature requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.