Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DefaultConfigurationProvider.getConfiguration stalls in tests #1397

Closed
lars-reimann opened this issue Mar 1, 2024 · 4 comments · Fixed by #1402
Closed

DefaultConfigurationProvider.getConfiguration stalls in tests #1397

lars-reimann opened this issue Mar 1, 2024 · 4 comments · Fixed by #1402
Labels
bug Something isn't working
Milestone

Comments

@lars-reimann
Copy link
Member

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.

@lars-reimann lars-reimann added the bug Something isn't working label Mar 1, 2024
@msujew
Copy link
Member

msujew commented Mar 1, 2024

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

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
Copy link
Member Author

lars-reimann commented Mar 1, 2024

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
Copy link
Member

msujew commented Mar 1, 2024

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
Copy link
Member Author

Sure, I'll get to it right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants