Add LSP hover tests #4254
Add LSP hover tests #4254
Conversation
This takes an initial stab at adding some tests to the LSP implementation, starting with the hover feature. (I figure we can share the server across the other features.) We've been bitten a couple times by breaking changes that would be great to catch with tests.
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
emcfarlane
left a comment
emcfarlane
left a comment
There was a problem hiding this comment.
Awesome, this is exactly what we needed. I can help followup with coverage.
Haven't been able to figure out how to make the paths work; maybe someone else more familiar with testing windows would know. And use t.Context instead of parameter.
|
Ended up skipping the tests on windows for now; if we can figure out how to make the paths work we ought to re-enable them. |
doriable
left a comment
doriable
left a comment
There was a problem hiding this comment.
I think overall looks good, I'm pre-approving because I hope that adjustments are minimal. Thanks for doing this, this is super helpful and will hopefully make changes to the LSP more stable.
|
Going to merge as-is; I think there are probably a few other hover situations we ought to be testing (mostly options, but perhaps there are others), but don't want to churn much more after approval. |
This takes an initial stab at adding some tests to the LSP implementation, starting with the hover feature. (I figure we can share the server across the other features.) We've been bitten a couple times by breaking changes that would be great to catch with tests.