Add LSP test for completions #4256
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
stefanvanburen
left a comment
stefanvanburen
left a comment
There was a problem hiding this comment.
I'm happy to iterate on these (and realize I didn't necessarily exhaust all of these in my own tests), but I think it'd also be good to have tests covering syntax, packages and imports, options, and any of the specialized cases we have based on the parentDef. (We can follow-up on missing ones, though, especially if we're working through fixing some of this.)
| for _, expected := range tt.expectedContains { | ||
| assert.Contains(t, labels, expected, "expected completion list to contain %q", expected) | ||
| } |
There was a problem hiding this comment.
I realize I may have done a similar thing in my tests, but would it make sense to do an assert.ElementsMatch (ignoring ordering) so we know if the full set of completions changes? or is that too noisy (i.e., too many completion suggestions)?
There was a problem hiding this comment.
I think theres too many.
| expectedNotContains []string | ||
| expectNoCompletions bool |
There was a problem hiding this comment.
nit: we don't use either of these — not sure if expectedNotContains is useful if we switch to using assert.ElementsMatch below, but can probably remove it, and it might be nice to have a couple of negative tests for places where we don't have completion suggestions (using expectNoCompletions).
There was a problem hiding this comment.
We are missing a bunch here, was going to follow up with some fixes needed. Theres an issue with state which isn't captured in this test. I think we need to have an update, followed by a completion to show this issue.
This adds basic autocompletion tests to the LSP.