-
Notifications
You must be signed in to change notification settings - Fork 18k
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
Comments
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):
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 |
I don't know if a score change is desirable. Another option is to raise the deep candidate limit for |
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? |
Change https://golang.org/cl/227417 mentions this issue: |
FWIW, I think the |
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. |
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./cc @heschik @findleyr
The text was updated successfully, but these errors were encountered: