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

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 6, 2020

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
Copy link
Contributor Author

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

@stamblerre stamblerre commented Apr 7, 2020

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

@gopherbot gopherbot commented Apr 7, 2020

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

@heschik
Copy link
Contributor

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

@stamblerre stamblerre commented Apr 7, 2020

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.

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
4 participants
You can’t perform that action at this time.