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: friction working with internal/lsp/tests #54845

Open
4 of 9 tasks
findleyr opened this issue Sep 2, 2022 · 10 comments
Open
4 of 9 tasks

x/tools/gopls: friction working with internal/lsp/tests #54845

findleyr opened this issue Sep 2, 2022 · 10 comments
Assignees
Labels
gopls Issues related to the Go language server, gopls. Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Sep 2, 2022

This issue catalogs friction experienced with the LSP "marker tests" in internal/lsp/testdata (run by internal/lsp/tests).

These tests have long been tricky to work with, and recently have caused a significant amount of friction while making changes to error messages in go/parser and go/types.

Notable problems:

  • test output include noisy LSP logs for the entire test session. It would be better if these logs were scoped to the actual failing package, or completely excluded.
  • test output includes red-herring "errors" that are not actually related to the test failure
  • tests are all run in the same session / workspace, so changes in far-away files can affect e.g. completion results
  • auto-generated test names (which include the annotation position) are not stable, and confusing
  • failure messages can be hard to read, because they do a poor job of highlighting differences between expected and actual output.
  • tests often match error messages too precisely, resulting in churn when error messages change across Go versions
  • test annotations are not documented; it is not clear how to add new annotations
  • tests run in multiple contexts (as tests for the internal/lsp/source, internal/lsp/cmd and gopls packages), and this is not clearly documented, nor are the differences between these contexts made clear. (and the necessity for all three contexts is not clear)
  • tests use summary*.txt.golden files (depending on go version) as checksums to ensure that the expected number of tests ran. These are (by construction!) change detectors.

CC @adonovan @griesemer

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Sep 2, 2022
@gopherbot gopherbot added this to the Unreleased milestone Sep 2, 2022
@findleyr findleyr modified the milestones: Unreleased, gopls/later Sep 5, 2022
@adonovan adonovan added the Testing An issue that has been verified to require only test changes, not just a test failure. label Sep 8, 2022
@gopherbot
Copy link

gopherbot commented Sep 19, 2022

Change https://go.dev/cl/431843 mentions this issue: gopls/internal/lsp: suppress noisy log output in tests

@gopherbot
Copy link

gopherbot commented Sep 19, 2022

Change https://go.dev/cl/431844 mentions this issue: gopls/internal/lsp/tests: pass in a *testing.T to Data.Golden

gopherbot pushed a commit to golang/tools that referenced this issue Sep 20, 2022
Disable logging for lsp tests: logs are not scoped to the failing test
and are therefore misleading.

For golang/go#54845

Change-Id: I232e4cfc114382121923e8e697452007793ec3c9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/431843
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>
@gopherbot
Copy link

gopherbot commented Sep 20, 2022

Change https://go.dev/cl/405254 mentions this issue: internal/lsp/source: derive document symbols from syntax alone

@gopherbot
Copy link

gopherbot commented Sep 20, 2022

Change https://go.dev/cl/432138 mentions this issue: gopls/internal/lsp: simplify prepareRename tests

gopherbot pushed a commit to golang/tools that referenced this issue Sep 21, 2022
Data.Golden is called from subtests: use the *testing.T from the caller,
so that we can get a more meaningful failure.

For golang/go#54845

Change-Id: I136df0c6a7a11bcf364b78ecac42ba2b51a15bb0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/431844
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

gopherbot commented Sep 21, 2022

Change https://go.dev/cl/432336 mentions this issue: gopls/internal/lsp/tests: use the mustRange helper in more places

@gopherbot
Copy link

gopherbot commented Sep 21, 2022

Change https://go.dev/cl/432337 mentions this issue: gopls/internal/lsp/tests: simplify collectCompletionItems, remove Data.t

@gopherbot
Copy link

gopherbot commented Sep 22, 2022

Change https://go.dev/cl/432955 mentions this issue: gopls/test: disable stderr output for command line tests as well

gopherbot pushed a commit to golang/tools that referenced this issue Sep 22, 2022
Suppress output in gopls command line tests, similarly to CL 431843.

For golang/go#54845

Change-Id: I7f1e1de52c9a8fb0d902bfdd61bc99d4e2e308b9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/432955
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 23, 2022
The documentSymbols handler joined syntax information with type
information, meaning that it was only able to satisfy requests for files
in valid workspace packages. However, the value added by type
information was limited, and in many cases could be derived from syntax
alone. For example, while generating symbols for a const declaration, we
don't need the type checker to tell us that the symbol kind is const.

Refactor the documentSymbols handler to derive symbols from syntax
alone. This leads to some simplifications from the code, in addition to
eliminating the dependency on package data. Also, simplify symbol
details to just use types.ExprString, which includes some missing
information such as function return values. Also, update handling to
support Go 1.18 type embedding in interfaces.

Notably, this reverts decisions like golang/go#31202, in which we went to
effort to make the symbol kind more accurate. In my opinion (and the
opinion expressed in golang/go#52797), the cost of requiring type
information is not worth the minor improvement in accuracy of the symbol
kind, which (as far as I know) is only used for UI elements.

To facilitate testing (and start to clean up the test framework), make
several simplifications / improvements to the marker tests:
- simplify the collection of symbol data
- eliminate unused marks
- just use cmp.Diff for comparing results
- allow for arbitrary nesting of symbols.
- remove unnecessary @symbol annotations from workspace_symbol tests --
  their data is not used by workspace_symbol handlers
- remove Symbol and WorkspaceSymbol handlers from source_test.go. On
  inspection, these handlers were redundant with lsp_test.go.
Notably, the collection and assembly of @symbol annotations is still way
too complicated. It would be much simpler to just have a single golden
file summarizing the entire output, rather than weaving it together from
annotations. However, I realized this too late, and so it will have to
wait for a separate CL.

Fixes golang/go#52797
Fixes golang/vscode-go#2242
Updates golang/go#54845

Change-Id: I3a2c2d39f59f9d045a6cedf8023ff0c80a69d974
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405254
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 23, 2022
- simplify collection to use an expected span.Span and use mustRange
- remove the source_test.go version of the test, as it is redundant

For golang/go#54845

Change-Id: I3a7da8547e27dc157fb513486a151031ec135746
Reviewed-on: https://go-review.googlesource.com/c/tools/+/432138
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 23, 2022
In order to narrow usage of tests.Data.t, use the mustRange helper in
more places.

For golang/go#54845

Change-Id: I446ca520fa76afb2bc10c1fd5a5765859176dd6a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/432336
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>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 23, 2022
The marker function collectCompletionItems required at least three
arguments. Express this in its signature, leaving a final variadic
argument for documentation. I considered just making this final argument
mandatory, but opted for minimizing the diff given that there are 400+
existing @item annotations.

With this change the only use of tests.Data.t is in mustRange. Since
conversion to range should always succeed, I switched this usage to a
panic and removed the t field.

For golang/go#54845

Change-Id: I407f07cb85fa1356ceb6dba366007f69d1b6a068
Reviewed-on: https://go-review.googlesource.com/c/tools/+/432337
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

gopherbot commented Sep 26, 2022

Change https://go.dev/cl/434635 mentions this issue: gopls: fix the reset_golden.sh script and regenerate golden files

@gopherbot
Copy link

gopherbot commented Sep 26, 2022

Change https://go.dev/cl/434636 mentions this issue: gopls: update to handle 'undefined:' versus 'undeclared' in type errors

gopherbot pushed a commit to golang/tools that referenced this issue Sep 26, 2022
Update the reset_golden.sh script to run the ./test and ./internal/lsp
directories, following the deprecation of ./internal/lsp/cmd and
./internal/lsp/source tests. Also, fix it to generate golden data for
tests that only run at 1.17 and earlier.

Use the newly fixed script to sync the golden data.

For golang/go#54845

Change-Id: I2b42dac91cf1c7e2e8e25fd2aa8ab23c91e05c6c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/434635
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 26, 2022
Update gopls to handle the new form of "undeclared name: ..." error
messages, "undefined: ...", and update tests to be tolerant of both.

Also do some minor cleanup of test error messages related to mismatching
diagnostics.

With this change, TryBots should succeed at CL 432556.

Updates golang/go#54845

Change-Id: I9214d00c59110cd34470845b72d3e2f5e73291c1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/434636
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>
@gopherbot
Copy link

gopherbot commented Sep 27, 2022

Change https://go.dev/cl/435355 mentions this issue: gopls/internal/lsp: tolerate new 'imported and not used' error message

gopherbot pushed a commit to golang/tools that referenced this issue Sep 27, 2022
Tolerate the new form of the "... imported but not used" error message,
to allow landing this change in go/types.

Along the way, improve the test output when comparing diagnostics, and
formatting results.

For golang/go#54845

Change-Id: I998d539f82e0f70c85bdb4c40858be5e01dd08b6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/435355
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
@findleyr findleyr self-assigned this Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

3 participants