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: frequent failures in TestSource/*/testdata/lsp/CompletionSnippets/address_68_11_1_placeholders #38269

Closed
stamblerre opened this issue Apr 6, 2020 · 6 comments
Labels
FrozenDueToAge 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

@stamblerre
Copy link
Contributor

I had thought that this was fixed by https://golang.org/cl/227057, but looks like it's still popping up everywhere. It seems to only happen in the source tests (internal/lsp/source), not the LSP tests (internal/lsp), which makes me wonder if the timeouts are set differently in those tests.

--- FAIL: TestSource/GOPATH/testdata/lsp/CompletionSnippets/address_68_11_1_placeholders (0.03s)
source_test.go:143: /workdir/tmp/TestSource_GOPATH417832833/lsp/src/golang.org/x/tools/internal/lsp/address/address.go:68:11-12: 
couldn't find completion matching "&getFoo().ptr().c"

/cc @heschik @findleyr

@stamblerre stamblerre added this to the gopls/v0.4.0 milestone Apr 6, 2020
@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 Apr 6, 2020
@stamblerre stamblerre added the Testing An issue that has been verified to require only test changes, not just a test failure. label Apr 6, 2020
@stamblerre
Copy link
Contributor Author

stamblerre commented Apr 7, 2020

So, this doesn't seem to be entirely the timeout. Looks like something weird going on with unimported completions getting ranked above deep completions. My guess is that it happens flakily because sometimes the unimported completions get canceled?

From logs I see the following completions (in order):

&syscall.Stderr    9.1
&syscall.Stdin     9.1
&syscall.Stdout    9.1
&getFoo().ptr().c  8.712

Since we cap deep completions at 3 results, the correct result gets lost. A quick fix for the problem would be to turn off unimported completions in that test, but it seems to me like we should have a scoring rule that unimported completions rank below matching deep completions.

/cc @muirdm

@muirdm
Copy link

muirdm commented Apr 7, 2020

I don't know if a score change is desirable. &getFoo().ptr().c is deeper, so all else being equal I'm not sure it should be ranked above the syscall candidates.

Another option is to raise the deep candidate limit for @rank and @snippet. The limit of three deep candidates is only really material when testing @complete and you are testing the entire list of candidates. @rank and @snippet only consider one (or two) particular candidates.

@stamblerre
Copy link
Contributor Author

I'm not sure, I think that candidates that require an additional import should probably be below candidates that are already reachable, at least when the types are the same. In this case, any unimported variable of type int might be ranked higher, which doesn't seem right to me. What do you think @heschik?

@gopherbot
Copy link

Change https://golang.org/cl/227417 mentions this issue: internal/lsp: disable unimported completions for snippet tests

@heschi
Copy link
Contributor

heschi commented Apr 7, 2020

FWIW, I think the syscall candidates actually are more plausible here than the getFoo one. Without having done a thorough analysis it's hard to say but my feeling is that depth is a much stronger signal than imported-ness.

@stamblerre
Copy link
Contributor Author

Ok, maybe I need to think about this more. In general, I do feel like we need to document our scoring rules, but that's a separate issue. Disabling unimported completions in the test seems like the right fix to me.

@golang golang locked and limited conversation to collaborators Apr 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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

4 participants