Replace mocks/stubs in tests with LSP #21
Conversation
Previously, the tests were tied to the functions that our Node language server exposes. This changes the tests to operate as an LSP Client. While clients are a bit low level, it means that the tests are no longer coupled to the values that `vscode-languageserver/node` expects or mocking and stubbing. This also also fix coverage, which wasn’t reported due to typo where `c8 ignore end` was used, which doesn’t do anything, instead of `c8 ignore stop`. It also adds stack traces when plugins crash, without them it’s hard to figure out what happened where and why.
github-actions
bot
added
👋 phase/new
| : end | ||
|
|
||
| if (start) { | ||
| return Range.create(start, end || start) |
There was a problem hiding this comment.
This doesn’t catch the situation if end is a valid unist point, but start isn’t. I recall running into this in a real rule while testing manually, but I don’t remember which one.
There was a problem hiding this comment.
It should work: see L72’s else case on L74!
(it’s a rather weird case you ran into btw, I don’t remember seeing something like that. Still: I’m all for very “defensive” / “safe” code for this 👍)
| : end | ||
|
|
||
| if (start) { | ||
| return Range.create(start, end || start) |
There was a problem hiding this comment.
This doesn’t catch the situation if end is a valid unist point, but start isn’t. I recall running into this in a real rule while testing manually, but I don’t remember which one.
| @@ -175,7 +169,7 @@ export function configureUnifiedLanguageServer( | |||
| streamError: new PassThrough(), | |||
| streamOut: new PassThrough() | |||
| }, | |||
| (error, code, context) => { | |||
| (error, _, context) => { | |||
There was a problem hiding this comment.
I prefer using a proper name for variables, even if they are unused.
There was a problem hiding this comment.
Main reason for me is that TS in my editor is info’ing about it being unused otherwise.
I’d be 🤷♂️ otherwise
Co-authored-by: Remco Haszing <[email protected]>
This comment has been minimized.
This comment has been minimized.
|
Merging as it looks like remaining conversations seem optional to me and have settled? |
This comment has been minimized.
This comment has been minimized.
Related-to GH-21. Closes GH-36. Reviewed-by: Merlijn Vos <[email protected]> Reviewed-by: Titus Wormer <[email protected]>
Initial checklist
Description of changes
Previously, the tests were tied to the functions that our Node language server exposes. This changes the tests to operate as an LSP Client. While clients are a bit low level, it means that the tests are no longer coupled to the values that
vscode-languageserver/nodeexpects or mocking and stubbing.This change will make GH-20 and similar features easier. Due to the previous mocking the expected argument
eventtoonInitializewasn’t given, but now it is.This also also fix coverage, which wasn’t reported due to typo where
c8 ignore endwas used, which doesn’t do anything, instead ofc8 ignore stop.It also adds stack traces when plugins crash, without them it’s hard to figure out what happened where and why.
/cc @remcohaszing