From 04e0b3441647a894d73ba5bb36e45550b79de46f Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Tue, 5 May 2020 21:15:32 -0700 Subject: [PATCH 01/21] Fix path --- src/client/activation/node/languageClientFactory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/activation/node/languageClientFactory.ts b/src/client/activation/node/languageClientFactory.ts index 8490c4910041..d607afdd5d31 100644 --- a/src/client/activation/node/languageClientFactory.ts +++ b/src/client/activation/node/languageClientFactory.ts @@ -20,7 +20,7 @@ export class NodeLanguageClientFactory implements ILanguageClientFactory { constructor( @inject(IFileSystem) private readonly fs: IFileSystem, @inject(ILanguageServerFolderService) private readonly languageServerFolderService: ILanguageServerFolderService - ) {} + ) { } public async createLanguageClient( resource: Resource, From 4477900742b0dea66a9c1219a8e16b5254f2f69d Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 11:40:57 -0700 Subject: [PATCH 02/21] Actually fix settings --- src/client/testing/common/updateTestSettings.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/client/testing/common/updateTestSettings.ts b/src/client/testing/common/updateTestSettings.ts index 1f5c79faf32c..1a309a66c1b5 100644 --- a/src/client/testing/common/updateTestSettings.ts +++ b/src/client/testing/common/updateTestSettings.ts @@ -31,7 +31,7 @@ export class UpdateTestSettingService implements IExtensionActivationService { const filesToBeFixed = await this.getFilesToBeFixed(resource); await Promise.all(filesToBeFixed.map((file) => this.fixSettingInFile(file))); } - public getSettingsFiles(resource: Resource) { + public getSettingsFiles(resource: Resource): string[] { const settingsFiles: string[] = []; if (this.application.userSettingsFile) { settingsFiles.push(this.application.userSettingsFile); @@ -42,7 +42,7 @@ export class UpdateTestSettingService implements IExtensionActivationService { } return settingsFiles; } - public async getFilesToBeFixed(resource: Resource) { + public async getFilesToBeFixed(resource: Resource): Promise { const files = this.getSettingsFiles(resource); const result = await Promise.all( files.map(async (file) => { @@ -87,10 +87,11 @@ export class UpdateTestSettingService implements IExtensionActivationService { return fileContents; } - public async doesFileNeedToBeFixed(filePath: string) { + public async doesFileNeedToBeFixed(filePath: string): Promise { try { const contents = await this.fs.readFile(filePath); return ( + contents.indexOf('python.jediEnabled') > 0 || contents.indexOf('python.unitTest.') > 0 || contents.indexOf('.pyTest') > 0 || contents.indexOf('.pep8') > 0 From d0d50dedd938bb5380c1413c25b6150dc420a938 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 11:45:41 -0700 Subject: [PATCH 03/21] Add news --- news/2 Fixes/12429.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/12429.md diff --git a/news/2 Fixes/12429.md b/news/2 Fixes/12429.md new file mode 100644 index 000000000000..4fea2a4f8f5c --- /dev/null +++ b/news/2 Fixes/12429.md @@ -0,0 +1 @@ +Fixed issue when `python.jediEnabled` setting was not removed and `python.languageServer` setting was not updated. From 03aa5f92820ac00a443d1b0cec174f8b0ad5e98c Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 11:53:02 -0700 Subject: [PATCH 04/21] Add test --- .../diagnostics/checks/updateTestSettings.unit.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts b/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts index 819b27b9f8cb..c9a5bd704ee0 100644 --- a/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts +++ b/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts @@ -168,6 +168,12 @@ suite('Application Diagnostics - Check Test Settings', () => { assert.ok(!needsToBeFixed); verify(fs.readFile(__filename)).once(); }); + test('Verify `python.jediEnabled` is found in user settings', async () => { + when(fs.readFile(__filename)).thenResolve('"python.jediEnabled": false'); + const needsToBeFixed = await diagnosticService.doesFileNeedToBeFixed(__filename); + assert.ok(needsToBeFixed); + verify(fs.readFile(__filename)).once(); + }); [ { From e4a032f43211c9082e5ae4f0db96deee6160471f Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 11:54:54 -0700 Subject: [PATCH 05/21] Format --- src/client/activation/node/languageClientFactory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/activation/node/languageClientFactory.ts b/src/client/activation/node/languageClientFactory.ts index 8bd72f7d60b0..a8d9cbdab41d 100644 --- a/src/client/activation/node/languageClientFactory.ts +++ b/src/client/activation/node/languageClientFactory.ts @@ -20,7 +20,7 @@ export class NodeLanguageClientFactory implements ILanguageClientFactory { constructor( @inject(IFileSystem) private readonly fs: IFileSystem, @inject(ILanguageServerFolderService) private readonly languageServerFolderService: ILanguageServerFolderService - ) { } + ) {} public async createLanguageClient( resource: Resource, From 27eeec76f8519ed0beb45274db2cd9200bde1f26 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 16:29:44 -0700 Subject: [PATCH 06/21] Suppress 'jediEnabled' removal --- src/client/testing/common/updateTestSettings.ts | 13 +++---------- .../checks/updateTestSettings.unit.test.ts | 12 ++++++------ 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/client/testing/common/updateTestSettings.ts b/src/client/testing/common/updateTestSettings.ts index 1a309a66c1b5..b7b60aad8ad8 100644 --- a/src/client/testing/common/updateTestSettings.ts +++ b/src/client/testing/common/updateTestSettings.ts @@ -107,13 +107,13 @@ export class UpdateTestSettingService implements IExtensionActivationService { // - `true` or missing then set to `languageServer: Jedi`. // - `false` and `languageServer` is present, do nothing. // - `false` and `languageServer` is NOT present, set `languageServer` to `Microsoft`. - // `jediEnabled` is then removed. + // `jediEnabled` is NOT removed since JSONC parser may also remove comments. const jediEnabledPath = ['python.jediEnabled']; const languageServerPath = ['python.languageServer']; try { - let ast = parseTree(fileContent); - let jediEnabledNode = findNodeAtLocation(ast, jediEnabledPath); + const ast = parseTree(fileContent); + const jediEnabledNode = findNodeAtLocation(ast, jediEnabledPath); const jediEnabled = jediEnabledNode ? getNodeValue(jediEnabledNode) : true; const languageServerNode = findNodeAtLocation(ast, languageServerPath); const formattingOptions: FormattingOptions = { @@ -135,13 +135,6 @@ export class UpdateTestSettingService implements IExtensionActivationService { } fileContent = applyEdits(fileContent, edits); - // Remove jediEnabled - ast = parseTree(fileContent); - jediEnabledNode = findNodeAtLocation(ast, jediEnabledPath); - if (jediEnabledNode) { - edits = modify(fileContent, jediEnabledPath, undefined, { formattingOptions }); - fileContent = applyEdits(fileContent, edits); - } // tslint:disable-next-line:no-empty } catch {} return fileContent; diff --git a/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts b/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts index c9a5bd704ee0..d70dd05728ef 100644 --- a/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts +++ b/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts @@ -224,32 +224,32 @@ suite('Application Diagnostics - Check Test Settings', () => { { testTitle: 'jediEnabled: true, no languageServer setting', contents: '{ "python.jediEnabled": true }', - expectedContent: '{"python.languageServer": "Jedi"}' + expectedContent: '{ "python.jediEnabled": true, "python.languageServer": "Jedi"}' }, { testTitle: 'jediEnabled: true, languageServer setting present', contents: '{ "python.jediEnabled": true }', - expectedContent: '{"python.languageServer": "Jedi"}' + expectedContent: '{ "python.jediEnabled": true, "python.languageServer": "Jedi"}' }, { testTitle: 'jediEnabled: false, no languageServer setting', contents: '{ "python.jediEnabled": false }', - expectedContent: '{"python.languageServer": "Microsoft"}' + expectedContent: '{ "python.jediEnabled": false, "python.languageServer": "Microsoft"}' }, { testTitle: 'jediEnabled: false, languageServer is Microsoft', contents: '{ "python.jediEnabled": false, "python.languageServer": "Microsoft" }', - expectedContent: '{"python.languageServer": "Microsoft"}' + expectedContent: '{ "python.jediEnabled": false, "python.languageServer": "Microsoft"}' }, { testTitle: 'jediEnabled: false, languageServer is None', contents: '{ "python.jediEnabled": false, "python.languageServer": "None" }', - expectedContent: '{"python.languageServer": "None"}' + expectedContent: '{ "python.jediEnabled": false, "python.languageServer": "None"}' }, { testTitle: 'jediEnabled: false, languageServer is Jedi', contents: '{ "python.jediEnabled": false, "python.languageServer": "Jedi" }', - expectedContent: '{"python.languageServer": "Jedi"}' + expectedContent: '{ "python.jediEnabled": false, "python.languageServer": "Jedi"}' } ].forEach((item) => { test(item.testTitle, async () => { From 3ed655b7f239d923598226cbc7ae9461f98ae266 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 26 Jun 2020 08:31:45 -0700 Subject: [PATCH 07/21] Drop survey first launch threshold --- src/client/languageServices/languageServerSurveyBanner.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/languageServices/languageServerSurveyBanner.ts b/src/client/languageServices/languageServerSurveyBanner.ts index 3d58991a6094..721b9d05d965 100644 --- a/src/client/languageServices/languageServerSurveyBanner.ts +++ b/src/client/languageServices/languageServerSurveyBanner.ts @@ -44,8 +44,8 @@ export class LanguageServerSurveyBanner implements IPythonExtensionBanner { @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory, @inject(IBrowserService) private browserService: IBrowserService, @inject(ILanguageServerFolderService) private lsService: ILanguageServerFolderService, - showAfterMinimumEventsCount: number = 100, - showBeforeMaximumEventsCount: number = 500 + showAfterMinimumEventsCount: number = 30, + showBeforeMaximumEventsCount: number = 200 ) { this.minCompletionsBeforeShow = showAfterMinimumEventsCount; this.maxCompletionsBeforeShow = showBeforeMaximumEventsCount; From bbe5a7f52ab83ba125d84f59c2ec9f24c65cb957 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Wed, 8 Jul 2020 12:52:14 -0700 Subject: [PATCH 08/21] Remove LS experiments --- src/client/activation/activationService.ts | 16 +- .../activation/activationService.unit.test.ts | 281 ++---------------- 2 files changed, 19 insertions(+), 278 deletions(-) diff --git a/src/client/activation/activationService.ts b/src/client/activation/activationService.ts index f825a5235dc5..bef0c7e0d79e 100644 --- a/src/client/activation/activationService.ts +++ b/src/client/activation/activationService.ts @@ -9,12 +9,10 @@ import { LSNotSupportedDiagnosticServiceId } from '../application/diagnostics/ch import { IDiagnosticsService } from '../application/diagnostics/types'; import { IApplicationShell, ICommandManager, IWorkspaceService } from '../common/application/types'; import { STANDARD_OUTPUT_CHANNEL } from '../common/constants'; -import { LSControl, LSEnabled } from '../common/experiments/groups'; import { traceError } from '../common/logger'; import { IConfigurationService, IDisposableRegistry, - IExperimentsManager, IOutputChannel, IPersistentStateFactory, IPythonSettings, @@ -58,8 +56,7 @@ export class LanguageServerExtensionActivationService constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer, - @inject(IPersistentStateFactory) private stateFactory: IPersistentStateFactory, - @inject(IExperimentsManager) private readonly abExperiments: IExperimentsManager + @inject(IPersistentStateFactory) private stateFactory: IPersistentStateFactory ) { this.workspaceService = this.serviceContainer.get(IWorkspaceService); this.interpreterService = this.serviceContainer.get(IInterpreterService); @@ -175,17 +172,6 @@ export class LanguageServerExtensionActivationService * @returns `true` if user is using jedi, `false` if user is using language server */ public useJedi(): boolean { - // Check if `languageServer` setting is missing (default configuration). - if (this.isJediUsingDefaultConfiguration(this.resource)) { - // If user is assigned to an experiment (i.e. use LS), return false. - if (this.abExperiments.inExperiment(LSEnabled)) { - return false; - } - // Send telemetry if user is in control group - this.abExperiments.sendTelemetryIfInExperiment(LSControl); - return true; // Do use Jedi as it is default. - } - // 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(); diff --git a/src/test/activation/activationService.unit.test.ts b/src/test/activation/activationService.unit.test.ts index ae0c34aa5a6d..fea8949e050e 100644 --- a/src/test/activation/activationService.unit.test.ts +++ b/src/test/activation/activationService.unit.test.ts @@ -16,13 +16,11 @@ import { import { LSNotSupportedDiagnosticServiceId } from '../../client/application/diagnostics/checks/lsNotSupported'; import { IDiagnostic, IDiagnosticsService } from '../../client/application/diagnostics/types'; import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../client/common/application/types'; -import { LSControl, LSEnabled } from '../../client/common/experiments/groups'; import { IPlatformService } from '../../client/common/platform/types'; import { IConfigurationService, IDisposable, IDisposableRegistry, - IExperimentsManager, IOutputChannel, IPersistentState, IPersistentStateFactory, @@ -53,7 +51,6 @@ suite('Language Server Activation - ActivationService', () => { let lsNotSupportedDiagnosticService: TypeMoq.IMock; let stateFactory: TypeMoq.IMock; let state: TypeMoq.IMock>; - let experiments: TypeMoq.IMock; let workspaceConfig: TypeMoq.IMock; let interpreterService: TypeMoq.IMock; let interpreterChangedHandler!: Function; @@ -67,7 +64,6 @@ suite('Language Server Activation - ActivationService', () => { state = TypeMoq.Mock.ofType>(); const configService = TypeMoq.Mock.ofType(); pythonSettings = TypeMoq.Mock.ofType(); - experiments = TypeMoq.Mock.ofType(); const langFolderServiceMock = TypeMoq.Mock.ofType(); const folderVer: FolderVersionPair = { path: '', @@ -179,16 +175,10 @@ suite('Language Server Activation - ActivationService', () => { .returns(() => activator.object) .verifiable(TypeMoq.Times.once()); - experiments - .setup((ex) => ex.inExperiment(TypeMoq.It.isAny())) - .returns(() => false) - .verifiable(TypeMoq.Times.never()); - await activationService.activate(undefined); activator.verifyAll(); serviceContainer.verifyAll(); - experiments.verifyAll(); } async function testReloadMessage(settingName: string): Promise { @@ -205,8 +195,7 @@ suite('Language Server Activation - ActivationService', () => { const activator = TypeMoq.Mock.ofType(); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); workspaceService.verifyAll(); @@ -244,8 +233,7 @@ suite('Language Server Activation - ActivationService', () => { const activator = TypeMoq.Mock.ofType(); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await testActivation(activationService, activator, true); @@ -255,8 +243,7 @@ suite('Language Server Activation - ActivationService', () => { const activator = TypeMoq.Mock.ofType(); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await testActivation(activationService, activator, false); @@ -267,8 +254,7 @@ suite('Language Server Activation - ActivationService', () => { const activator = TypeMoq.Mock.ofType(); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await testActivation(activationService, activator); @@ -278,8 +264,7 @@ suite('Language Server Activation - ActivationService', () => { const activator = TypeMoq.Mock.ofType(); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await testActivation(activationService, activator); @@ -294,8 +279,7 @@ suite('Language Server Activation - ActivationService', () => { const activator = TypeMoq.Mock.ofType(); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await testActivation(activationService, activator, false, LanguageServerType.None); }); @@ -316,8 +300,7 @@ suite('Language Server Activation - ActivationService', () => { const activator = TypeMoq.Mock.ofType(); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); workspaceService.verifyAll(); @@ -392,8 +375,7 @@ suite('Language Server Activation - ActivationService', () => { pythonSettings.setup((p) => p.languageServer).returns(() => LanguageServerType.Microsoft); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); const ls1 = await activationService.get(folder1.uri, interpreter1); const ls2 = await activationService.get(folder1.uri, interpreter2); @@ -463,8 +445,7 @@ suite('Language Server Activation - ActivationService', () => { pythonSettings.setup((p) => p.languageServer).returns(() => LanguageServerType.Microsoft); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await activationService.activate(folder1.uri); await interpreterChangedHandler(); @@ -486,8 +467,7 @@ suite('Language Server Activation - ActivationService', () => { const activatorJedi = TypeMoq.Mock.ofType(); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); const diagnostics: IDiagnostic[] = []; lsNotSupportedDiagnosticService @@ -562,17 +542,12 @@ suite('Language Server Activation - ActivationService', () => { .setup((w) => w.getWorkspaceFolderIdentifier(resource, '')) .returns(() => resource!.fsPath) .verifiable(TypeMoq.Times.atLeastOnce()); - experiments - .setup((ex) => ex.inExperiment(TypeMoq.It.isAny())) - .returns(() => false) - .verifiable(TypeMoq.Times.never()); await activationService.activate(resource); activator.verifyAll(); serviceContainer.verifyAll(); workspaceService.verifyAll(); - experiments.verifyAll(); } test('Activator is disposed if activated workspace is removed and LS is "Microsoft"', async () => { pythonSettings.setup((p) => p.languageServer).returns(() => LanguageServerType.Microsoft); @@ -584,8 +559,7 @@ suite('Language Server Activation - ActivationService', () => { .verifiable(TypeMoq.Times.once()); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); workspaceService.verifyAll(); expect(workspaceFoldersChangedHandler).not.to.be.equal(undefined, 'Handler not set'); @@ -624,8 +598,7 @@ suite('Language Server Activation - ActivationService', () => { const activator1 = TypeMoq.Mock.ofType(); const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); const folder1 = { name: 'one', uri: Uri.parse('one'), index: 1 }; const folder2 = { name: 'two', uri: Uri.parse('two'), index: 2 }; @@ -642,15 +615,10 @@ suite('Language Server Activation - ActivationService', () => { .setup((a) => a.start(folder1.uri, undefined)) .returns(() => Promise.resolve()) .verifiable(TypeMoq.Times.once()); - experiments - .setup((ex) => ex.inExperiment(TypeMoq.It.isAny())) - .returns(() => false) - .verifiable(TypeMoq.Times.never()); await activationService.activate(folder1.uri); activator1.verifyAll(); activator1.verify((a) => a.activate(), TypeMoq.Times.once()); serviceContainer.verifyAll(); - experiments.verifyAll(); const activator2 = TypeMoq.Mock.ofType(); serviceContainer @@ -667,16 +635,11 @@ suite('Language Server Activation - ActivationService', () => { .returns(() => Promise.resolve()) .verifiable(TypeMoq.Times.never()); activator2.setup((a) => a.activate()).verifiable(TypeMoq.Times.never()); - experiments - .setup((ex) => ex.inExperiment(TypeMoq.It.isAny())) - .returns(() => false) - .verifiable(TypeMoq.Times.never()); await activationService.activate(folder2.uri); serviceContainer.verifyAll(); activator1.verifyAll(); activator1.verify((a) => a.activate(), TypeMoq.Times.exactly(2)); activator2.verifyAll(); - experiments.verifyAll(); }); } } @@ -693,7 +656,6 @@ suite('Language Server Activation - ActivationService', () => { let lsNotSupportedDiagnosticService: TypeMoq.IMock; let stateFactory: TypeMoq.IMock; let state: TypeMoq.IMock>; - let experiments: TypeMoq.IMock; let workspaceConfig: TypeMoq.IMock; let interpreterService: TypeMoq.IMock; setup(() => { @@ -706,7 +668,6 @@ suite('Language Server Activation - ActivationService', () => { state = TypeMoq.Mock.ofType>(); const configService = TypeMoq.Mock.ofType(); pythonSettings = TypeMoq.Mock.ofType(); - experiments = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); const e = new EventEmitter(); interpreterService.setup((i) => i.onDidChangeInterpreter).returns(() => e.event); @@ -785,8 +746,7 @@ suite('Language Server Activation - ActivationService', () => { const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await activationService.sendTelemetryForChosenLanguageServer(LanguageServerType.Jedi); @@ -805,8 +765,7 @@ suite('Language Server Activation - ActivationService', () => { const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await activationService.sendTelemetryForChosenLanguageServer(LanguageServerType.Microsoft); @@ -825,8 +784,7 @@ suite('Language Server Activation - ActivationService', () => { const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await activationService.sendTelemetryForChosenLanguageServer(LanguageServerType.Jedi); @@ -845,8 +803,7 @@ suite('Language Server Activation - ActivationService', () => { const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); await activationService.sendTelemetryForChosenLanguageServer(LanguageServerType.Jedi); @@ -854,204 +811,6 @@ suite('Language Server Activation - ActivationService', () => { }); }); - suite('Test useJedi()', () => { - let serviceContainer: TypeMoq.IMock; - let pythonSettings: TypeMoq.IMock; - let appShell: TypeMoq.IMock; - let cmdManager: TypeMoq.IMock; - let workspaceService: TypeMoq.IMock; - let platformService: TypeMoq.IMock; - let lsNotSupportedDiagnosticService: TypeMoq.IMock; - let stateFactory: TypeMoq.IMock; - let state: TypeMoq.IMock>; - let experiments: TypeMoq.IMock; - let workspaceConfig: TypeMoq.IMock; - let interpreterService: TypeMoq.IMock; - setup(() => { - serviceContainer = TypeMoq.Mock.ofType(); - appShell = TypeMoq.Mock.ofType(); - workspaceService = TypeMoq.Mock.ofType(); - cmdManager = TypeMoq.Mock.ofType(); - platformService = TypeMoq.Mock.ofType(); - stateFactory = TypeMoq.Mock.ofType(); - state = TypeMoq.Mock.ofType>(); - const configService = TypeMoq.Mock.ofType(); - pythonSettings = TypeMoq.Mock.ofType(); - experiments = TypeMoq.Mock.ofType(); - interpreterService = TypeMoq.Mock.ofType(); - const e = new EventEmitter(); - interpreterService.setup((i) => i.onDidChangeInterpreter).returns(() => e.event); - const langFolderServiceMock = TypeMoq.Mock.ofType(); - const folderVer: FolderVersionPair = { - path: '', - version: new SemVer('1.2.3') - }; - lsNotSupportedDiagnosticService = TypeMoq.Mock.ofType(); - workspaceService.setup((w) => w.hasWorkspaceFolders).returns(() => false); - workspaceService.setup((w) => w.workspaceFolders).returns(() => []); - configService.setup((c) => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); - langFolderServiceMock - .setup((l) => l.getCurrentLanguageServerDirectory()) - .returns(() => Promise.resolve(folderVer)); - stateFactory - .setup((f) => - f.createGlobalPersistentState( - TypeMoq.It.isValue('SWITCH_LS'), - TypeMoq.It.isAny(), - TypeMoq.It.isAny() - ) - ) - .returns(() => state.object); - state.setup((s) => s.value).returns(() => undefined); - state.setup((s) => s.updateValue(TypeMoq.It.isAny())).returns(() => Promise.resolve()); - workspaceConfig = TypeMoq.Mock.ofType(); - workspaceService - .setup((ws) => ws.getConfiguration('python', TypeMoq.It.isAny())) - .returns(() => workspaceConfig.object); - const output = TypeMoq.Mock.ofType(); - serviceContainer - .setup((c) => c.get(TypeMoq.It.isValue(IOutputChannel), TypeMoq.It.isAny())) - .returns(() => output.object); - serviceContainer - .setup((c) => c.get(TypeMoq.It.isValue(IWorkspaceService))) - .returns(() => workspaceService.object); - serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IApplicationShell))).returns(() => appShell.object); - serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IDisposableRegistry))).returns(() => []); - serviceContainer - .setup((c) => c.get(TypeMoq.It.isValue(IConfigurationService))) - .returns(() => configService.object); - serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(ICommandManager))).returns(() => cmdManager.object); - serviceContainer - .setup((c) => c.get(TypeMoq.It.isValue(IPlatformService))) - .returns(() => platformService.object); - serviceContainer - .setup((c) => c.get(TypeMoq.It.isValue(IInterpreterService))) - .returns(() => interpreterService.object); - serviceContainer - .setup((c) => c.get(TypeMoq.It.isValue(ILanguageServerFolderService))) - .returns(() => langFolderServiceMock.object); - serviceContainer - .setup((s) => - s.get( - TypeMoq.It.isValue(IDiagnosticsService), - TypeMoq.It.isValue(LSNotSupportedDiagnosticServiceId) - ) - ) - .returns(() => lsNotSupportedDiagnosticService.object); - }); - - test('If default value (Jedi) of language server is being used, and LSEnabled experiment is enabled, then return false', async () => { - const settings = {}; - experiments - .setup((ex) => ex.inExperiment(LSEnabled)) - .returns(() => true) - .verifiable(TypeMoq.Times.once()); - experiments - .setup((ex) => ex.sendTelemetryIfInExperiment(TypeMoq.It.isAny())) - .returns(() => undefined) - .verifiable(TypeMoq.Times.never()); - workspaceConfig - .setup((c) => c.inspect('languageServer')) - .returns(() => settings as any) - .verifiable(TypeMoq.Times.once()); - - const activationService = new LanguageServerExtensionActivationService( - serviceContainer.object, - stateFactory.object, - experiments.object - ); - const result = activationService.useJedi(); - expect(result).to.equal(false, 'LS should be enabled'); - - workspaceService.verifyAll(); - workspaceConfig.verifyAll(); - experiments.verifyAll(); - }); - - test('If default value of languageServer (Jedi) is being used, and LSEnabled experiment is disabled, then send telemetry if user is in Experiment LSControl and useJedi() should be true)', async () => { - const settings = {}; - experiments - .setup((ex) => ex.inExperiment(LSEnabled)) - .returns(() => false) - .verifiable(TypeMoq.Times.once()); - experiments - .setup((ex) => ex.sendTelemetryIfInExperiment(LSControl)) - .returns(() => undefined) - .verifiable(TypeMoq.Times.once()); - workspaceConfig - .setup((c) => c.inspect('languageServer')) - .returns(() => settings as any) - .verifiable(TypeMoq.Times.once()); - - const activationService = new LanguageServerExtensionActivationService( - serviceContainer.object, - stateFactory.object, - experiments.object - ); - const result = activationService.useJedi(); - expect(result).to.equal(true, 'Return value should be true'); - - pythonSettings.verifyAll(); - experiments.verifyAll(); - workspaceService.verifyAll(); - workspaceConfig.verifyAll(); - }); - - suite( - 'If default value of languageServer (Jedi) is not being used, then no experiments are used, and python settings value is returned', - async () => { - [ - { - testName: 'Returns false when python settings value is Microsoft', - pythonSettingsValue: LanguageServerType.Microsoft, - expectedResult: false - }, - { - testName: 'Returns true when python settings value is Jedi', - pythonSettingsValue: LanguageServerType.Jedi, - expectedResult: true - } - ].forEach((testParams) => { - test(testParams.testName, async () => { - const settings = { workspaceFolderValue: true }; - experiments - .setup((ex) => ex.inExperiment(LSEnabled)) - .returns(() => false) - .verifiable(TypeMoq.Times.never()); - experiments - .setup((ex) => ex.sendTelemetryIfInExperiment(LSControl)) - .returns(() => undefined) - .verifiable(TypeMoq.Times.never()); - workspaceConfig - .setup((c) => c.inspect('languageServer')) - .returns(() => settings as any) - .verifiable(TypeMoq.Times.once()); - pythonSettings - .setup((p) => p.languageServer) - .returns(() => testParams.pythonSettingsValue) - .verifiable(TypeMoq.Times.once()); - - const activationService = new LanguageServerExtensionActivationService( - serviceContainer.object, - stateFactory.object, - experiments.object - ); - const result = activationService.useJedi(); - expect(result).to.equal( - testParams.expectedResult, - `Return value should be ${testParams.expectedResult}` - ); - - pythonSettings.verifyAll(); - experiments.verifyAll(); - workspaceService.verifyAll(); - workspaceConfig.verifyAll(); - }); - }); - } - ); - }); - suite('Function isJediUsingDefaultConfiguration()', () => { let serviceContainer: TypeMoq.IMock; let pythonSettings: TypeMoq.IMock; @@ -1062,7 +821,6 @@ suite('Language Server Activation - ActivationService', () => { let lsNotSupportedDiagnosticService: TypeMoq.IMock; let stateFactory: TypeMoq.IMock; let state: TypeMoq.IMock>; - let experiments: TypeMoq.IMock; let workspaceConfig: TypeMoq.IMock; let interpreterService: TypeMoq.IMock; setup(() => { @@ -1075,7 +833,6 @@ suite('Language Server Activation - ActivationService', () => { state = TypeMoq.Mock.ofType>(); const configService = TypeMoq.Mock.ofType(); pythonSettings = TypeMoq.Mock.ofType(); - experiments = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); const e = new EventEmitter(); interpreterService.setup((i) => i.onDidChangeInterpreter).returns(() => e.event); @@ -1166,8 +923,7 @@ suite('Language Server Activation - ActivationService', () => { const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); const result = activationService.isJediUsingDefaultConfiguration(Uri.parse('a')); expect(result).to.equal(expectedResult); @@ -1187,8 +943,7 @@ suite('Language Server Activation - ActivationService', () => { const activationService = new LanguageServerExtensionActivationService( serviceContainer.object, - stateFactory.object, - experiments.object + stateFactory.object ); const result = activationService.isJediUsingDefaultConfiguration(Uri.parse('a')); expect(result).to.equal(false, 'Return value should be false'); From f29f7d7f6658c44cb5c1b3ca74bd9a8ad671b934 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Wed, 8 Jul 2020 14:15:30 -0700 Subject: [PATCH 09/21] Frequency + tests --- package.nls.json | 3 + .../activation/languageServer/activator.ts | 18 +- src/client/activation/none/activator.ts | 17 +- src/client/activation/serviceRegistry.ts | 4 +- src/client/common/types.ts | 2 +- src/client/common/utils/localize.ts | 7 + .../proposeLanguageServerBanner.ts | 78 ++++---- src/client/providers/jediProxy.ts | 18 +- .../languageServer/activator.unit.test.ts | 8 +- .../activation/serviceRegistry.unit.test.ts | 4 +- ...roposeNewLanguageServerBanner.unit.test.ts | 189 +++++++++--------- 11 files changed, 184 insertions(+), 164 deletions(-) diff --git a/package.nls.json b/package.nls.json index c3638945435e..16fb857e8979 100644 --- a/package.nls.json +++ b/package.nls.json @@ -106,6 +106,9 @@ "python.snippet.launch.django.label": "Python: Django", "python.snippet.launch.flask.label": "Python: Flask", "python.snippet.launch.pyramid.label": "Python: Pyramid Application", + "LanguageService.proposePylanceMessage": "Try out a new faster, feature-rich language server for Python by Microsoft - Pylance!", + "LanguageService.tryItNow" : "Try it now", + "LanguageService.remindMeLater" : "Remind me later", "LanguageService.bannerLabelYes": "Yes, take survey now", "LanguageService.bannerLabelNo": "No, thanks", "LanguageService.lsFailedToStart": "We encountered an issue starting the Language Server. Reverting to the alternative, Jedi. Check the Python output panel for details.", diff --git a/src/client/activation/languageServer/activator.ts b/src/client/activation/languageServer/activator.ts index 42194f36adbe..7b183a951424 100644 --- a/src/client/activation/languageServer/activator.ts +++ b/src/client/activation/languageServer/activator.ts @@ -1,13 +1,15 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { inject, injectable } from 'inversify'; +import { inject, injectable, named } from 'inversify'; import * as path from 'path'; import { IWorkspaceService } from '../../common/application/types'; +import { isTestExecution } from '../../common/constants'; import { traceDecorators } from '../../common/logger'; import { IFileSystem } from '../../common/platform/types'; -import { IConfigurationService, Resource } from '../../common/types'; +import { BANNER_NAME_PROPOSE_LS, IConfigurationService, IPythonExtensionBanner, Resource } from '../../common/types'; +import { PythonInterpreter } from '../../pythonEnvironments/info'; import { LanguageServerActivatorBase } from '../common/activatorBase'; import { ILanguageServerDownloader, ILanguageServerFolderService, ILanguageServerManager } from '../types'; @@ -27,11 +29,21 @@ export class DotNetLanguageServerActivator extends LanguageServerActivatorBase { @inject(IFileSystem) fs: IFileSystem, @inject(ILanguageServerDownloader) lsDownloader: ILanguageServerDownloader, @inject(ILanguageServerFolderService) languageServerFolderService: ILanguageServerFolderService, - @inject(IConfigurationService) configurationService: IConfigurationService + @inject(IConfigurationService) configurationService: IConfigurationService, + @inject(IPythonExtensionBanner) + @named(BANNER_NAME_PROPOSE_LS) + private proposePylancePopup: IPythonExtensionBanner ) { super(manager, workspace, fs, lsDownloader, languageServerFolderService, configurationService); } + public async start(resource: Resource, interpreter?: PythonInterpreter): Promise { + if (!isTestExecution()) { + await this.proposePylancePopup.showBanner(); + } + return super.start(resource, interpreter); + } + @traceDecorators.error('Failed to ensure language server is available') public async ensureLanguageServerIsAvailable(resource: Resource): Promise { const languageServerFolderPath = await this.ensureLanguageServerFileIsAvailable(resource, 'mscorlib.dll'); diff --git a/src/client/activation/none/activator.ts b/src/client/activation/none/activator.ts index 8d808ae87ce0..f9cc3f58611b 100644 --- a/src/client/activation/none/activator.ts +++ b/src/client/activation/none/activator.ts @@ -1,6 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { injectable } from 'inversify'; +import { inject, injectable, named } from 'inversify'; import { CancellationToken, CodeLens, @@ -20,7 +20,8 @@ import { TextDocument, WorkspaceEdit } from 'vscode'; -import { Resource } from '../../common/types'; +import { isTestExecution } from '../../common/constants'; +import { BANNER_NAME_PROPOSE_LS, IPythonExtensionBanner, Resource } from '../../common/types'; import { PythonInterpreter } from '../../pythonEnvironments/info'; import { ILanguageServerActivator } from '../types'; @@ -33,8 +34,16 @@ import { ILanguageServerActivator } from '../types'; */ @injectable() export class NoLanguageServerExtensionActivator implements ILanguageServerActivator { - // tslint:disable-next-line: no-empty - public async start(_resource: Resource, _interpreter?: PythonInterpreter): Promise {} + constructor( + @inject(IPythonExtensionBanner) + @named(BANNER_NAME_PROPOSE_LS) + private proposePylancePopup: IPythonExtensionBanner + ) {} + public async start(_resource: Resource, _interpreter?: PythonInterpreter): Promise { + if (!isTestExecution()) { + await this.proposePylancePopup.showBanner(); + } + } // tslint:disable-next-line: no-empty public dispose(): void {} // tslint:disable-next-line: no-empty diff --git a/src/client/activation/serviceRegistry.ts b/src/client/activation/serviceRegistry.ts index f5bec7c65f35..0f1fa5497f67 100644 --- a/src/client/activation/serviceRegistry.ts +++ b/src/client/activation/serviceRegistry.ts @@ -12,7 +12,7 @@ import { import { DataScienceSurveyBanner } from '../datascience/dataScienceSurveyBanner'; import { InteractiveShiftEnterBanner } from '../datascience/shiftEnterBanner'; import { IServiceManager } from '../ioc/types'; -import { ProposeLanguageServerBanner } from '../languageServices/proposeLanguageServerBanner'; +import { ProposePylanceBanner } from '../languageServices/proposeLanguageServerBanner'; import { AATesting } from './aaTesting'; import { ExtensionActivationManager } from './activationManager'; import { LanguageServerExtensionActivationService } from './activationService'; @@ -86,7 +86,7 @@ export function registerTypes(serviceManager: IServiceManager, languageServerTyp serviceManager.addSingleton( IPythonExtensionBanner, - ProposeLanguageServerBanner, + ProposePylanceBanner, BANNER_NAME_PROPOSE_LS ); serviceManager.addSingleton( diff --git a/src/client/common/types.ts b/src/client/common/types.ts index 99557821e85c..d7fab4ddc24e 100644 --- a/src/client/common/types.ts +++ b/src/client/common/types.ts @@ -525,7 +525,7 @@ export interface IPythonExtensionBanner { readonly enabled: boolean; showBanner(): Promise; } -export const BANNER_NAME_PROPOSE_LS: string = 'ProposeLS'; +export const BANNER_NAME_PROPOSE_LS: string = 'ProposePylance'; export const BANNER_NAME_DS_SURVEY: string = 'DSSurveyBanner'; export const BANNER_NAME_INTERACTIVE_SHIFTENTER: string = 'InteractiveShiftEnterBanner'; diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 455069417e30..2a8302139528 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -101,6 +101,13 @@ export namespace AttachProcess { } export namespace LanguageService { + export const proposePylanceMessage = localize( + 'LanguageService.proposePylanceMessage', + 'Try out a new faster, feature-rich language server for Python by Microsoft - Pylance!' + ); + export const tryItNow = localize('LanguageService.tryItNow', 'Try it now'); + export const remindMeLater = localize('LanguageService.remindMeLater', 'Remind me later'); + export const bannerLabelYes = localize('LanguageService.bannerLabelYes', 'Yes, take survey now'); export const bannerLabelNo = localize('LanguageService.bannerLabelNo', 'No, thanks'); export const lsFailedToStart = localize( diff --git a/src/client/languageServices/proposeLanguageServerBanner.ts b/src/client/languageServices/proposeLanguageServerBanner.ts index 55645f90f220..8c94b0cfc8b7 100644 --- a/src/client/languageServices/proposeLanguageServerBanner.ts +++ b/src/client/languageServices/proposeLanguageServerBanner.ts @@ -6,21 +6,24 @@ import { inject, injectable } from 'inversify'; import { ConfigurationTarget } from 'vscode'; import { LanguageServerType } from '../activation/types'; -import { IApplicationShell } from '../common/application/types'; +import { IApplicationEnvironment, IApplicationShell } from '../common/application/types'; import '../common/extensions'; import { IConfigurationService, IPersistentStateFactory, IPythonExtensionBanner } from '../common/types'; +import { LanguageService } from '../common/utils/localize'; import { getRandomBetween } from '../common/utils/random'; // persistent state names, exported to make use of in testing export enum ProposeLSStateKeys { - ShowBanner = 'ProposeLSBanner' + ShowBanner = 'ProposePylanceBanner' } -enum ProposeLSLabelIndex { - Yes, - No, - Later -} +const frequencyPerServerType = new Map([ + [LanguageServerType.Node, 0], + [LanguageServerType.Microsoft, 50], + [LanguageServerType.None, 50], + // Banner for Jedi users is suppressed until further notice. + [LanguageServerType.Jedi, 0] +]); /* This class represents a popup that propose that the user try out a new @@ -30,21 +33,28 @@ and will show as soon as it is instructed to do so, if a random sample function enables the popup for this user. */ @injectable() -export class ProposeLanguageServerBanner implements IPythonExtensionBanner { +export class ProposePylanceBanner implements IPythonExtensionBanner { private initialized?: boolean; private disabledInCurrentSession: boolean = false; - private sampleSizePerHundred: number; - private bannerMessage: string = - 'Try out Preview of our new Python Language Server to get richer and faster IntelliSense completions, and syntax errors as you type.'; - private bannerLabels: string[] = ['Try it now', 'No thanks', 'Remind me Later']; + private sampleSizePerHundred = 0; constructor( @inject(IApplicationShell) private appShell: IApplicationShell, @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory, @inject(IConfigurationService) private configuration: IConfigurationService, - sampleSizePerOneHundredUsers: number = 10 + @inject(IApplicationEnvironment) private appEnvirontment: IApplicationEnvironment, + sampleSizePerHundred = -1 ) { - this.sampleSizePerHundred = sampleSizePerOneHundredUsers; + if (this.appEnvirontment.channel === 'insiders') { + this.sampleSizePerHundred = 100; + } else { + if (sampleSizePerHundred >= 0) { + this.sampleSizePerHundred = sampleSizePerHundred; + } else { + const lsType = this.configuration.getSettings().languageServer ?? LanguageServerType.Jedi; + this.sampleSizePerHundred = frequencyPerServerType.get(lsType) ?? 0; + } + } this.initialize(); } @@ -59,7 +69,6 @@ export class ProposeLanguageServerBanner implements IPythonExtensionBanner { return; } - // we only want 10% of folks that use Jedi to see this survey. const randomSample: number = getRandomBetween(0, 100); if (randomSample >= this.sampleSizePerHundred) { this.disable().ignoreErrors(); @@ -67,9 +76,7 @@ export class ProposeLanguageServerBanner implements IPythonExtensionBanner { } } public get enabled(): boolean { - // Banner is deactivated. - return false; - // return this.persistentState.createGlobalPersistentState(ProposeLSStateKeys.ShowBanner, true).value; + return this.persistentState.createGlobalPersistentState(ProposeLSStateKeys.ShowBanner, true).value; } public async showBanner(): Promise { @@ -82,25 +89,20 @@ export class ProposeLanguageServerBanner implements IPythonExtensionBanner { return; } - const response = await this.appShell.showInformationMessage(this.bannerMessage, ...this.bannerLabels); - switch (response) { - case this.bannerLabels[ProposeLSLabelIndex.Yes]: { - await this.enableLanguageServer(); - await this.disable(); - break; - } - case this.bannerLabels[ProposeLSLabelIndex.No]: { - await this.disable(); - break; - } - case this.bannerLabels[ProposeLSLabelIndex.Later]: { - this.disabledInCurrentSession = true; - break; - } - default: { - // Disable for the current session. - this.disabledInCurrentSession = true; - } + const response = await this.appShell.showInformationMessage( + LanguageService.proposePylanceMessage(), + LanguageService.tryItNow(), + LanguageService.bannerLabelNo(), + LanguageService.remindMeLater() + ); + + if (response === LanguageService.tryItNow()) { + await this.enableLanguageServer(); + await this.disable(); + } else if (response === LanguageService.bannerLabelNo()) { + await this.disable(); + } else { + this.disabledInCurrentSession = true; } } @@ -117,7 +119,7 @@ export class ProposeLanguageServerBanner implements IPythonExtensionBanner { public async enableLanguageServer(): Promise { await this.configuration.updateSetting( 'languageServer', - LanguageServerType.Microsoft, + LanguageServerType.Node, undefined, ConfigurationTarget.Global ); diff --git a/src/client/providers/jediProxy.ts b/src/client/providers/jediProxy.ts index c72a700f03f7..add675bb2e76 100644 --- a/src/client/providers/jediProxy.ts +++ b/src/client/providers/jediProxy.ts @@ -7,19 +7,13 @@ import * as path from 'path'; // @ts-ignore import * as pidusage from 'pidusage'; import { CancellationToken, CancellationTokenSource, CompletionItemKind, Disposable, SymbolKind, Uri } from 'vscode'; -import { isTestExecution } from '../common/constants'; import '../common/extensions'; import { IS_WINDOWS } from '../common/platform/constants'; import { IFileSystem } from '../common/platform/types'; import * as internalPython from '../common/process/internal/python'; import * as internalScripts from '../common/process/internal/scripts'; import { IPythonExecutionFactory } from '../common/process/types'; -import { - BANNER_NAME_PROPOSE_LS, - IConfigurationService, - IPythonExtensionBanner, - IPythonSettings -} from '../common/types'; +import { IConfigurationService, IPythonSettings } from '../common/types'; import { createDeferred, Deferred } from '../common/utils/async'; import { swallowExceptions } from '../common/utils/decorators'; import { StopWatch } from '../common/utils/stopWatch'; @@ -157,7 +151,6 @@ export class JediProxy implements Disposable { private pidUsageFailures = { timer: new StopWatch(), counter: 0 }; private lastCmdIdProcessed?: number; private lastCmdIdProcessedForPidUsage?: number; - private proposeNewLanguageServerPopup: IPythonExtensionBanner; private readonly disposables: Disposable[] = []; private timer?: NodeJS.Timer | number; @@ -174,12 +167,6 @@ export class JediProxy implements Disposable { this.startLanguageServer() .then(() => this.initialized.resolve()) .ignoreErrors(); - - this.proposeNewLanguageServerPopup = serviceContainer.get( - IPythonExtensionBanner, - BANNER_NAME_PROPOSE_LS - ); - this.checkJediMemoryFootprint().ignoreErrors(); } @@ -332,9 +319,6 @@ export class JediProxy implements Disposable { private async startLanguageServer(): Promise { const newAutoComletePaths = await this.buildAutoCompletePaths(); this.additionalAutoCompletePaths = newAutoComletePaths; - if (!isTestExecution()) { - await this.proposeNewLanguageServerPopup.showBanner(); - } return this.restartLanguageServer(); } private restartLanguageServer(): Promise { diff --git a/src/test/activation/languageServer/activator.unit.test.ts b/src/test/activation/languageServer/activator.unit.test.ts index 87fa327568a7..1b393fa95488 100644 --- a/src/test/activation/languageServer/activator.unit.test.ts +++ b/src/test/activation/languageServer/activator.unit.test.ts @@ -21,9 +21,10 @@ import { PythonSettings } from '../../../client/common/configSettings'; import { ConfigurationService } from '../../../client/common/configuration/service'; import { FileSystem } from '../../../client/common/platform/fileSystem'; import { IFileSystem } from '../../../client/common/platform/types'; -import { IConfigurationService, IPythonSettings } from '../../../client/common/types'; +import { IConfigurationService, IPythonExtensionBanner, IPythonSettings } from '../../../client/common/types'; import { createDeferred } from '../../../client/common/utils/async'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; +import { ProposePylanceBanner } from '../../../client/languageServices/proposeLanguageServerBanner'; import { sleep } from '../../core'; // tslint:disable:max-func-body-length @@ -37,6 +38,7 @@ suite('Language Server - Activator', () => { let lsFolderService: ILanguageServerFolderService; let configuration: IConfigurationService; let settings: IPythonSettings; + let banner: IPythonExtensionBanner; setup(() => { manager = mock(DotNetLanguageServerManager); workspaceService = mock(WorkspaceService); @@ -45,6 +47,7 @@ suite('Language Server - Activator', () => { lsFolderService = mock(DotNetLanguageServerFolderService); configuration = mock(ConfigurationService); settings = mock(PythonSettings); + banner = mock(ProposePylanceBanner); when(configuration.getSettings(anything())).thenReturn(instance(settings)); activator = new DotNetLanguageServerActivator( instance(manager), @@ -52,7 +55,8 @@ suite('Language Server - Activator', () => { instance(fs), instance(lsDownloader), instance(lsFolderService), - instance(configuration) + instance(configuration), + instance(banner) ); }); test('Manager must be started without any workspace', async () => { diff --git a/src/test/activation/serviceRegistry.unit.test.ts b/src/test/activation/serviceRegistry.unit.test.ts index 18e2bc853ba3..51219665453e 100644 --- a/src/test/activation/serviceRegistry.unit.test.ts +++ b/src/test/activation/serviceRegistry.unit.test.ts @@ -57,7 +57,7 @@ import { DataScienceSurveyBanner } from '../../client/datascience/dataScienceSur import { InteractiveShiftEnterBanner } from '../../client/datascience/shiftEnterBanner'; import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceManager } from '../../client/ioc/types'; -import { ProposeLanguageServerBanner } from '../../client/languageServices/proposeLanguageServerBanner'; +import { ProposePylanceBanner } from '../../client/languageServices/proposeLanguageServerBanner'; // tslint:disable:max-func-body-length @@ -101,7 +101,7 @@ suite('Unit Tests - Language Server Activation Service Registry', () => { verify( serviceManager.addSingleton( IPythonExtensionBanner, - ProposeLanguageServerBanner, + ProposePylanceBanner, BANNER_NAME_PROPOSE_LS ) ).once(); diff --git a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts index 42e34a2106a0..791a0eb259d6 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts @@ -3,103 +3,102 @@ 'use strict'; -// tslint:disable:no-any max-func-body-length +import { expect } from 'chai'; +import * as typemoq from 'typemoq'; +import { IApplicationEnvironment, IApplicationShell } from '../../../client/common/application/types'; +import { IConfigurationService, IPersistentState, IPersistentStateFactory } from '../../../client/common/types'; +import { LanguageService } from '../../../client/common/utils/localize'; +import { ProposeLSStateKeys, ProposePylanceBanner } from '../../../client/languageServices/proposeLanguageServerBanner'; -// import { expect } from 'chai'; -// import * as typemoq from 'typemoq'; -// import { IApplicationShell } from '../../../client/common/application/types'; -// import { IConfigurationService, IPersistentState, IPersistentStateFactory } from '../../../client/common/types'; -// import { -// ProposeLanguageServerBanner, -// ProposeLSStateKeys -// } from '../../../client/languageServices/proposeLanguageServerBanner'; +suite('Propose Pylance Banner', () => { + let config: typemoq.IMock; + let appShell: typemoq.IMock; + let appEnv: typemoq.IMock; -// suite('Propose New Language Server Banner', () => { -// let config: typemoq.IMock; -// let appShell: typemoq.IMock; -// const message = -// 'Try out Preview of our new Python Language Server to get richer and faster IntelliSense completions, and syntax errors as you type.'; -// const yes = 'Try it now'; -// const no = 'No thanks'; -// const later = 'Remind me Later'; + const message = LanguageService.proposePylanceMessage(); + const yes = LanguageService.tryItNow(); + const no = LanguageService.bannerLabelNo(); + const later = LanguageService.remindMeLater(); -// setup(() => { -// config = typemoq.Mock.ofType(); -// appShell = typemoq.Mock.ofType(); -// }); -// test('Is debugger enabled upon creation?', () => { -// const enabledValue: boolean = true; -// const testBanner: ProposeLanguageServerBanner = preparePopup(enabledValue, 100, appShell.object, config.object); -// expect(testBanner.enabled).to.be.equal(true, 'Sampling 100/100 should always enable the banner.'); -// }); -// test('Do not show banner when it is disabled', () => { -// appShell -// .setup((a) => -// a.showInformationMessage( -// typemoq.It.isValue(message), -// typemoq.It.isValue(yes), -// typemoq.It.isValue(no), -// typemoq.It.isValue(later) -// ) -// ) -// .verifiable(typemoq.Times.never()); -// const enabled: boolean = true; -// const testBanner: ProposeLanguageServerBanner = preparePopup(enabled, 0, appShell.object, config.object); -// testBanner.showBanner().ignoreErrors(); -// }); -// test('shouldShowBanner must return false when Banner is implicitly disabled by sampling', () => { -// const enabled: boolean = true; -// const testBanner: ProposeLanguageServerBanner = preparePopup(enabled, 0, appShell.object, config.object); -// expect(testBanner.enabled).to.be.equal(false, 'We implicitly disabled the banner, it should never show.'); -// }); -// test('shouldShowBanner must return false when Banner is explicitly disabled', async () => { -// const enabled: boolean = true; -// const testBanner: ProposeLanguageServerBanner = preparePopup(enabled, 100, appShell.object, config.object); + setup(() => { + config = typemoq.Mock.ofType(); + appShell = typemoq.Mock.ofType(); + appEnv = typemoq.Mock.ofType(); + }); + test('Sampling 100/100 enables the banner', () => { + const enabledValue = true; + const testBanner = preparePopup(enabledValue, appShell.object, config.object, appEnv.object, 100); + expect(testBanner.enabled).to.be.equal(true, 'Sampling 100/100 should always enable the banner.'); + }); + test('Do not show banner when it is disabled', () => { + appShell + .setup((a) => + a.showInformationMessage( + typemoq.It.isValue(message), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later) + ) + ) + .verifiable(typemoq.Times.never()); + const enabled: boolean = true; + const testBanner = preparePopup(enabled, appShell.object, config.object, appEnv.object, 0); + testBanner.showBanner().ignoreErrors(); + }); + test('shouldShowBanner must return false when Banner is implicitly disabled by sampling', () => { + const enabled = true; + const testBanner = preparePopup(enabled, appShell.object, config.object, appEnv.object, 0); + expect(testBanner.enabled).to.be.equal(false, 'We implicitly disabled the banner, it should never show.'); + }); + test('shouldShowBanner must return false when Banner is explicitly disabled', async () => { + const enabled = true; + const testBanner = preparePopup(enabled, appShell.object, config.object, appEnv.object, 100); -// expect(await testBanner.shouldShowBanner()).to.be.equal( -// true, -// '100% sample size should always make the banner enabled.' -// ); -// await testBanner.disable(); -// expect(await testBanner.shouldShowBanner()).to.be.equal( -// false, -// 'Explicitly disabled banner shouldShowBanner != false.' -// ); -// }); -// }); + expect(await testBanner.shouldShowBanner()).to.be.equal( + true, + '100% sample size should always make the banner enabled.' + ); + await testBanner.disable(); + expect(await testBanner.shouldShowBanner()).to.be.equal( + false, + 'Explicitly disabled banner shouldShowBanner != false.' + ); + }); +}); -// function preparePopup( -// enabledValue: boolean, -// sampleValue: number, -// appShell: IApplicationShell, -// config: IConfigurationService -// ): ProposeLanguageServerBanner { -// const myfactory: typemoq.IMock = typemoq.Mock.ofType(); -// const val: typemoq.IMock> = typemoq.Mock.ofType>(); -// val.setup((a) => a.updateValue(typemoq.It.isValue(true))).returns(() => { -// enabledValue = true; -// return Promise.resolve(); -// }); -// val.setup((a) => a.updateValue(typemoq.It.isValue(false))).returns(() => { -// enabledValue = false; -// return Promise.resolve(); -// }); -// val.setup((a) => a.value).returns(() => { -// return enabledValue; -// }); -// myfactory -// .setup((a) => -// a.createGlobalPersistentState(typemoq.It.isValue(ProposeLSStateKeys.ShowBanner), typemoq.It.isValue(true)) -// ) -// .returns(() => { -// return val.object; -// }); -// myfactory -// .setup((a) => -// a.createGlobalPersistentState(typemoq.It.isValue(ProposeLSStateKeys.ShowBanner), typemoq.It.isValue(false)) -// ) -// .returns(() => { -// return val.object; -// }); -// return new ProposeLanguageServerBanner(appShell, myfactory.object, config, sampleValue); -// } +function preparePopup( + enabledValue: boolean, + appShell: IApplicationShell, + config: IConfigurationService, + appEnv: IApplicationEnvironment, + sampleSizePerHundred: number +): ProposePylanceBanner { + const myfactory: typemoq.IMock = typemoq.Mock.ofType(); + const val: typemoq.IMock> = typemoq.Mock.ofType>(); + val.setup((a) => a.updateValue(typemoq.It.isValue(true))).returns(() => { + enabledValue = true; + return Promise.resolve(); + }); + val.setup((a) => a.updateValue(typemoq.It.isValue(false))).returns(() => { + enabledValue = false; + return Promise.resolve(); + }); + val.setup((a) => a.value).returns(() => { + return enabledValue; + }); + myfactory + .setup((a) => + a.createGlobalPersistentState(typemoq.It.isValue(ProposeLSStateKeys.ShowBanner), typemoq.It.isValue(true)) + ) + .returns(() => { + return val.object; + }); + myfactory + .setup((a) => + a.createGlobalPersistentState(typemoq.It.isValue(ProposeLSStateKeys.ShowBanner), typemoq.It.isValue(false)) + ) + .returns(() => { + return val.object; + }); + return new ProposePylanceBanner(appShell, myfactory.object, config, appEnv, sampleSizePerHundred); +} From 19ccc2059c76ca114c9cb294ee0e6f4c5570ee80 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Wed, 8 Jul 2020 15:02:48 -0700 Subject: [PATCH 10/21] Fix test --- src/test/datascience/dataScienceIocContainer.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/test/datascience/dataScienceIocContainer.ts b/src/test/datascience/dataScienceIocContainer.ts index c86b6a1cf202..a72303d9545b 100644 --- a/src/test/datascience/dataScienceIocContainer.ts +++ b/src/test/datascience/dataScienceIocContainer.ts @@ -151,6 +151,7 @@ import { TerminalActivationProviders } from '../../client/common/terminal/types'; import { + BANNER_NAME_PROPOSE_LS, GLOBAL_MEMENTO, IAsyncDisposableRegistry, IBrowserService, @@ -170,6 +171,7 @@ import { IOutputChannel, IPathUtils, IPersistentStateFactory, + IPythonExtensionBanner, IPythonSettings, IsWindows, ProductType, @@ -365,6 +367,7 @@ import { InterpreterVersionService } from '../../client/interpreter/interpreterV import { registerInterpreterTypes } from '../../client/interpreter/serviceRegistry'; import { VirtualEnvironmentManager } from '../../client/interpreter/virtualEnvs'; import { IVirtualEnvironmentManager } from '../../client/interpreter/virtualEnvs/types'; +import { ProposePylanceBanner } from '../../client/languageServices/proposeLanguageServerBanner'; import { CacheableLocatorPromiseCache } from '../../client/pythonEnvironments/discovery/locators/services/cacheableLocatorService'; import { InterpreterType, PythonInterpreter } from '../../client/pythonEnvironments/info'; import { registerForIOC } from '../../client/pythonEnvironments/legacyIOC'; @@ -757,6 +760,11 @@ export class DataScienceIocContainer extends UnitTestIocContainer { LanguageServerType.Microsoft ); this.serviceManager.add(ILanguageServerManager, DotNetLanguageServerManager); + this.serviceManager.add( + IPythonExtensionBanner, + ProposePylanceBanner, + BANNER_NAME_PROPOSE_LS + ); } else if (languageServerType === LanguageServerType.Node) { this.serviceManager.add( ILanguageServerActivator, From 4dec978c44b7eb2eed4e314dddfc4a89deece2f0 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 15 Jul 2020 16:16:30 -0700 Subject: [PATCH 11/21] Update message to match spec --- package.nls.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package.nls.json b/package.nls.json index 16fb857e8979..99b3ca99b0dd 100644 --- a/package.nls.json +++ b/package.nls.json @@ -106,9 +106,9 @@ "python.snippet.launch.django.label": "Python: Django", "python.snippet.launch.flask.label": "Python: Flask", "python.snippet.launch.pyramid.label": "Python: Pyramid Application", - "LanguageService.proposePylanceMessage": "Try out a new faster, feature-rich language server for Python by Microsoft - Pylance!", - "LanguageService.tryItNow" : "Try it now", - "LanguageService.remindMeLater" : "Remind me later", + "LanguageService.proposePylanceMessage": "Try out a new faster, feature-rich language server for Python by Microsoft, Pylance! Install the extension now.", + "LanguageService.tryItNow": "Try it now", + "LanguageService.remindMeLater": "Remind me later", "LanguageService.bannerLabelYes": "Yes, take survey now", "LanguageService.bannerLabelNo": "No, thanks", "LanguageService.lsFailedToStart": "We encountered an issue starting the Language Server. Reverting to the alternative, Jedi. Check the Python output panel for details.", From a7fe6a81de84fb31093c78ec0fbd4080f8a67d72 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 15 Jul 2020 19:09:07 -0700 Subject: [PATCH 12/21] Open workspace for extension rather than changing setting --- .../node/languageServerFolderService.ts | 5 ++-- src/client/common/constants.ts | 2 ++ .../proposeLanguageServerBanner.ts | 16 ++++------ .../languageServerFolderService.unit.test.ts | 8 ++--- ...roposeNewLanguageServerBanner.unit.test.ts | 29 +++++++++++++++++-- 5 files changed, 39 insertions(+), 21 deletions(-) diff --git a/src/client/activation/node/languageServerFolderService.ts b/src/client/activation/node/languageServerFolderService.ts index 44922a052970..0ccb89b28fc3 100644 --- a/src/client/activation/node/languageServerFolderService.ts +++ b/src/client/activation/node/languageServerFolderService.ts @@ -8,14 +8,13 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { SemVer } from 'semver'; import { IWorkspaceService } from '../../common/application/types'; +import { PYLANCE_EXTENSION_ID } from '../../common/constants'; import { NugetPackage } from '../../common/nuget/types'; import { IConfigurationService, IExtensions, Resource } from '../../common/types'; import { IServiceContainer } from '../../ioc/types'; import { LanguageServerFolderService } from '../common/languageServerFolderService'; import { FolderVersionPair, ILanguageServerFolderService, NodeLanguageServerFolder } from '../types'; -export const PylanceExtensionName = 'ms-python.vscode-pylance'; - class FallbackNodeLanguageServerFolderService extends LanguageServerFolderService { constructor(serviceContainer: IServiceContainer) { super(serviceContainer, NodeLanguageServerFolder); @@ -101,7 +100,7 @@ export class NodeLanguageServerFolderService implements ILanguageServerFolderSer return undefined; } - const extension = this.extensions.getExtension(PylanceExtensionName); + const extension = this.extensions.getExtension(PYLANCE_EXTENSION_ID); if (!extension) { return undefined; } diff --git a/src/client/common/constants.ts b/src/client/common/constants.ts index fb4ccf0e9ef9..7abe0634ac2e 100644 --- a/src/client/common/constants.ts +++ b/src/client/common/constants.ts @@ -114,4 +114,6 @@ export const UseCustomEditorApi = Symbol('USE_CUSTOM_EDITOR'); export const UseVSCodeNotebookEditorApi = Symbol('USE_NATIVEEDITOR'); export const UseProposedApi = Symbol('USE_VSC_PROPOSED_API'); +export const PYLANCE_EXTENSION_ID = 'ms-python.vscode-pylance'; + export * from '../constants'; diff --git a/src/client/languageServices/proposeLanguageServerBanner.ts b/src/client/languageServices/proposeLanguageServerBanner.ts index 8c94b0cfc8b7..a1f23880920b 100644 --- a/src/client/languageServices/proposeLanguageServerBanner.ts +++ b/src/client/languageServices/proposeLanguageServerBanner.ts @@ -4,14 +4,17 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import { ConfigurationTarget } from 'vscode'; +import * as vscode from 'vscode'; import { LanguageServerType } from '../activation/types'; import { IApplicationEnvironment, IApplicationShell } from '../common/application/types'; +import { PYLANCE_EXTENSION_ID } from '../common/constants'; import '../common/extensions'; import { IConfigurationService, IPersistentStateFactory, IPythonExtensionBanner } from '../common/types'; import { LanguageService } from '../common/utils/localize'; import { getRandomBetween } from '../common/utils/random'; +export const PylanceExtensionUri = `${vscode.env.uriScheme}:extension/${PYLANCE_EXTENSION_ID}`; + // persistent state names, exported to make use of in testing export enum ProposeLSStateKeys { ShowBanner = 'ProposePylanceBanner' @@ -97,7 +100,7 @@ export class ProposePylanceBanner implements IPythonExtensionBanner { ); if (response === LanguageService.tryItNow()) { - await this.enableLanguageServer(); + this.appShell.openUrl(PylanceExtensionUri); await this.disable(); } else if (response === LanguageService.bannerLabelNo()) { await this.disable(); @@ -115,13 +118,4 @@ export class ProposePylanceBanner implements IPythonExtensionBanner { .createGlobalPersistentState(ProposeLSStateKeys.ShowBanner, false) .updateValue(false); } - - public async enableLanguageServer(): Promise { - await this.configuration.updateSetting( - 'languageServer', - LanguageServerType.Node, - undefined, - ConfigurationTarget.Global - ); - } } diff --git a/src/test/activation/node/languageServerFolderService.unit.test.ts b/src/test/activation/node/languageServerFolderService.unit.test.ts index 115bdecc7eda..b944dde8ffb6 100644 --- a/src/test/activation/node/languageServerFolderService.unit.test.ts +++ b/src/test/activation/node/languageServerFolderService.unit.test.ts @@ -9,10 +9,10 @@ import { Extension, Uri, WorkspaceConfiguration } from 'vscode'; import { ILanguageServerFolder, ILSExtensionApi, - NodeLanguageServerFolderService, - PylanceExtensionName + NodeLanguageServerFolderService } from '../../../client/activation/node/languageServerFolderService'; import { IWorkspaceService } from '../../../client/common/application/types'; +import { PYLANCE_EXTENSION_ID } from '../../../client/common/constants'; import { IConfigurationService, IExtensions, IPythonSettings } from '../../../client/common/types'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -115,7 +115,7 @@ suite('Node Language Server Folder Service', () => { test('lsExtension not installed', async () => { pythonSettings.setup((p) => p.downloadLanguageServer).returns(() => true); workspaceConfiguration.setup((wc) => wc.get('packageName')).returns(() => undefined); - extensions.setup((e) => e.getExtension(PylanceExtensionName)).returns(() => undefined); + extensions.setup((e) => e.getExtension(PYLANCE_EXTENSION_ID)).returns(() => undefined); const folderService = new TestService( serviceContainer.object, @@ -148,7 +148,7 @@ suite('Node Language Server Folder Service', () => { extension.setup((e) => e.exports).returns(() => extensionApi); pythonSettings.setup((p) => p.downloadLanguageServer).returns(() => true); workspaceConfiguration.setup((wc) => wc.get('packageName')).returns(() => undefined); - extensions.setup((e) => e.getExtension(PylanceExtensionName)).returns(() => extension.object); + extensions.setup((e) => e.getExtension(PYLANCE_EXTENSION_ID)).returns(() => extension.object); folderService = new TestService( serviceContainer.object, configService.object, diff --git a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts index 791a0eb259d6..ed9e563cd783 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts @@ -8,7 +8,11 @@ import * as typemoq from 'typemoq'; import { IApplicationEnvironment, IApplicationShell } from '../../../client/common/application/types'; import { IConfigurationService, IPersistentState, IPersistentStateFactory } from '../../../client/common/types'; import { LanguageService } from '../../../client/common/utils/localize'; -import { ProposeLSStateKeys, ProposePylanceBanner } from '../../../client/languageServices/proposeLanguageServerBanner'; +import { + ProposeLSStateKeys, + ProposePylanceBanner, + PylanceExtensionUri +} from '../../../client/languageServices/proposeLanguageServerBanner'; suite('Propose Pylance Banner', () => { let config: typemoq.IMock; @@ -30,7 +34,7 @@ suite('Propose Pylance Banner', () => { const testBanner = preparePopup(enabledValue, appShell.object, config.object, appEnv.object, 100); expect(testBanner.enabled).to.be.equal(true, 'Sampling 100/100 should always enable the banner.'); }); - test('Do not show banner when it is disabled', () => { + test('Do not show banner when it is disabled', async () => { appShell .setup((a) => a.showInformationMessage( @@ -43,7 +47,8 @@ suite('Propose Pylance Banner', () => { .verifiable(typemoq.Times.never()); const enabled: boolean = true; const testBanner = preparePopup(enabled, appShell.object, config.object, appEnv.object, 0); - testBanner.showBanner().ignoreErrors(); + await testBanner.showBanner(); + appShell.verifyAll(); }); test('shouldShowBanner must return false when Banner is implicitly disabled by sampling', () => { const enabled = true; @@ -64,6 +69,24 @@ suite('Propose Pylance Banner', () => { 'Explicitly disabled banner shouldShowBanner != false.' ); }); + test('Clicking yes opens the extension marketplace entry', async () => { + appShell + .setup((a) => + a.showInformationMessage( + typemoq.It.isValue(message), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later) + ) + ) + .returns(async () => yes) + .verifiable(typemoq.Times.once()); + appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.once()); + + const testBanner = preparePopup(true, appShell.object, config.object, appEnv.object, 100); + await testBanner.showBanner(); + appShell.verifyAll(); + }); }); function preparePopup( From f7e631a60fa1a63f8a9a8bfc2ea00fe6c48d2ed0 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 15 Jul 2020 19:25:42 -0700 Subject: [PATCH 13/21] Fix localization string --- src/client/common/utils/localize.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 2a8302139528..bdb40e26afb8 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -103,7 +103,7 @@ export namespace AttachProcess { export namespace LanguageService { export const proposePylanceMessage = localize( 'LanguageService.proposePylanceMessage', - 'Try out a new faster, feature-rich language server for Python by Microsoft - Pylance!' + 'Try out a new faster, feature-rich language server for Python by Microsoft, Pylance! Install the extension now.' ); export const tryItNow = localize('LanguageService.tryItNow', 'Try it now'); export const remindMeLater = localize('LanguageService.remindMeLater', 'Remind me later'); From 370d52ba92e3460a94d7bafea927a0b5ae6ef0d6 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 15 Jul 2020 19:30:01 -0700 Subject: [PATCH 14/21] Show banners asynchronously --- src/client/activation/languageServer/activator.ts | 2 +- src/client/activation/none/activator.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/activation/languageServer/activator.ts b/src/client/activation/languageServer/activator.ts index 7b183a951424..cc4bb9be9678 100644 --- a/src/client/activation/languageServer/activator.ts +++ b/src/client/activation/languageServer/activator.ts @@ -39,7 +39,7 @@ export class DotNetLanguageServerActivator extends LanguageServerActivatorBase { public async start(resource: Resource, interpreter?: PythonInterpreter): Promise { if (!isTestExecution()) { - await this.proposePylancePopup.showBanner(); + this.proposePylancePopup.showBanner().ignoreErrors(); } return super.start(resource, interpreter); } diff --git a/src/client/activation/none/activator.ts b/src/client/activation/none/activator.ts index f9cc3f58611b..5266e8ab39c1 100644 --- a/src/client/activation/none/activator.ts +++ b/src/client/activation/none/activator.ts @@ -41,7 +41,7 @@ export class NoLanguageServerExtensionActivator implements ILanguageServerActiva ) {} public async start(_resource: Resource, _interpreter?: PythonInterpreter): Promise { if (!isTestExecution()) { - await this.proposePylancePopup.showBanner(); + this.proposePylancePopup.showBanner().ignoreErrors(); } } // tslint:disable-next-line: no-empty From 2450fc7f06dee2fe40620a7b30de1adf6a0de2a6 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 20 Jul 2020 13:40:13 -0700 Subject: [PATCH 15/21] Add experiments --- src/client/common/experiments/groups.ts | 5 + .../proposeLanguageServerBanner.ts | 64 +++------- ...roposeNewLanguageServerBanner.unit.test.ts | 115 +++++++++++++----- 3 files changed, 104 insertions(+), 80 deletions(-) diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts index 3757a03d54a0..797d4e1d5db5 100644 --- a/src/client/common/experiments/groups.ts +++ b/src/client/common/experiments/groups.ts @@ -99,3 +99,8 @@ export enum RemoveKernelToolbarInInteractiveWindow { export enum EnableTrustedNotebooks { experiment = 'EnableTrustedNotebooks' } + +// Experiment to offer switch to Pylance language server +export enum TryPylance { + experiment = 'tryPylance' +} diff --git a/src/client/languageServices/proposeLanguageServerBanner.ts b/src/client/languageServices/proposeLanguageServerBanner.ts index a1f23880920b..8c4e13e8d8fc 100644 --- a/src/client/languageServices/proposeLanguageServerBanner.ts +++ b/src/client/languageServices/proposeLanguageServerBanner.ts @@ -6,28 +6,25 @@ import { inject, injectable } from 'inversify'; import * as vscode from 'vscode'; import { LanguageServerType } from '../activation/types'; -import { IApplicationEnvironment, IApplicationShell } from '../common/application/types'; +import { IApplicationShell } from '../common/application/types'; import { PYLANCE_EXTENSION_ID } from '../common/constants'; +import { TryPylance } from '../common/experiments/groups'; import '../common/extensions'; -import { IConfigurationService, IPersistentStateFactory, IPythonExtensionBanner } from '../common/types'; +import { + IConfigurationService, + IExperimentService, + IPersistentStateFactory, + IPythonExtensionBanner +} from '../common/types'; import { LanguageService } from '../common/utils/localize'; -import { getRandomBetween } from '../common/utils/random'; export const PylanceExtensionUri = `${vscode.env.uriScheme}:extension/${PYLANCE_EXTENSION_ID}`; // persistent state names, exported to make use of in testing export enum ProposeLSStateKeys { - ShowBanner = 'ProposePylanceBanner' + ShowBanner = 'TryPylanceBanner' } -const frequencyPerServerType = new Map([ - [LanguageServerType.Node, 0], - [LanguageServerType.Microsoft, 50], - [LanguageServerType.None, 50], - // Banner for Jedi users is suppressed until further notice. - [LanguageServerType.Jedi, 0] -]); - /* This class represents a popup that propose that the user try out a new feature of the extension, and optionally enable that new feature if they @@ -37,48 +34,20 @@ function enables the popup for this user. */ @injectable() export class ProposePylanceBanner implements IPythonExtensionBanner { - private initialized?: boolean; private disabledInCurrentSession: boolean = false; - private sampleSizePerHundred = 0; constructor( @inject(IApplicationShell) private appShell: IApplicationShell, @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory, @inject(IConfigurationService) private configuration: IConfigurationService, - @inject(IApplicationEnvironment) private appEnvirontment: IApplicationEnvironment, - sampleSizePerHundred = -1 - ) { - if (this.appEnvirontment.channel === 'insiders') { - this.sampleSizePerHundred = 100; - } else { - if (sampleSizePerHundred >= 0) { - this.sampleSizePerHundred = sampleSizePerHundred; - } else { - const lsType = this.configuration.getSettings().languageServer ?? LanguageServerType.Jedi; - this.sampleSizePerHundred = frequencyPerServerType.get(lsType) ?? 0; - } - } - this.initialize(); - } + @inject(IExperimentService) private experiments: IExperimentService + ) {} - public initialize() { - if (this.initialized) { - return; - } - this.initialized = true; - - // Don't even bother adding handlers if banner has been turned off. - if (!this.enabled) { - return; - } - - const randomSample: number = getRandomBetween(0, 100); - if (randomSample >= this.sampleSizePerHundred) { - this.disable().ignoreErrors(); - return; - } - } public get enabled(): boolean { + const lsType = this.configuration.getSettings().languageServer ?? LanguageServerType.Jedi; + if (lsType === LanguageServerType.Jedi || lsType === LanguageServerType.Node) { + return false; + } return this.persistentState.createGlobalPersistentState(ProposeLSStateKeys.ShowBanner, true).value; } @@ -110,7 +79,8 @@ export class ProposePylanceBanner implements IPythonExtensionBanner { } public async shouldShowBanner(): Promise { - return Promise.resolve(this.enabled && !this.disabledInCurrentSession); + const inExperiment = await this.experiments.inExperiment(TryPylance.experiment); + return inExperiment && this.enabled && !this.disabledInCurrentSession; } public async disable(): Promise { diff --git a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts index ed9e563cd783..2afd5f3aab05 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts @@ -5,8 +5,16 @@ import { expect } from 'chai'; import * as typemoq from 'typemoq'; -import { IApplicationEnvironment, IApplicationShell } from '../../../client/common/application/types'; -import { IConfigurationService, IPersistentState, IPersistentStateFactory } from '../../../client/common/types'; +import { LanguageServerType } from '../../../client/activation/types'; +import { IApplicationShell } from '../../../client/common/application/types'; +import { TryPylance } from '../../../client/common/experiments/groups'; +import { + IConfigurationService, + IExperimentService, + IPersistentState, + IPersistentStateFactory, + IPythonSettings +} from '../../../client/common/types'; import { LanguageService } from '../../../client/common/utils/localize'; import { ProposeLSStateKeys, @@ -14,10 +22,26 @@ import { PylanceExtensionUri } from '../../../client/languageServices/proposeLanguageServerBanner'; +interface IExperimentLsCombination { + inExperiment: boolean; + lsType: LanguageServerType; + shouldShowBanner: boolean; +} +const testData: IExperimentLsCombination[] = [ + { inExperiment: true, lsType: LanguageServerType.None, shouldShowBanner: true }, + { inExperiment: true, lsType: LanguageServerType.Microsoft, shouldShowBanner: true }, + { inExperiment: true, lsType: LanguageServerType.Node, shouldShowBanner: false }, + { inExperiment: true, lsType: LanguageServerType.Jedi, shouldShowBanner: false }, + { inExperiment: false, lsType: LanguageServerType.None, shouldShowBanner: false }, + { inExperiment: false, lsType: LanguageServerType.Microsoft, shouldShowBanner: false }, + { inExperiment: false, lsType: LanguageServerType.Node, shouldShowBanner: false }, + { inExperiment: false, lsType: LanguageServerType.Jedi, shouldShowBanner: false } +]; + suite('Propose Pylance Banner', () => { let config: typemoq.IMock; let appShell: typemoq.IMock; - let appEnv: typemoq.IMock; + let settings: typemoq.IMock; const message = LanguageService.proposePylanceMessage(); const yes = LanguageService.tryItNow(); @@ -26,13 +50,19 @@ suite('Propose Pylance Banner', () => { setup(() => { config = typemoq.Mock.ofType(); + settings = typemoq.Mock.ofType(); + config.setup((x) => x.getSettings(typemoq.It.isAny())).returns(() => settings.object); appShell = typemoq.Mock.ofType(); - appEnv = typemoq.Mock.ofType(); }); - test('Sampling 100/100 enables the banner', () => { - const enabledValue = true; - const testBanner = preparePopup(enabledValue, appShell.object, config.object, appEnv.object, 100); - expect(testBanner.enabled).to.be.equal(true, 'Sampling 100/100 should always enable the banner.'); + testData.forEach((t) => { + test(`${t.inExperiment ? 'In' : 'Not in'} experiment and "python.languageServer": "${t.lsType}" should ${ + t.shouldShowBanner ? 'show' : 'not show' + } banner`, async () => { + settings.setup((x) => x.languageServer).returns(() => t.lsType); + const testBanner = preparePopup(true, appShell.object, config.object, t.inExperiment); + const actual = await testBanner.shouldShowBanner(); + expect(actual).to.be.equal(t.shouldShowBanner, `shouldShowBanner() returned ${actual}`); + }); }); test('Do not show banner when it is disabled', async () => { appShell @@ -45,31 +75,49 @@ suite('Propose Pylance Banner', () => { ) ) .verifiable(typemoq.Times.never()); - const enabled: boolean = true; - const testBanner = preparePopup(enabled, appShell.object, config.object, appEnv.object, 0); + const testBanner = preparePopup(false, appShell.object, config.object, true); await testBanner.showBanner(); - appShell.verifyAll(); }); - test('shouldShowBanner must return false when Banner is implicitly disabled by sampling', () => { - const enabled = true; - const testBanner = preparePopup(enabled, appShell.object, config.object, appEnv.object, 0); - expect(testBanner.enabled).to.be.equal(false, 'We implicitly disabled the banner, it should never show.'); + test('Clicking No shoule disable the banner', async () => { + appShell + .setup((a) => + a.showInformationMessage( + typemoq.It.isValue(message), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later) + ) + ) + .returns(async () => no) + .verifiable(typemoq.Times.once()); + appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.never()); + + const testBanner = preparePopup(true, appShell.object, config.object, true); + await testBanner.showBanner(); + expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled when user clicked No'); }); - test('shouldShowBanner must return false when Banner is explicitly disabled', async () => { - const enabled = true; - const testBanner = preparePopup(enabled, appShell.object, config.object, appEnv.object, 100); + test('Clicking Later shoule disable banner in session', async () => { + appShell + .setup((a) => + a.showInformationMessage( + typemoq.It.isValue(message), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later) + ) + ) + .returns(async () => later) + .verifiable(typemoq.Times.once()); + appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.never()); - expect(await testBanner.shouldShowBanner()).to.be.equal( + const testBanner = preparePopup(true, appShell.object, config.object, true); + await testBanner.showBanner(); + expect(testBanner.enabled).to.be.equal( true, - '100% sample size should always make the banner enabled.' - ); - await testBanner.disable(); - expect(await testBanner.shouldShowBanner()).to.be.equal( - false, - 'Explicitly disabled banner shouldShowBanner != false.' + 'Banner should not be permanently disabled when user clicked Later' ); }); - test('Clicking yes opens the extension marketplace entry', async () => { + test('Clicking Yes opens the extension marketplace entry', async () => { appShell .setup((a) => a.showInformationMessage( @@ -83,9 +131,9 @@ suite('Propose Pylance Banner', () => { .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.once()); - const testBanner = preparePopup(true, appShell.object, config.object, appEnv.object, 100); + const testBanner = preparePopup(true, appShell.object, config.object, true); await testBanner.showBanner(); - appShell.verifyAll(); + expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled after opening store URL'); }); }); @@ -93,11 +141,11 @@ function preparePopup( enabledValue: boolean, appShell: IApplicationShell, config: IConfigurationService, - appEnv: IApplicationEnvironment, - sampleSizePerHundred: number + inExperiment: boolean ): ProposePylanceBanner { - const myfactory: typemoq.IMock = typemoq.Mock.ofType(); - const val: typemoq.IMock> = typemoq.Mock.ofType>(); + const myfactory = typemoq.Mock.ofType(); + const val = typemoq.Mock.ofType>(); + const experiments = typemoq.Mock.ofType(); val.setup((a) => a.updateValue(typemoq.It.isValue(true))).returns(() => { enabledValue = true; return Promise.resolve(); @@ -123,5 +171,6 @@ function preparePopup( .returns(() => { return val.object; }); - return new ProposePylanceBanner(appShell, myfactory.object, config, appEnv, sampleSizePerHundred); + experiments.setup((x) => x.inExperiment(TryPylance.experiment)).returns(() => Promise.resolve(inExperiment)); + return new ProposePylanceBanner(appShell, myfactory.object, config, experiments.object); } From 0300726b7e7df70ef653c23be498953699e06c7f Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 20 Jul 2020 13:40:39 -0700 Subject: [PATCH 16/21] Formatting --- .../testing/banners/proposeNewLanguageServerBanner.unit.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts index 2afd5f3aab05..232f3574ba87 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts @@ -55,7 +55,7 @@ suite('Propose Pylance Banner', () => { appShell = typemoq.Mock.ofType(); }); testData.forEach((t) => { - test(`${t.inExperiment ? 'In' : 'Not in'} experiment and "python.languageServer": "${t.lsType}" should ${ + test(`${t.inExperiment ? 'In' : 'Not in'} experiment and "python.languageServer": "${t.lsType}" should ${ t.shouldShowBanner ? 'show' : 'not show' } banner`, async () => { settings.setup((x) => x.languageServer).returns(() => t.lsType); From f0d528c1ad67f5f3a01a80c6bd02e12dd8dab17a Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 20 Jul 2020 13:41:42 -0700 Subject: [PATCH 17/21] Typo --- .../banners/proposeNewLanguageServerBanner.unit.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts index 232f3574ba87..82cb4a04c9ba 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts @@ -78,7 +78,7 @@ suite('Propose Pylance Banner', () => { const testBanner = preparePopup(false, appShell.object, config.object, true); await testBanner.showBanner(); }); - test('Clicking No shoule disable the banner', async () => { + test('Clicking No should disable the banner', async () => { appShell .setup((a) => a.showInformationMessage( @@ -96,7 +96,7 @@ suite('Propose Pylance Banner', () => { await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled when user clicked No'); }); - test('Clicking Later shoule disable banner in session', async () => { + test('Clicking Later should disable banner in session', async () => { appShell .setup((a) => a.showInformationMessage( From 57de3f8ac6d3de6097770a1554fa4a692e6e972f Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 20 Jul 2020 16:08:52 -0700 Subject: [PATCH 18/21] Put back verifyAll --- .../banners/proposeNewLanguageServerBanner.unit.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts index 82cb4a04c9ba..d5afc33009d8 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts @@ -77,6 +77,7 @@ suite('Propose Pylance Banner', () => { .verifiable(typemoq.Times.never()); const testBanner = preparePopup(false, appShell.object, config.object, true); await testBanner.showBanner(); + appShell.verifyAll(); }); test('Clicking No should disable the banner', async () => { appShell @@ -95,6 +96,7 @@ suite('Propose Pylance Banner', () => { const testBanner = preparePopup(true, appShell.object, config.object, true); await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled when user clicked No'); + appShell.verifyAll(); }); test('Clicking Later should disable banner in session', async () => { appShell @@ -116,6 +118,7 @@ suite('Propose Pylance Banner', () => { true, 'Banner should not be permanently disabled when user clicked Later' ); + appShell.verifyAll(); }); test('Clicking Yes opens the extension marketplace entry', async () => { appShell @@ -134,6 +137,7 @@ suite('Propose Pylance Banner', () => { const testBanner = preparePopup(true, appShell.object, config.object, true); await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled after opening store URL'); + appShell.verifyAll(); }); }); From b3c85fc398f4eb6ddfcdced71eea997c51810188 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 20 Jul 2020 17:36:26 -0700 Subject: [PATCH 19/21] Remove obsolete experiments, add Pylance --- package.json | 4 ++-- src/client/common/experiments/groups.ts | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index e90ef37bca4b..933ae269905e 100644 --- a/package.json +++ b/package.json @@ -1827,7 +1827,6 @@ "default": [], "items": { "enum": [ - "LS - enabled", "AlwaysDisplayTestExplorer - experiment", "ShowExtensionSurveyPrompt - enabled", "Reload - experiment", @@ -1841,6 +1840,7 @@ "DeprecatePythonPath - experiment", "RunByLine - experiment", "EnableTrustedNotebooks", + "tryPylance", "All" ] }, @@ -1852,7 +1852,6 @@ "default": [], "items": { "enum": [ - "LS - enabled", "AlwaysDisplayTestExplorer - experiment", "ShowExtensionSurveyPrompt - enabled", "Reload - experiment", @@ -1866,6 +1865,7 @@ "DeprecatePythonPath - experiment", "RunByLine - experiment", "EnableTrustedNotebooks", + "tryPylance", "All" ] }, diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts index 797d4e1d5db5..6daf346c23f8 100644 --- a/src/client/common/experiments/groups.ts +++ b/src/client/common/experiments/groups.ts @@ -1,6 +1,3 @@ -export const LSControl = 'LS - control'; -export const LSEnabled = 'LS - enabled'; - // Experiment to check whether to always display the test explorer. export enum AlwaysDisplayTestExplorerGroups { control = 'AlwaysDisplayTestExplorer - control', From 1ca6b119f12fc390d0fa6a4f73105fd78f627f3c Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Tue, 21 Jul 2020 11:16:34 -0700 Subject: [PATCH 20/21] Suppress experiment if Pylance is installed --- experiments.json | 12 ------- .../proposeLanguageServerBanner.ts | 9 ++++- ...roposeNewLanguageServerBanner.unit.test.ts | 36 ++++++++++++++----- 3 files changed, 36 insertions(+), 21 deletions(-) diff --git a/experiments.json b/experiments.json index 0b4daf3c9ed6..322b3a4d30a5 100644 --- a/experiments.json +++ b/experiments.json @@ -65,18 +65,6 @@ "min": 0, "max": 0 }, - { - "name": "LS - enabled", - "salt": "LS", - "min": 0, - "max": 4 - }, - { - "name": "LS - control", - "salt": "LS", - "min": 20, - "max": 24 - }, { "name": "UseTerminalToGetActivatedEnvVars - experiment", "salt": "UseTerminalToGetActivatedEnvVars", diff --git a/src/client/languageServices/proposeLanguageServerBanner.ts b/src/client/languageServices/proposeLanguageServerBanner.ts index 8c4e13e8d8fc..a8c108dc8b33 100644 --- a/src/client/languageServices/proposeLanguageServerBanner.ts +++ b/src/client/languageServices/proposeLanguageServerBanner.ts @@ -13,6 +13,7 @@ import '../common/extensions'; import { IConfigurationService, IExperimentService, + IExtensions, IPersistentStateFactory, IPythonExtensionBanner } from '../common/types'; @@ -40,7 +41,8 @@ export class ProposePylanceBanner implements IPythonExtensionBanner { @inject(IApplicationShell) private appShell: IApplicationShell, @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory, @inject(IConfigurationService) private configuration: IConfigurationService, - @inject(IExperimentService) private experiments: IExperimentService + @inject(IExperimentService) private experiments: IExperimentService, + @inject(IExtensions) readonly extensions: IExtensions ) {} public get enabled(): boolean { @@ -79,6 +81,11 @@ export class ProposePylanceBanner implements IPythonExtensionBanner { } public async shouldShowBanner(): Promise { + // Do not prompt if Pylance is already installed. + if (this.extensions.getExtension(PYLANCE_EXTENSION_ID)) { + return false; + } + // Only prompt for users in experiment. const inExperiment = await this.experiments.inExperiment(TryPylance.experiment); return inExperiment && this.enabled && !this.disabledInCurrentSession; } diff --git a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts index d5afc33009d8..b622906754c1 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts @@ -5,12 +5,15 @@ import { expect } from 'chai'; import * as typemoq from 'typemoq'; +import { Extension } from 'vscode'; import { LanguageServerType } from '../../../client/activation/types'; import { IApplicationShell } from '../../../client/common/application/types'; +import { PYLANCE_EXTENSION_ID } from '../../../client/common/constants'; import { TryPylance } from '../../../client/common/experiments/groups'; import { IConfigurationService, IExperimentService, + IExtensions, IPersistentState, IPersistentStateFactory, IPythonSettings @@ -59,11 +62,19 @@ suite('Propose Pylance Banner', () => { t.shouldShowBanner ? 'show' : 'not show' } banner`, async () => { settings.setup((x) => x.languageServer).returns(() => t.lsType); - const testBanner = preparePopup(true, appShell.object, config.object, t.inExperiment); + const testBanner = preparePopup(true, appShell.object, config.object, t.inExperiment, false); const actual = await testBanner.shouldShowBanner(); expect(actual).to.be.equal(t.shouldShowBanner, `shouldShowBanner() returned ${actual}`); }); }); + testData.forEach((t) => { + test(`When Pylance is installed, banner should not be shown when "python.languageServer": "${t.lsType}"`, async () => { + settings.setup((x) => x.languageServer).returns(() => t.lsType); + const testBanner = preparePopup(true, appShell.object, config.object, t.inExperiment, true); + const actual = await testBanner.shouldShowBanner(); + expect(actual).to.be.equal(false, `shouldShowBanner() returned ${actual}`); + }); + }); test('Do not show banner when it is disabled', async () => { appShell .setup((a) => @@ -75,7 +86,7 @@ suite('Propose Pylance Banner', () => { ) ) .verifiable(typemoq.Times.never()); - const testBanner = preparePopup(false, appShell.object, config.object, true); + const testBanner = preparePopup(false, appShell.object, config.object, true, false); await testBanner.showBanner(); appShell.verifyAll(); }); @@ -93,7 +104,7 @@ suite('Propose Pylance Banner', () => { .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.never()); - const testBanner = preparePopup(true, appShell.object, config.object, true); + const testBanner = preparePopup(true, appShell.object, config.object, true, false); await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled when user clicked No'); appShell.verifyAll(); @@ -112,7 +123,7 @@ suite('Propose Pylance Banner', () => { .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.never()); - const testBanner = preparePopup(true, appShell.object, config.object, true); + const testBanner = preparePopup(true, appShell.object, config.object, true, false); await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal( true, @@ -134,7 +145,7 @@ suite('Propose Pylance Banner', () => { .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.once()); - const testBanner = preparePopup(true, appShell.object, config.object, true); + const testBanner = preparePopup(true, appShell.object, config.object, true, false); await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled after opening store URL'); appShell.verifyAll(); @@ -145,11 +156,11 @@ function preparePopup( enabledValue: boolean, appShell: IApplicationShell, config: IConfigurationService, - inExperiment: boolean + inExperiment: boolean, + pylanceInstalled: boolean ): ProposePylanceBanner { const myfactory = typemoq.Mock.ofType(); const val = typemoq.Mock.ofType>(); - const experiments = typemoq.Mock.ofType(); val.setup((a) => a.updateValue(typemoq.It.isValue(true))).returns(() => { enabledValue = true; return Promise.resolve(); @@ -175,6 +186,15 @@ function preparePopup( .returns(() => { return val.object; }); + + const experiments = typemoq.Mock.ofType(); experiments.setup((x) => x.inExperiment(TryPylance.experiment)).returns(() => Promise.resolve(inExperiment)); - return new ProposePylanceBanner(appShell, myfactory.object, config, experiments.object); + + const extensions = typemoq.Mock.ofType(); + // tslint:disable-next-line: no-any + const extension = typemoq.Mock.ofType>(); + extensions + .setup((x) => x.getExtension(PYLANCE_EXTENSION_ID)) + .returns(() => (pylanceInstalled ? extension.object : undefined)); + return new ProposePylanceBanner(appShell, myfactory.object, config, experiments.object, extensions.object); } From c1de88a99c8ef72646119d7a7a4f23b42a008a66 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Tue, 21 Jul 2020 14:51:05 -0700 Subject: [PATCH 21/21] PR feedback --- .../application/applicationEnvironment.ts | 3 ++ src/client/common/application/types.ts | 4 +++ src/client/common/constants.ts | 3 +- .../proposeLanguageServerBanner.ts | 10 ++++--- ...roposeNewLanguageServerBanner.unit.test.ts | 30 +++++++++++-------- 5 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/client/common/application/applicationEnvironment.ts b/src/client/common/application/applicationEnvironment.ts index a16082e0b848..3d01d85f4b35 100644 --- a/src/client/common/application/applicationEnvironment.ts +++ b/src/client/common/application/applicationEnvironment.ts @@ -88,4 +88,7 @@ export class ApplicationEnvironment implements IApplicationEnvironment { const version = parse(this.packageJson.version); return !version || version.prerelease.length > 0 ? 'insiders' : 'stable'; } + public get uriScheme(): string { + return vscode.env.uriScheme; + } } diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index f1d7f38900c6..2459ca1aded7 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -1024,6 +1024,10 @@ export interface IApplicationEnvironment { * The version of the editor. */ readonly vscodeVersion: string; + /** + * The custom uri scheme the editor registers to in the operating system. + */ + readonly uriScheme: string; } export const IWebPanelMessageListener = Symbol('IWebPanelMessageListener'); diff --git a/src/client/common/constants.ts b/src/client/common/constants.ts index 7abe0634ac2e..6096285fa170 100644 --- a/src/client/common/constants.ts +++ b/src/client/common/constants.ts @@ -15,6 +15,7 @@ export const PYTHON_ALLFILES = [{ language: PYTHON_LANGUAGE }]; export const PVSC_EXTENSION_ID = 'ms-python.python'; export const CODE_RUNNER_EXTENSION_ID = 'formulahendry.code-runner'; +export const PYLANCE_EXTENSION_ID = 'ms-python.vscode-pylance'; export const AppinsightsKey = 'AIF-d9b70cd4-b9f9-4d70-929b-a071c400b217'; export namespace Commands { @@ -114,6 +115,4 @@ export const UseCustomEditorApi = Symbol('USE_CUSTOM_EDITOR'); export const UseVSCodeNotebookEditorApi = Symbol('USE_NATIVEEDITOR'); export const UseProposedApi = Symbol('USE_VSC_PROPOSED_API'); -export const PYLANCE_EXTENSION_ID = 'ms-python.vscode-pylance'; - export * from '../constants'; diff --git a/src/client/languageServices/proposeLanguageServerBanner.ts b/src/client/languageServices/proposeLanguageServerBanner.ts index a8c108dc8b33..dfc792c48e47 100644 --- a/src/client/languageServices/proposeLanguageServerBanner.ts +++ b/src/client/languageServices/proposeLanguageServerBanner.ts @@ -4,9 +4,8 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import * as vscode from 'vscode'; import { LanguageServerType } from '../activation/types'; -import { IApplicationShell } from '../common/application/types'; +import { IApplicationEnvironment, IApplicationShell } from '../common/application/types'; import { PYLANCE_EXTENSION_ID } from '../common/constants'; import { TryPylance } from '../common/experiments/groups'; import '../common/extensions'; @@ -19,7 +18,9 @@ import { } from '../common/types'; import { LanguageService } from '../common/utils/localize'; -export const PylanceExtensionUri = `${vscode.env.uriScheme}:extension/${PYLANCE_EXTENSION_ID}`; +export function getPylanceExtensionUri(appEnv: IApplicationEnvironment): string { + return `${appEnv.uriScheme}:extension/${PYLANCE_EXTENSION_ID}`; +} // persistent state names, exported to make use of in testing export enum ProposeLSStateKeys { @@ -39,6 +40,7 @@ export class ProposePylanceBanner implements IPythonExtensionBanner { constructor( @inject(IApplicationShell) private appShell: IApplicationShell, + @inject(IApplicationEnvironment) private appEnv: IApplicationEnvironment, @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory, @inject(IConfigurationService) private configuration: IConfigurationService, @inject(IExperimentService) private experiments: IExperimentService, @@ -71,7 +73,7 @@ export class ProposePylanceBanner implements IPythonExtensionBanner { ); if (response === LanguageService.tryItNow()) { - this.appShell.openUrl(PylanceExtensionUri); + this.appShell.openUrl(getPylanceExtensionUri(this.appEnv)); await this.disable(); } else if (response === LanguageService.bannerLabelNo()) { await this.disable(); diff --git a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts index b622906754c1..db4911814901 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts @@ -7,7 +7,7 @@ import { expect } from 'chai'; import * as typemoq from 'typemoq'; import { Extension } from 'vscode'; import { LanguageServerType } from '../../../client/activation/types'; -import { IApplicationShell } from '../../../client/common/application/types'; +import { IApplicationEnvironment, IApplicationShell } from '../../../client/common/application/types'; import { PYLANCE_EXTENSION_ID } from '../../../client/common/constants'; import { TryPylance } from '../../../client/common/experiments/groups'; import { @@ -20,9 +20,9 @@ import { } from '../../../client/common/types'; import { LanguageService } from '../../../client/common/utils/localize'; import { + getPylanceExtensionUri, ProposeLSStateKeys, - ProposePylanceBanner, - PylanceExtensionUri + ProposePylanceBanner } from '../../../client/languageServices/proposeLanguageServerBanner'; interface IExperimentLsCombination { @@ -44,6 +44,7 @@ const testData: IExperimentLsCombination[] = [ suite('Propose Pylance Banner', () => { let config: typemoq.IMock; let appShell: typemoq.IMock; + let appEnv: typemoq.IMock; let settings: typemoq.IMock; const message = LanguageService.proposePylanceMessage(); @@ -56,13 +57,15 @@ suite('Propose Pylance Banner', () => { settings = typemoq.Mock.ofType(); config.setup((x) => x.getSettings(typemoq.It.isAny())).returns(() => settings.object); appShell = typemoq.Mock.ofType(); + appEnv = typemoq.Mock.ofType(); + appEnv.setup((x) => x.uriScheme).returns(() => 'scheme'); }); testData.forEach((t) => { test(`${t.inExperiment ? 'In' : 'Not in'} experiment and "python.languageServer": "${t.lsType}" should ${ t.shouldShowBanner ? 'show' : 'not show' } banner`, async () => { settings.setup((x) => x.languageServer).returns(() => t.lsType); - const testBanner = preparePopup(true, appShell.object, config.object, t.inExperiment, false); + const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, t.inExperiment, false); const actual = await testBanner.shouldShowBanner(); expect(actual).to.be.equal(t.shouldShowBanner, `shouldShowBanner() returned ${actual}`); }); @@ -70,7 +73,7 @@ suite('Propose Pylance Banner', () => { testData.forEach((t) => { test(`When Pylance is installed, banner should not be shown when "python.languageServer": "${t.lsType}"`, async () => { settings.setup((x) => x.languageServer).returns(() => t.lsType); - const testBanner = preparePopup(true, appShell.object, config.object, t.inExperiment, true); + const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, t.inExperiment, true); const actual = await testBanner.shouldShowBanner(); expect(actual).to.be.equal(false, `shouldShowBanner() returned ${actual}`); }); @@ -86,7 +89,7 @@ suite('Propose Pylance Banner', () => { ) ) .verifiable(typemoq.Times.never()); - const testBanner = preparePopup(false, appShell.object, config.object, true, false); + const testBanner = preparePopup(false, appShell.object, appEnv.object, config.object, true, false); await testBanner.showBanner(); appShell.verifyAll(); }); @@ -102,9 +105,9 @@ suite('Propose Pylance Banner', () => { ) .returns(async () => no) .verifiable(typemoq.Times.once()); - appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.never()); + appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never()); - const testBanner = preparePopup(true, appShell.object, config.object, true, false); + const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, true, false); await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled when user clicked No'); appShell.verifyAll(); @@ -121,9 +124,9 @@ suite('Propose Pylance Banner', () => { ) .returns(async () => later) .verifiable(typemoq.Times.once()); - appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.never()); + appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never()); - const testBanner = preparePopup(true, appShell.object, config.object, true, false); + const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, true, false); await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal( true, @@ -143,9 +146,9 @@ suite('Propose Pylance Banner', () => { ) .returns(async () => yes) .verifiable(typemoq.Times.once()); - appShell.setup((a) => a.openUrl(PylanceExtensionUri)).verifiable(typemoq.Times.once()); + appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.once()); - const testBanner = preparePopup(true, appShell.object, config.object, true, false); + const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, true, false); await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled after opening store URL'); appShell.verifyAll(); @@ -155,6 +158,7 @@ suite('Propose Pylance Banner', () => { function preparePopup( enabledValue: boolean, appShell: IApplicationShell, + appEnv: IApplicationEnvironment, config: IConfigurationService, inExperiment: boolean, pylanceInstalled: boolean @@ -196,5 +200,5 @@ function preparePopup( extensions .setup((x) => x.getExtension(PYLANCE_EXTENSION_ID)) .returns(() => (pylanceInstalled ? extension.object : undefined)); - return new ProposePylanceBanner(appShell, myfactory.object, config, experiments.object, extensions.object); + return new ProposePylanceBanner(appShell, appEnv, myfactory.object, config, experiments.object, extensions.object); }