refactor(tests): Replace MockLanguageServer singletons with configurable, per-testcase LanguageServer instances#1546
Conversation
| public synchronized void didChange(DidChangeTextDocumentParams params) { | ||
| super.didChange(params); | ||
| try { | ||
| Thread.sleep(1000); |
| final int currentCount = idx + 1; | ||
| return CompletableFuture.completedFuture(hoverResponse).thenApplyAsync(t -> { | ||
| try { | ||
| Thread.sleep(currentCount * 1000); |
| final int currentCount = idx + 1; | ||
| CompletableFuture<Hover> result = CompletableFuture.completedFuture(hoverResponse).thenApplyAsync(t -> { | ||
| try { | ||
| Thread.sleep(currentCount * 1000); |
bd21a96 to
d14ef6c
Compare
|
My original motivation for these changes is that the Mock LS does not properly close its outputstream on server exit. @rubenporras Can you please look at a few modified tests and share your opinion on the proposed changes? Edit: Maybe @sebthom or @mickaelistria also have an opinion on these changes? |
…ble, per-testcase LanguageServer instances
d14ef6c to
51b1893
Compare
|
I don't have time right now to do a thorough code review, but I strongly support the direction of this refactoring. I had considered doing something similar myself at one point, so I'm glad to see this happening. |
|
Hi @FlorianKroiss , I do not have the time to properly look at the PR, but if it addresses the AsynchronousCloseException/Broken Pipe/ClosedByInterruptExceptions then I am very happy if you can finish this. |
The tests which require interaction with a Language Server use a single MockLanguageServer instance per test, which has a few draw-backs:
This PR introduces a
MockLanguageServerExtensionwhich enables individual test cases to configure instances of theMockLanguageServerwhich are tailored towards the needs of the test case.The
MockLanguageServerFactorycan be used to do so and is automatically injected into a test parameter of the same type. A dummy example test case might look like this:Note that it's not required to call any method on
MockLanguageServerFactory, in which case a default configuration will be used. It's not even necessary to add theMockLanguageServerFactory factoryparameter.Most of the code changes in this PR are straight forward, i.e., replacing calls to
MockLanguageServer.INSTANCEwith the appropriate new mechanisms fromMockLanguageServerFactory.A few more cleanups that are also contained in this PR:
factory.withCapabilities(MockLanguageServer::multiRootCapabilities)ConnectDocumentToLanguageServerSetupParticipant.waitForAllwas changed to accommodate for the fact that the CompletableFutures are only added to the list after 1 second.AbstractTestandAllCleanExtensionhave been removed, as their functionality is superseded byMockLanguageServerExtension