From 7539e9a2285e5b0bb83c7641b642c35d12dab656 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Mon, 20 Jul 2020 17:41:08 -0700 Subject: [PATCH 1/5] Ensure languageServer value is valid, send event during activate --- src/client/activation/activationService.ts | 3 ++- src/client/common/configSettings.ts | 8 ++++---- .../configSettings/configSettings.unit.test.ts | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/client/activation/activationService.ts b/src/client/activation/activationService.ts index f825a5235dc5..7ceca31a7a66 100644 --- a/src/client/activation/activationService.ts +++ b/src/client/activation/activationService.ts @@ -188,7 +188,6 @@ export class LanguageServerExtensionActivationService // Configuration is non-default, so `languageServer` should be present. const configurationService = this.serviceContainer.get(IConfigurationService); const lstType = configurationService.getSettings(this.resource).languageServer; - this.sendTelemetryForChosenLanguageServer(lstType).ignoreErrors(); return lstType === LanguageServerType.Jedi; } @@ -253,6 +252,8 @@ export class LanguageServerExtensionActivationService break; } + this.sendTelemetryForChosenLanguageServer(serverType).ignoreErrors(); + await this.logStartup(serverType); let server = this.serviceContainer.get(ILanguageServerActivator, serverType); try { diff --git a/src/client/common/configSettings.ts b/src/client/common/configSettings.ts index 57c865fccfa2..b52bd496a520 100644 --- a/src/client/common/configSettings.ts +++ b/src/client/common/configSettings.ts @@ -231,11 +231,11 @@ export class PythonSettings implements IPythonSettings { pythonSettings.get('autoUpdateLanguageServer', true) )!; - let ls = pythonSettings.get('languageServer'); - if (!ls) { - ls = LanguageServerType.Jedi; + const ls = pythonSettings.get('languageServer') ?? LanguageServerType.Jedi; + this.languageServer = systemVariables.resolveAny(ls); + if (!(this.languageServer in LanguageServerType)) { + this.languageServer = LanguageServerType.Jedi; } - this.languageServer = systemVariables.resolveAny(ls)!; // tslint:disable-next-line:no-backbone-get-set-outside-model no-non-null-assertion this.jediPath = systemVariables.resolveAny(pythonSettings.get('jediPath'))!; diff --git a/src/test/common/configSettings/configSettings.unit.test.ts b/src/test/common/configSettings/configSettings.unit.test.ts index 757824b8a833..9c5f728f4d05 100644 --- a/src/test/common/configSettings/configSettings.unit.test.ts +++ b/src/test/common/configSettings/configSettings.unit.test.ts @@ -193,6 +193,20 @@ suite('Python Settings', async () => { config.verifyAll(); }); + test('Invalid languageServer type is fixed', () => { + expected.languageServer = 'invalid' as any; + initializeConfig(expected); + config + .setup((c) => c.get('languageServer')) + .returns(() => expected.languageServer) + .verifiable(TypeMoq.Times.once()); + + settings.update(config.object); + + expect(settings.languageServer).to.be.equal(LanguageServerType.Jedi); + config.verifyAll(); + }); + function testExperiments(enabled: boolean) { expected.pythonPath = 'python3'; // tslint:disable-next-line:no-any From 86a92405ce7abf430aaabb792ec30eb6eaeacf5c Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Mon, 20 Jul 2020 17:44:30 -0700 Subject: [PATCH 2/5] Tweak code to only assign this.languageServer once --- src/client/common/configSettings.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/client/common/configSettings.ts b/src/client/common/configSettings.ts index b52bd496a520..0fae464cbc93 100644 --- a/src/client/common/configSettings.ts +++ b/src/client/common/configSettings.ts @@ -231,11 +231,12 @@ export class PythonSettings implements IPythonSettings { pythonSettings.get('autoUpdateLanguageServer', true) )!; - const ls = pythonSettings.get('languageServer') ?? LanguageServerType.Jedi; - this.languageServer = systemVariables.resolveAny(ls); - if (!(this.languageServer in LanguageServerType)) { - this.languageServer = LanguageServerType.Jedi; + let ls = pythonSettings.get('languageServer') ?? LanguageServerType.Jedi; + ls = systemVariables.resolveAny(ls); + if (!(ls in LanguageServerType)) { + ls = LanguageServerType.Jedi; } + this.languageServer = ls; // tslint:disable-next-line:no-backbone-get-set-outside-model no-non-null-assertion this.jediPath = systemVariables.resolveAny(pythonSettings.get('jediPath'))!; From 02012d4faaca43f611a7f74164607f15d67b9cc1 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Mon, 20 Jul 2020 18:32:42 -0700 Subject: [PATCH 3/5] Fix enum membership check --- src/client/common/configSettings.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/common/configSettings.ts b/src/client/common/configSettings.ts index 0fae464cbc93..d0066f58b378 100644 --- a/src/client/common/configSettings.ts +++ b/src/client/common/configSettings.ts @@ -233,7 +233,7 @@ export class PythonSettings implements IPythonSettings { let ls = pythonSettings.get('languageServer') ?? LanguageServerType.Jedi; ls = systemVariables.resolveAny(ls); - if (!(ls in LanguageServerType)) { + if (!Object.values(LanguageServerType).includes(ls)) { ls = LanguageServerType.Jedi; } this.languageServer = ls; From 20958c92e04a06ea86d80d158d76c5d154b2d6ab Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Mon, 20 Jul 2020 18:41:48 -0700 Subject: [PATCH 4/5] Fix tests --- .../configSettings.unit.test.ts | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/test/common/configSettings/configSettings.unit.test.ts b/src/test/common/configSettings/configSettings.unit.test.ts index 9c5f728f4d05..9ad2a4990ced 100644 --- a/src/test/common/configSettings/configSettings.unit.test.ts +++ b/src/test/common/configSettings/configSettings.unit.test.ts @@ -193,18 +193,29 @@ suite('Python Settings', async () => { config.verifyAll(); }); - test('Invalid languageServer type is fixed', () => { - expected.languageServer = 'invalid' as any; - initializeConfig(expected); - config - .setup((c) => c.get('languageServer')) - .returns(() => expected.languageServer) - .verifiable(TypeMoq.Times.once()); + function testLanguageServer(languageServer: LanguageServerType, expectedValue: LanguageServerType) { + test(`languageServer=${languageServer}`, () => { + expected.pythonPath = 'python3'; + expected.languageServer = languageServer; + initializeConfig(expected); + config + .setup((c) => c.get('languageServer')) + .returns(() => expected.languageServer) + .verifiable(TypeMoq.Times.once()); - settings.update(config.object); + settings.update(config.object); - expect(settings.languageServer).to.be.equal(LanguageServerType.Jedi); - config.verifyAll(); + expect(settings.languageServer).to.be.equal(expectedValue); + config.verifyAll(); + }); + } + + suite('languageServer settings', async () => { + Object.values(LanguageServerType).forEach(async (languageServer) => { + testLanguageServer(languageServer, languageServer); + }); + + testLanguageServer('invalid' as LanguageServerType, LanguageServerType.Jedi); }); function testExperiments(enabled: boolean) { From ee47e39d5b7e692a1924f70e7d92ef37c59e7436 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Mon, 20 Jul 2020 18:43:09 -0700 Subject: [PATCH 5/5] Just use enum value as test name --- src/test/common/configSettings/configSettings.unit.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/common/configSettings/configSettings.unit.test.ts b/src/test/common/configSettings/configSettings.unit.test.ts index 9ad2a4990ced..6a6cbc07cf91 100644 --- a/src/test/common/configSettings/configSettings.unit.test.ts +++ b/src/test/common/configSettings/configSettings.unit.test.ts @@ -194,7 +194,7 @@ suite('Python Settings', async () => { }); function testLanguageServer(languageServer: LanguageServerType, expectedValue: LanguageServerType) { - test(`languageServer=${languageServer}`, () => { + test(languageServer, () => { expected.pythonPath = 'python3'; expected.languageServer = languageServer; initializeConfig(expected);