Skip to content

refactor(tests): Replace MockLanguageServer singletons with configurable, per-testcase LanguageServer instances#1546

Open
FlorianKroiss wants to merge 1 commit into
eclipse-lsp4e:mainfrom
FlorianKroiss:mock-server-extension
Open

refactor(tests): Replace MockLanguageServer singletons with configurable, per-testcase LanguageServer instances#1546
FlorianKroiss wants to merge 1 commit into
eclipse-lsp4e:mainfrom
FlorianKroiss:mock-server-extension

Conversation

@FlorianKroiss

@FlorianKroiss FlorianKroiss commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

The tests which require interaction with a Language Server use a single MockLanguageServer instance per test, which has a few draw-backs:

  1. The MockLanguageServer does not behave like a proper LS. Namely, it's exit() did nothing, where a "real" LS would have to close the outbound connection to the client. However, since the MockLanguageServer serves as the endpoint for many connections, it could not know which one called exit()
  2. Tests which start multiple servers cannot track the state of the individually launched servers, because they all are connected to the same instance.
  3. Tests had to make sure to extend AbstractTest or use AllCleanExtension so that a new MockLanguageServer is instantiated and no State is leaked between tests.

This PR introduces a MockLanguageServerExtension which enables individual test cases to configure instances of the MockLanguageServer which are tailored towards the needs of the test case.
The MockLanguageServerFactory can 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:

	@Test
	public void myTest(MockLanguageServerFactory factory) throws Exception {
		// Executed for each created server.
		factory.withConfiguration((idx, server) -> {
			if (idx == 0) {
				// Server 0 delivers one entry
				server.setCodeLens(List.of(new CodeLens()));
			} else {
				// Server 1 delivers two entries
				server.setCodeLens(List.of(new CodeLens(), new CodeLens()));
			}
		});
		// Override default capabilities for created servers
		factory.withCapabilities(() -> {
			var cap = MockLanguageServer.defaultServerCapabilities();
			cap.setCodeActionProvider(Boolean.TRUE);
			return cap;
		});
		
		// Open file which will launch a server
		IFile file = TestUtils.createUniqueTestFile(project, "someFile");
		ITextViewer viewer = TestUtils.openTextViewer(file);
		// [... do stuff with viewer ...]
		
		// Change configuration of started server
		factory.getServer().setCodeLens(null);

		// Assert stuff on started servers
		assertTrue(factory.getServerCount() == 2);
		assertTrue(factory.getServers().get(1).getState() == MockServerState.RUNNING);
	}

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 the MockLanguageServerFactory factory parameter.

Most of the code changes in this PR are straight forward, i.e., replacing calls to MockLanguageServer.INSTANCE with the appropriate new mechanisms from MockLanguageServerFactory.

A few more cleanups that are also contained in this PR:

  • MockLanguageServerMultiRootFolders/MockConnectionProviderMultiRootFolders and associated entries in the fragment.xml have been removed. These were outdated copies of the MockLanguageServer, with the only difference, that they advertised multi-root support. Tests which require a LS with multi-root support, can now just set the respective capabilities with factory.withCapabilities(MockLanguageServer::multiRootCapabilities)
  • AbstractTest and AllCleanExtension have been removed, as their functionality is superseded by MockLanguageServerExtension

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);
@FlorianKroiss FlorianKroiss force-pushed the mock-server-extension branch from bd21a96 to d14ef6c Compare June 14, 2026 21:02
@FlorianKroiss

FlorianKroiss commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

My original motivation for these changes is that the Mock LS does not properly close its outputstream on server exit.
Fixing this is one of the requirements to fixing all of the numerous AsynchronousCloseException/Broken Pipe/ClosedByInterruptExceptions in our test execution.

@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?

@FlorianKroiss FlorianKroiss force-pushed the mock-server-extension branch from d14ef6c to 51b1893 Compare June 24, 2026 17:46
@sebthom

sebthom commented Jun 24, 2026

Copy link
Copy Markdown
Member

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.

@rubenporras

Copy link
Copy Markdown
Contributor

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.

@FlorianKroiss FlorianKroiss force-pushed the mock-server-extension branch from 51b1893 to ed89510 Compare June 29, 2026 18:55
@FlorianKroiss FlorianKroiss marked this pull request as ready for review June 29, 2026 19:49
@FlorianKroiss

FlorianKroiss commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

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.

It's at least a step in that direction.
From my side, I think this is good to go, as we can still fine-tune it later on.
I think the bot remarks can be dismissed, as the relevant code was only moved.

@FlorianKroiss FlorianKroiss requested a review from rubenporras July 2, 2026 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants