Refactor to improve tests #37

remcohaszing · 2022-03-14T16:07:59Z

Initial checklist

I read the support docs
I read the contributing guide
I agree to follow the code of conduct
I searched issues and couldn’t find anything (or linked relevant results below)
If applicable, I’ve added docs and tests

Description of changes

This check was previously deemed unreachable, but it can be reached.

This is based on #35.

This way of testing doesn’t require to use any delays or timeouts. This also works by sending a response, awaiting a reply, then asserting the reply, instead of sending a number of requests only to assert all responses together later. Also any logging messages sent from the connection.console are logged to the console in tests. All of these changes together make troubleshooting any tests much nicer to deal with.

One more test is remaining. It got stuck after rewriting.

This check was previously deemed unreachable, but it can be reached.

wooorm · 2022-03-14T20:29:06Z

What's up with CI?

wooorm

Couple Qs! Looks pretty good. What do you make of the CI failure?

lib/index.js

test/index.js

remcohaszing · 2022-03-15T15:21:14Z

I think I’ve seen this CI failure locally once across running the tests dozens of times locally. It must be a rare race condition. I’ll investigate.

This is essentially a type-safe variant of vscode-jsonrpc

remcohaszing · 2022-03-23T21:23:07Z

package.json

@@ -82,6 +82,7 @@
  "typeCoverage": {
    "atLeast": 100,
    "detail": true,
+    "ignoreNested": true,


I added this, because InitializeRequest.type is of type ProtocolRequestType<_InitializeParams & WorkspaceFoldersInitializeParams & WorkDoneProgressParams, InitializeResult<any>, never, InitializeError, void>.

In other words: An upstream type includes an any type, which we don’t even use.

Oh interesting. I didn’t know about this field. It sounds like it might be useful in other projects too

wooorm · 2022-03-24T10:01:10Z

test/index.js

-    })
+  const connection = startLanguageServer(t, 'remark.js', '.')
+  const initializeResponse = await connection.sendRequest(
+    InitializeRequest.type,


I’m not a fan of these *.types (e.g., InitializeRequest.types). It requires knowing this VS Code project that doesn’t have docs. I am not a fan of depending on projects that are not documented (and whose code is unreadable).

Can we somehow pass, say, 'initialize', to, well, send an initialize event?

Rest looks good!

The use of *.type is what allows to infer the request and response types. It’s not a string, but an object which is typed in a way that allows TypeScript to infer types. Using a string removes type safety, meaning we need to manually add type annotations instead.

Personally I’m not a fan of how this has been implemented by vscode-languageserver-protocol either, but I do think this is the way to go. This protects us from using wrong request and response types, and it closely resembles how a real client would implement it.

As for the project being undocumented: A basic usage example is missing from their readme, but once you get that figured out, the rest is both pretty self-explanatory and well documented through jsdoc.

wooorm · 2022-03-24T10:50:40Z

Anything particar to check otherwise? Stuff you're 🤔🤷‍♂️ about?

remcohaszing · 2022-03-24T11:02:46Z

I’m not fond of the last commit (4e7e25d), but it’s an upstream issue.

It might be note-worthy to state I reran npm run test-api in a for loop 100 times several times to troubleshoot that, and there are no flaky tests anymore.

wooorm · 2022-03-24T14:36:02Z

thanks!

wooorm · 2022-03-30T13:18:13Z

https://github.com/unifiedjs/unified-language-server/blame/8efe2bc5bb9a2867e9033b269c7351dce79f37d5/test/index.js#L753 seems to indicate that console.logs should work, but when I add one to the server the tests crash?

On L1 in lib/index.js:

console.log('xxx:')

Yields:

/Users/tilde/Projects/oss/unified-language-server/node_modules/vscode-jsonrpc/lib/common/messageReader.js:138
                    throw new Error('Header must provide a Content-Length property.');
                    ^

Error: Header must provide a Content-Length property.
    at StreamMessageReader.onData (/Users/tilde/Projects/oss/unified-language-server/node_modules/vscode-jsonrpc/lib/common/messageReader.js:138:27)
    at Socket.<anonymous> (/Users/tilde/Projects/oss/unified-language-server/node_modules/vscode-jsonrpc/lib/common/messageReader.js:122:18)
    at Socket.emit (node:events:520:28)

remcohaszing · 2022-03-30T13:58:41Z

You need to write connection.console.log. I do think hijacking the console to support this would be a nice feature too.

wooorm · 2022-03-30T14:03:47Z

Ohhh right pff.
Do you mean hijacking in the tests, to make our life easier, or to hijack in the server, so that debugging in plugins would be sent across the protocol as well?

remcohaszing · 2022-03-30T14:25:08Z

I mean for the server to hijack the global console in the server process, so any console output is also sent to the language client. I don’t think it makes sense for the language server to crash, just because some third party package calls console.log

wooorm · 2022-03-30T14:39:03Z

it’s theoretically a bit weird how to do it if multiple servers are started, although I don’t see that practically happening?

I was under the impression this error only occurred in the tests but now I’m not sure, as you say, I guess it occurs for anything that writes to stdout? Alternatively then we could point console.* to console.error so that it writes to stderr? 🤷‍♂️

remcohaszing · 2022-03-30T14:52:34Z

I was under the impression this error only occurred in the tests but now I’m not sure, as you say, I guess it occurs for anything that writes to stdout?

Only if the server is started using the stdio protocol. So for vim-lsp, nvim-lspconfig, and emacs lsp-mode: yes, for vscode-remark: no.

I created #41 for further discussion.

remcohaszing added 7 commits March 7, 2022 21:29

Rewrite more tests

0ddd434

One more test is remaining. It got stuck after rewriting.

Rewrite remaining test

1d843c9

Fix type coverage

e289ed0

Fix tests for Node 12

21f9d27

Fix resolving paths in Windows

ec97739

Add test for formatting unsynchronized documents

91acc86

This check was previously deemed unreachable, but it can be reached.

remcohaszing requested review from wooorm, ChristianMurphy and Murderlon March 14, 2022 16:08

github-actions bot added the 👋 phase/new Post is being triaged automatically label Mar 14, 2022

This comment has been minimized.

Sign in to view

github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Mar 14, 2022

ChristianMurphy approved these changes Mar 14, 2022

View reviewed changes

ChristianMurphy added the 🕸️ area/tests This affects tests label Mar 14, 2022

wooorm reviewed Mar 15, 2022

View reviewed changes

lib/index.js Show resolved Hide resolved

test/index.js Outdated Show resolved Hide resolved

test/index.js Outdated Show resolved Hide resolved

test/index.js Outdated Show resolved Hide resolved

test/index.js Outdated Show resolved Hide resolved

wooorm mentioned this pull request Mar 15, 2022

Rewrite tests using vscode-jsonrpc #35

Closed

5 tasks

remcohaszing added 3 commits March 23, 2022 21:23

Use vscode-languageserver-protocol for testing

440d511

This is essentially a type-safe variant of vscode-jsonrpc

Explain a document might be unsynchronized

3dc9ae1

Add workaround for timing issue

4e7e25d

This comment was marked as resolved.

Sign in to view

remcohaszing commented Mar 23, 2022

View reviewed changes

wooorm reviewed Mar 24, 2022

View reviewed changes

wooorm approved these changes Mar 24, 2022

View reviewed changes

wooorm changed the title ~~Add test for formatting unsynchronized documents~~ Refactor to improve tests Mar 24, 2022

wooorm merged commit 2f5c4af into unifiedjs:main Mar 24, 2022

wooorm added the 💪 phase/solved Post is done label Mar 24, 2022

This comment has been minimized.

Sign in to view

github-actions bot removed the 🤞 phase/open Post is being triaged manually label Mar 24, 2022

remcohaszing deleted the add-missing-test branch August 6, 2022 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to improve tests #37

Refactor to improve tests #37

remcohaszing commented Mar 14, 2022 •

edited

Loading

This comment has been minimized.

wooorm commented Mar 14, 2022

wooorm left a comment

remcohaszing commented Mar 15, 2022

This comment was marked as resolved.

remcohaszing Mar 23, 2022

wooorm Mar 24, 2022

wooorm Mar 24, 2022

remcohaszing Mar 24, 2022

wooorm commented Mar 24, 2022

remcohaszing commented Mar 24, 2022

wooorm commented Mar 24, 2022

This comment has been minimized.

wooorm commented Mar 30, 2022

remcohaszing commented Mar 30, 2022

wooorm commented Mar 30, 2022

remcohaszing commented Mar 30, 2022

wooorm commented Mar 30, 2022

remcohaszing commented Mar 30, 2022

Refactor to improve tests #37

Refactor to improve tests #37

Conversation

remcohaszing commented Mar 14, 2022 • edited Loading

Initial checklist

Description of changes

This comment has been minimized.

wooorm commented Mar 14, 2022

wooorm left a comment

Choose a reason for hiding this comment

remcohaszing commented Mar 15, 2022

This comment was marked as resolved.

remcohaszing Mar 23, 2022

Choose a reason for hiding this comment

wooorm Mar 24, 2022

Choose a reason for hiding this comment

wooorm Mar 24, 2022

Choose a reason for hiding this comment

remcohaszing Mar 24, 2022

Choose a reason for hiding this comment

wooorm commented Mar 24, 2022

remcohaszing commented Mar 24, 2022

wooorm commented Mar 24, 2022

This comment has been minimized.

wooorm commented Mar 30, 2022

remcohaszing commented Mar 30, 2022

wooorm commented Mar 30, 2022

remcohaszing commented Mar 30, 2022

wooorm commented Mar 30, 2022

remcohaszing commented Mar 30, 2022

remcohaszing commented Mar 14, 2022 •

edited

Loading