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

findleyr · 2022-09-02T18:18:54Z

This issue catalogs friction experienced with the "old" 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

The text was updated successfully, but these errors were encountered:

gopherbot · 2022-09-19T22:04:20Z

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

gopherbot · 2022-09-19T22:38:38Z

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

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 · 2022-09-20T20:52:32Z

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

gopherbot · 2022-09-20T22:48:20Z

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

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 · 2022-09-21T13:33:42Z

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

gopherbot · 2022-09-21T13:33:42Z

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

gopherbot · 2022-09-22T18:23:54Z

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

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>

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>

- 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>

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>

@item

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 · 2022-09-26T16:21:00Z

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

gopherbot · 2022-09-26T18:42:54Z

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

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>

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 · 2022-09-27T17:10:41Z

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

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 · 2023-01-19T18:16:37Z

@adonovan and I just discussed where to go with this next, and there are a few major problems we'd like to solve:

the global namespace of marker tests: adding a new test can cause action at a distance
the lack of discoverability for new markers, and generally poor documentation for marker tests
the discrepancy with the regtests (and regtests could be marker tests, but aren't because of 1 and 2)

We just brainstormed a design for solving all of these problems:

represent the marker, collection method, and application method as an instance of a new type
run this new type through the regtest framework

To illustrate what I mean, here is a hypothetical implementation of an isolated marker test:

func TestMarkerTest(t *testing.T) {
	const mod = `
-- go.mod --
module mod.com

go 1.12
-- main.go --
package main

var C = 3

func main() {
	println(C) //@rename("C", "D")
}
-- main.go.rename_C_D_unified --
- var C = 3
+ var D = 3
-	println(D) //@rename("C", "D")
+	println(D) //@rename("C", "D")
`
	RunMarkerTest(t, mod, tests.Rename)
	RunMarkerTest(t, mod, tests.Rename, tests.References)
	RunMarkerTest(t, mod, tests.All...)
}

Port the extraction tests, which involved a mixture of suggestedfix, extractmethod, and extractfunc markers (notably, nothing was running the extractfunc markers!). To do this, extend the new codeaction markers to accept an option slice of titles to use for filtering. Modify the method/func extraction tests to reduce verbosity and long lines. For golang/go#54845 Change-Id: I319d1c731a4cdb4ad952944667bc57d781ad176d Reviewed-on: https://go-review.googlesource.com/c/tools/+/539481 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

gopherbot · 2023-11-09T00:32:08Z

Change https://go.dev/cl/540915 mentions this issue: gopls/internal/regtest/marker: porting remaining suggestedfix tests

gopherbot · 2023-11-09T00:35:59Z

Change https://go.dev/cl/540916 mentions this issue: gopls/internal/regtest/marker: use diffs for rename tests

gopherbot · 2023-11-09T00:49:18Z

Change https://go.dev/cl/539664 mentions this issue: gopls/internal/regtest/marker: trim redundant header from golden diffs

gopherbot · 2023-11-09T02:04:45Z

Change https://go.dev/cl/539665 mentions this issue: gopls/internal/regtest/marker: port remaining rename tests

gopherbot · 2023-11-09T02:40:40Z

Change https://go.dev/cl/540917 mentions this issue: gopls/internal/regtest/marker: port the inlay hint tests

For golang/go#54845 Change-Id: I0b7057420f6bac90161b7861630be584ad7b27ad Reviewed-on: https://go-review.googlesource.com/c/tools/+/540655 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

For golang/go#54845 Change-Id: I04e3400d5e6d53e66ae3093448f68731714553d1 Reviewed-on: https://go-review.googlesource.com/c/tools/+/539482 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

For golang/go#54845 Change-Id: Ica94e365adc2936016d55bdf11b62391e2b47b92 Reviewed-on: https://go-review.googlesource.com/c/tools/+/540915 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Use diffs rather than full file content for rename tests. Otherwise, the golden content is too verbose, particularly for tests that are about to be ported. For golang/go#54845 Change-Id: Ib378355aa85beb9027e6f82bd2063d5fac975ee7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/540916 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>

The unified header adds no value for these diffs. Trim for brevity and readability (in particular because the header clashes visually with the txtar file header). For golang/go#54845 Change-Id: I88223529c5520f82e6751b98aae0c1c4291b56c2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/539664 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

For golang/go#54845 Change-Id: I6d5490703b50dbf944c170384f7f41311e5229bc Reviewed-on: https://go-review.googlesource.com/c/tools/+/539665 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>

For golang/go#54845 Change-Id: Id6b33174392badfc216de389cc95fdc93e3bd8d0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/540917 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

gopherbot · 2023-11-09T19:56:33Z

Change https://go.dev/cl/541196 mentions this issue: gopls/internal/regtest/marker: port the semantic tokens tests

gopherbot · 2023-11-09T20:14:08Z

Change https://go.dev/cl/541126 mentions this issue: gopls/internal/lsp/tests: remove AddImport support

gopherbot · 2023-11-09T20:27:36Z

Change https://go.dev/cl/541127 mentions this issue: gopls/internal/regtest/marker: clean up some random packages

gopherbot · 2023-11-09T21:01:21Z

Change https://go.dev/cl/541129 mentions this issue: gopls/internal/regtest/marker: port the selectionrange markers

These tests weren't actually asserting anything other than the lack of an error in the semantic tokens request (oops). Add some basic test coverage -- we also have regression tests for this feature. Notably, though, we had zero coverage of SemanticTokensRange. For golang/go#54845 Change-Id: Ib7b0df5406fbbbbb34c4a57a8d31395dd9e399d4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/541196 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>

I went to translate the one 'addimport' marker test to the new framework, only to decide that it would be better as a one-off regression test in ./internal/regtest/misc, only to discover that there was already a regression test misc.TestAddImport doing exactly what we want. So this coverage was completely redundant -- just remove it. For golang/go#54845 Change-Id: I1412990a9027b97b2b76c56462fe17c43c5a0c6e Reviewed-on: https://go-review.googlesource.com/c/tools/+/541126 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

%percent and noparse are merged into a strangefiles.txt suite, with the caveat that I have no idea why they existed. We already had a placeholder port of the (disabled) nested_complit test. For golang/go#54845 Change-Id: I81fd6a9c2403a9e10380b890495a51dcf243d2d9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/541127 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>

For golang/go#54845 Change-Id: I289688a677fa6497035912eb2330bb3f0e963392 Reviewed-on: https://go-review.googlesource.com/c/tools/+/541129 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com> Auto-Submit: Robert Findley <rfindley@google.com>

gopherbot · 2023-11-10T16:11:29Z

Change https://go.dev/cl/541236 mentions this issue: gopls/internal/regtest/marker: port remaining marker tests

findleyr · 2023-11-10T17:01:27Z

We ported all these tests to a new marker framework (x/tools/gopls/internal/regtest/marker), which addresses all of the problems we identified with the old tests.

gopherbot · 2023-11-15T21:26:30Z

Change https://go.dev/cl/542636 mentions this issue: gopls: remove the LiteralCompletions option

With the old marker tests gone, it is no longer used. Updates golang/go#54845 Change-Id: If44e6f8b53286cfb66ed0422f30f1d79197a239f Reviewed-on: https://go-review.googlesource.com/c/tools/+/542636 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>

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 added this to the Unreleased milestone Sep 2, 2022

findleyr modified the milestones: Unreleased, gopls/later Sep 5, 2022

adonovan added the Testing An issue that has been verified to require only test changes, not just a test failure. label Sep 8, 2022

findleyr self-assigned this Sep 29, 2022

findleyr mentioned this issue Jan 18, 2023

x/tools/gopls/internal/lsp/source: TestSource failures #57827

Closed

findleyr assigned adonovan Jan 19, 2023

findleyr modified the milestones: gopls/backlog, gopls/v0.15.0 Nov 10, 2023

gopherbot closed this as completed in golang/tools@4479ae8 Nov 10, 2023

findleyr self-assigned this Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

findleyr commented Sep 2, 2022 •

edited

gopherbot commented Sep 19, 2022

gopherbot commented Sep 19, 2022

gopherbot commented Sep 20, 2022

gopherbot commented Sep 20, 2022

gopherbot commented Sep 21, 2022

gopherbot commented Sep 21, 2022

gopherbot commented Sep 22, 2022

gopherbot commented Sep 26, 2022

gopherbot commented Sep 26, 2022

gopherbot commented Sep 27, 2022

findleyr commented Jan 19, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 10, 2023

findleyr commented Nov 10, 2023 •

edited

gopherbot commented Nov 15, 2023

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

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

Comments

findleyr commented Sep 2, 2022 • edited

gopherbot commented Sep 19, 2022

gopherbot commented Sep 19, 2022

gopherbot commented Sep 20, 2022

gopherbot commented Sep 20, 2022

gopherbot commented Sep 21, 2022

gopherbot commented Sep 21, 2022

gopherbot commented Sep 22, 2022

gopherbot commented Sep 26, 2022

gopherbot commented Sep 26, 2022

gopherbot commented Sep 27, 2022

findleyr commented Jan 19, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 9, 2023

gopherbot commented Nov 10, 2023

findleyr commented Nov 10, 2023 • edited

gopherbot commented Nov 15, 2023

findleyr commented Sep 2, 2022 •

edited

findleyr commented Nov 10, 2023 •

edited