-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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/internal/lsp/regtest: tracking various regtest improvements #39384
Comments
Change https://golang.org/cl/240059 mentions this issue: |
Change https://golang.org/cl/240058 mentions this issue: |
In the past, changes to GOPACKAGESDRIVER have led to some confusing regtest failures. Explicitly set it off. Updates golang/go#39384 Change-Id: I303a58380a5e46e6621c19b2edc40d43199bb343 Reviewed-on: https://go-review.googlesource.com/c/tools/+/240058 Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Rebecca Stambler <rstambler@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
Change https://golang.org/cl/241739 mentions this issue: |
For easier debugging (and less cruft if regtests are ctrl-c'ed), root all regtest sandboxes in a common directory. This also tries one last time to clean up the directory, and fails on an error. This might be flaky on windows, but hasn't been so far... Also give regtest sandboxes names derived from their test name. Updates golang/go#39384 Updates golang/go#38490 Change-Id: Iae53c29e75f5eb2b8d938d205fbeb463ffc82eb2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/240059 Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Change https://golang.org/cl/243057 mentions this issue: |
This is an alternative to CL 241739, preserving the exiting variadic options pattern and just adding a helper method to put the options up front. All things considered, I think this is better. CL 241739 was too complicated, and being able to reuse "curried" runners isn't actually important. Updates golang/go#39384 Change-Id: I7ecd80d310cac879520b2d1feebcf5bd5e96e89b Reviewed-on: https://go-review.googlesource.com/c/tools/+/243057 Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Heschi Kreinick <heschi@google.com>
Change https://golang.org/cl/252683 mentions this issue: |
Change https://golang.org/cl/252682 mentions this issue: |
Regtests are slow, and make `go test ./internal/lsp/...` slow. Also, having them in the tools module means they can't use staticcheck, go-diff, etc. Move them to the gopls module. This means that they're annoying to work with unless you open the gopls module, but hopefully that annoyance will be gone soon when we support multi-module workspaces. For golang/go#39384 Change-Id: Ib99c994ffdac56d4da13af981ad397a90a7523af Reviewed-on: https://go-review.googlesource.com/c/tools/+/252682 Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>
To match the actual gopls binary, use hooks.Options when creating the regtest server. Add a test for staticcheck diagnostics to leverage this. For golang/go#39384 Change-Id: I52837c2b12bb586a2530343bdfae5172b08df49c Reviewed-on: https://go-review.googlesource.com/c/tools/+/252683 Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Change https://golang.org/cl/255126 mentions this issue: |
Returning 'metBy' from expectations was not an ideal API, as extracting the result value was cumbersome and error prone. It also forced quite a bit of plumbing. Using OnceMet, we can improve this by instead using a 'ReadDiagnostics' expectation that reads diagnostics into a variable. For golang/go#39384 Change-Id: Ia73e5b496089240df3200626e5696305cb507c9a Reviewed-on: https://go-review.googlesource.com/c/tools/+/255126 Trust: Robert Findley <rfindley@google.com> Run-TryBot: Robert Findley <rfindley@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Change https://golang.org/cl/283512 mentions this issue: |
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>
Change https://golang.org/cl/287572 mentions this issue: |
Regtests have gotten large, and started timing out on some slow builders. They also can't yet be run in parallel, due to some process-level shared state (e.g. gc_details). Furthermore, there seems to be some per-process resource leak that we don't yet fully understand causing slowdown as the tests progress. Address these problems by splitting the regtests up into multiple packages. This also makes it easier to run only relevant tests during development. For golang/go#42789 For golang/go#39384 Change-Id: I1a74e4c379f3a650f4c434db44f9368e527aa459 Reviewed-on: https://go-review.googlesource.com/c/tools/+/287572 Trust: Robert Findley <rfindley@google.com> Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Rebecca Stambler <rstambler@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org>
Change https://go.dev/cl/416876 mentions this issue: |
Configuration of LSP settings within the regression test runner had become a bit of a grab-bag: some were configured via explicit fields on EditorConfig, some via the catch-all EditorConfig.Settings field, and others via custom RunOption implementations. Consolidate these fields as follows: - Add an EnvVars and Settings field, for configuring environment and LSP settings. - Eliminate the EditorConfig RunOption wrapper. RunOptions help build the config. - Remove RunOptions that just wrap a key-value settings pair. By definition settings are user-facing and cannot change without breaking compatibility. Therefore, our tests can and should set the exact string keys they are using. - Eliminate the unused SendPID option. Also clean up some logic to change configuration. For golang/go#39384 Change-Id: Id5d1614f139550cbc62db2bab1d1e1f545ad9393 Reviewed-on: https://go-review.googlesource.com/c/tools/+/416876 TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Alan Donovan <adonovan@google.com>
Change https://go.dev/cl/417587 mentions this issue: |
Change https://go.dev/cl/418774 mentions this issue: |
Each regtest does a significant amount of extra work re-doing things like parsing and type-checking the runtime package. We can share this work across regtests by using a shared cache, significantly speeding them up at the cost of potentially hiding bugs related to timing. Sharing this work still retains most of the benefit of the regtests, so implement this in the default mode (formerly called "singleton" and now renamed to "default"). In a subsequent CL, modes will be cleaned up so that "default" is the only mode that runs with -short. Making this change actually revealed a caching bug: our cached package stores error messages extracted from go/packages errors, but does not include these errors in the cache key. Fix this by hashing all metadata errors into the package cache key. Updates golang/go#39384 Change-Id: I37ab9604149d34c9a79fc02b0e1bc23fcb17c454 Reviewed-on: https://go-review.googlesource.com/c/tools/+/417587 TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Bryan Mills <bcmills@google.com>
Due to shared caching, the "default" tests can run faster than other execution modes and still retain most of the test coverage. Update the test runner to only run the singleton tests if testing.Short() is true, independent of GOOS. On the other hand, we lost noticeable coverage when disabling the Forwarded testing mode. Now that the regtests are lighter weight in general, reenable it on the longtests builders. While at it, clean up tests that used the server-side Options hook to instead use Settings, since clients would configure their server via Settings and Options can't work with a shared daemon. Updates golang/go#39384 Change-Id: I33e8b746188d795e88841727e6b7116cd4851dc2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/418774 Reviewed-by: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
Change https://go.dev/cl/461801 mentions this issue: |
Change https://go.dev/cl/461915 mentions this issue: |
Change https://go.dev/cl/461916 mentions this issue: |
Change https://go.dev/cl/461917 mentions this issue: |
Change https://go.dev/cl/461935 mentions this issue: |
Change https://go.dev/cl/461936 mentions this issue: |
Change https://go.dev/cl/461937 mentions this issue: |
Change https://go.dev/cl/461938 mentions this issue: |
Change https://go.dev/cl/461939 mentions this issue: |
Change https://go.dev/cl/461897 mentions this issue: |
It doesn't matter from the user's perspective whether gopls has sent empty diagnostics, or no diagnostics. As long as we use an AfterChange check (or have previously asserted that diagnostics are non-empty), there is no problem replacing the EmptyDiagnostics expectation with NoDiagnostics. Using this principle, eliminate uses of EmptyDiagnostics. Updates golang/go#39384 Change-Id: I5940c46bbb9943a930aeedbae74c3d3314b13574 Reviewed-on: https://go-review.googlesource.com/c/tools/+/461801 Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
The number of different regtest expectations related to diagnostics has grown significantly over time. Start to consolidate to just two expectations: Diagnostics, which asserts on the existence of diagnostics, and NoMatchingDiagnostics, which asserts on the nonexistence of diagnostics. Both accept an argument list to filter the set of diagnostics under consideration. In a subsequent CL, NoMatchingDiagnostics will be renamed to NoDiagnostics, once the existing expectation with that name has been replaced. Use this to eliminate the following expectations: - DiagnosticAtRegexpFromSource -> Diagnostics(AtRegexp, FromSource) - NoDiagnosticAtRegexp -> NoMatchingDiagnostics(AtRegexp) - NoOutstandingDiagnostics -> NoMatchingDiagnostics Updates golang/go#39384 Change-Id: I518b14eb00c9fcf62388a03efb668899939a8f82 Reviewed-on: https://go-review.googlesource.com/c/tools/+/461915 Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Remove the redundant GoSumDiagnostic. Updates golang/go#39384 Change-Id: I742fbe5d32dd55288c7632bed335f0d33e1015d5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/461916 gopls-CI: kokoro <noreply+kokoro@google.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com>
Simplify env.Await(OnceMet(...)) to env.OnceMet(...). Aside from avoiding boilerplate, this makes it easier to identify where we're still using Await. Updates golang/go#39384 Change-Id: I57a18242ce6b48e371e5ce4876ef01a6774fe15c Reviewed-on: https://go-review.googlesource.com/c/tools/+/461917 Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
Especially with the recent consolidation of position mapping into protocol.Mapper, there is no need for the fake package to use anything but protocol representation for positions, ranges, and edits. Make this change, eliminating redundant types and conversions, and simplifying the code. Notably, the representation of buffers as slices of lines is no longer useful. It proved most convenient to instead store a protocol.Mapper. Updates golang/go#39384 Change-Id: I8c131e55c4c281bd1fc2db6e5620cdfae0ebdee5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/461935 Run-TryBot: Robert Findley <rfindley@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Replace uses of NoDiagnostics with the more flexible NoMatchingDiagnostics, and rename NoMatchingDiagnostics to NoDiagnostics. Updates golang/go#39384 Change-Id: I15b19ad6c9b58c1ae88ec1b444bb589002f75a80 Reviewed-on: https://go-review.googlesource.com/c/tools/+/461936 gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Replace the DiagnosticAt expectation with a filter. Updates golang/go#39384 Change-Id: I29e61a531beb2097a88266943917b0ae43630e3f Reviewed-on: https://go-review.googlesource.com/c/tools/+/461937 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Alan Donovan <adonovan@google.com>
Replace with assertions on filtered diagnostics. Updates golang/go#39384 Change-Id: If88e29a8241008dd778fbfabe37d840b48f17691 Reviewed-on: https://go-review.googlesource.com/c/tools/+/461938 gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com>
Replace DiagnosticAtRegexp with diagnostic filters. This allowed eliminating DiagnosticExpectation, which was the only complicated implementation of the Expectation interface. Replace Expectation with the concrete (formerly named) SimpleExpectation. Updates golang/go#39384 Change-Id: I6716e869609dce9777025557494c8f81a606e4ff Reviewed-on: https://go-review.googlesource.com/c/tools/+/461939 Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com>
Consolidate the redundant env.Symbol and env.WorkspaceSymbol wrappers, and eliminate the unnecessary wrapper types. Updates golang/go#39384 Change-Id: Ibe3b7ca89c531a914e5044a7dc45ac30e7210f1b Reviewed-on: https://go-review.googlesource.com/c/tools/+/461897 gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Change https://go.dev/cl/462495 mentions this issue: |
Update all the places (I think) where we await diagnostics unbounded, except for a couple that can't yet be made eager due to lacking instrumentation (for example, around didChangeConfiguration). Updates golang/go#39384 Change-Id: Ib00d28ce0350737d39a123fd200fd435cc89f967 Reviewed-on: https://go-review.googlesource.com/c/tools/+/462495 gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Robert Findley <rfindley@google.com>
I think we've done enough here. |
This is a follow-up to #36879. We've been using the gopls regtests for a few months now, and several usability bugs / missing features / overall themes have emerged. None of these seemed worthy of an issue, but I want to track them in aggregate. I've been trying to let the regtests soak in their current form, but plan to take another pass soon.
Here's a list of what I've noted along the way, either from my own usage or feedback from others'. Feel free to comment with more, and I'll edit this comment to keep it up to date.
-regtest_timeout
to avoid waiting for the default regtest timeout).runner.Run
is hard to spot. Use a different options pattern to allow passing them before the test body.go
command). Perhaps awaiting 'NoOutstandingWork' could help here.gopls
module, so that they can run staticcheck.gopls
module, replace the synthetic file watching with actual file watching, using fsnotify. The fact that file-watching is faked has been a source of bugs. The file watching should also respect the GlobPattern provided in the file watching capability registration.DiagnosticAtRegexp
expectation lazy, so that it doesn't eagerly evaluate the regexp position (and therefore no longer needs theEnv
receiver).DiagnosticAtRegexp
match the full range of the regexp match, not just the starting position (this was a cause of some confusion in a CL).Return a copy of(the underlying requirement was instead achieved with less effort by adding a 'ReadDiagnostics' expectation that atomically reads from State).State
from Await: whatever caused expectations to be be metGOPACKAGESDRIVER=off
for all regtests.TMPDIR
for the entire regtest suite, rather than letting each regtest create it's own sandbox in/tmp
. This reduces cruft in/tmp
when tests are ctrl-c'ed before they can clean themselves up.CC @pjweinbgo @stamblerre @heschik
The text was updated successfully, but these errors were encountered: