eclipse-langium / langium

Next-gen language engineering / DSL framework
https://langium.org/
MIT License
665 stars 61 forks source link

`DefaultConfigurationProvider.getConfiguration` stalls in tests #1397

Closed lars-reimann closed 4 months ago

lars-reimann commented 4 months ago

After upgrading from langium 3.0.0-next.e78aeba to langium 3.0.0, many of the tests in Safe-DS time out. In particular, it's all tests that validate documents. This happens because some of our validation rules can be turned off using VS Code settings, so they contain code like

if (!(await settingsProvider.shouldValidateNameConvention())) {
    return;
}

The settingsProvider is just a thin wrapper around the DefaultConfigurationProvider and calls its getConfiguration method.

➡️ #1388 is likely related to this.

Langium version: 3.0.0 Package name: langium

Steps To Reproduce

Substitute SafeDs with some other language in the following code and run the tests with vitest:

import { describe, it } from 'vitest';
import { createSafeDsServices, SafeDsLanguageMetaData } from '../../../src/language/index.js';
import { NodeFileSystem } from 'langium/node';

const services = createSafeDsServices(NodeFileSystem).SafeDs;

describe('time out', () => {
    it('should not time out', async () => {
        await services.shared.workspace.ConfigurationProvider.getConfiguration(
            SafeDsLanguageMetaData.languageId,
            'validation',
        );
    });
});

Output:

Error: Test timed out in 5000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".
    at Timeout.<anonymous> (file:///E:/Repositories/safe-ds/DSL/node_modules/@vitest/runner/dist/index.js:63:16)
    at listOnTimeout (node:internal/timers:573:17)
    at processTimers (node:internal/timers:514:7)

Link to code example:

The current behavior

Tests time out because the service never readies up.

The expected behavior

Tests should pass.

msujew commented 4 months ago

This is working as expected, see here for the intended solution:

https://github.com/eclipse-langium/langium/blob/18a38cedb427d36bb955dd854548e9173955b2f8/packages/langium/test/workspace/configuration.test.ts#L17

We cannot really identify in the ConfigurationProvider whether we are running inside of a language server (which will eventually make the provider ready) or not. You need to call initialized explicitly to make the provider ready.

lars-reimann commented 4 months ago

Makes sense, thanks. I'm going to add the initialization of the configuration provider to our createSafeDsServices function and control this via an optional parameter.

Would it make sense to alter the Yeoman template to include such a parameter? Something like

export function create<%= LanguageName %>Services(context: DefaultSharedModuleContext, options?: ModuleOptions): {
    shared: LangiumSharedServices,
    <%= LanguageName %>: <%= LanguageName %>Services
} {
    const shared = inject(
        createDefaultSharedModule(context),
        <%= LanguageName %>GeneratedSharedModule
    );
    const <%= LanguageName %> = inject(
        createDefaultModule({ shared }),
        <%= LanguageName %>GeneratedModule,
        <%= LanguageName %>Module
    );
    shared.ServiceRegistry.register(<%= LanguageName %>);
    registerValidationChecks(<%= LanguageName %>);
    if (options?.initializeWithEmptyConfiguration) {
        shared.workspace.ConfigurationProvider.initialized({});
    }
    return { shared, <%= LanguageName %> };
}

interface ModuleOptions {
    initializeWithEmptyConfiguration?: boolean
}

Or is this something that should be handled purely by adopters?

msujew commented 4 months ago

We should be able to do something similar to this, yes:

if (!context.connection) {
  // We don't run inside of a language server
  // Therefore, initialize the configuration provider instantly
  shared.workspace.ConfigurationProvider.initialized({});
}

@lars-reimann Do you want to contribute this?

lars-reimann commented 4 months ago

Sure, I'll get to it right away.