x/tools/gopls: frequent failures in TestSource/*/testdata/lsp/CompletionSnippets/address_68_11_1_placeholders

[stamblerre]

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]

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]

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]

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]

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

[heschi]

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]

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.