From 10d245eeb1f4a6ff241f6c8c029791e34db61a9e Mon Sep 17 00:00:00 2001 From: Bill Schnurr Date: Wed, 11 Nov 2020 11:43:21 -0800 Subject: [PATCH 1/4] wip new jedi tryPylance banner --- pvsc.code-workspace | 2 +- .../activation/jedi/multiplexingActivator.ts | 59 ++++++--- src/client/activation/serviceRegistry.ts | 9 +- src/client/common/experiments/groups.ts | 4 +- src/client/common/types.ts | 1 + .../proposeLanguageServerBannerJedi.ts | 116 ++++++++++++++++++ .../activation/serviceRegistry.unit.test.ts | 14 ++- 7 files changed, 186 insertions(+), 19 deletions(-) create mode 100644 src/client/languageServices/proposeLanguageServerBannerJedi.ts diff --git a/pvsc.code-workspace b/pvsc.code-workspace index 644c7b74cff2..dbdd7821724d 100644 --- a/pvsc.code-workspace +++ b/pvsc.code-workspace @@ -17,6 +17,6 @@ "**/.vscode-test/insider/**": true, "**/.vscode-test/stable/**": true, "**/out/**": true - } + }, } } diff --git a/src/client/activation/jedi/multiplexingActivator.ts b/src/client/activation/jedi/multiplexingActivator.ts index 70f5d664fc62..abcb8e703e39 100644 --- a/src/client/activation/jedi/multiplexingActivator.ts +++ b/src/client/activation/jedi/multiplexingActivator.ts @@ -1,6 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { inject, injectable } from 'inversify'; +import { inject, injectable, named } from 'inversify'; import { CancellationToken, CompletionContext, @@ -9,13 +9,20 @@ import { Position, ReferenceContext, SignatureHelpContext, - TextDocument + TextDocument, } from 'vscode'; // tslint:disable-next-line: import-name import { IWorkspaceService } from '../../common/application/types'; +import { isTestExecution } from '../../common/constants'; import { JediLSP } from '../../common/experiments/groups'; import { IFileSystem } from '../../common/platform/types'; -import { IConfigurationService, IExperimentService, Resource } from '../../common/types'; +import { + BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS, + IConfigurationService, + IExperimentService, + IPythonExtensionBanner, + Resource, +} from '../../common/types'; import { IServiceManager } from '../../ioc/types'; import { PythonEnvironment } from '../../pythonEnvironments/info'; import { JediExtensionActivator } from '../jedi'; @@ -33,41 +40,53 @@ import { JediLanguageServerActivator } from './activator'; @injectable() export class MultiplexingJediLanguageServerActivator implements ILanguageServerActivator { private realLanguageServerPromise: Promise; + private realLanguageServer: ILanguageServerActivator | undefined; + private onDidChangeCodeLensesEmitter = new EventEmitter(); constructor( @inject(IServiceManager) private readonly manager: IServiceManager, - @inject(IExperimentService) experimentService: IExperimentService + @inject(IExperimentService) experimentService: IExperimentService, + @inject(IPythonExtensionBanner) + @named(BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS) + private proposePylancePopup: IPythonExtensionBanner, ) { // Check experiment service to see if using new Jedi LSP protocol this.realLanguageServerPromise = experimentService.inExperiment(JediLSP.experiment).then((inExperiment) => { + // Pick how to launch jedi based on if in the experiment or not. this.realLanguageServer = !inExperiment - ? // Pick how to launch jedi based on if in the experiment or not. - new JediExtensionActivator(this.manager) + ? new JediExtensionActivator(this.manager) : new JediLanguageServerActivator( - this.manager.get(ILanguageServerManager), - this.manager.get(IWorkspaceService), - this.manager.get(IFileSystem), - this.manager.get(IConfigurationService) - ); + this.manager.get(ILanguageServerManager), + this.manager.get(IWorkspaceService), + this.manager.get(IFileSystem), + this.manager.get(IConfigurationService), + ); return this.realLanguageServer; }); } + public async start(resource: Resource, interpreter: PythonEnvironment | undefined): Promise { const realServer = await this.realLanguageServerPromise; + if (!isTestExecution()) { + this.proposePylancePopup.showBanner().ignoreErrors(); + } return realServer.start(resource, interpreter); } + public activate(): void { if (this.realLanguageServer) { this.realLanguageServer.activate(); } } + public deactivate(): void { if (this.realLanguageServer) { this.realLanguageServer.deactivate(); } } + public get onDidChangeCodeLenses(): Event { return this.onDidChangeCodeLensesEmitter.event; } @@ -76,66 +95,76 @@ export class MultiplexingJediLanguageServerActivator implements ILanguageServerA if (this.realLanguageServer) { return this.realLanguageServer.connection; } + return undefined; } public get capabilities() { if (this.realLanguageServer) { return this.realLanguageServer.capabilities; } + return undefined; } public async provideRenameEdits( document: TextDocument, position: Position, newName: string, - token: CancellationToken + token: CancellationToken, ) { const server = await this.realLanguageServerPromise; return server.provideRenameEdits(document, position, newName, token); } + public async provideDefinition(document: TextDocument, position: Position, token: CancellationToken) { const server = await this.realLanguageServerPromise; return server.provideDefinition(document, position, token); } + public async provideHover(document: TextDocument, position: Position, token: CancellationToken) { const server = await this.realLanguageServerPromise; return server.provideHover(document, position, token); } + public async provideReferences( document: TextDocument, position: Position, context: ReferenceContext, - token: CancellationToken + token: CancellationToken, ) { const server = await this.realLanguageServerPromise; return server.provideReferences(document, position, context, token); } + public async provideCompletionItems( document: TextDocument, position: Position, token: CancellationToken, - context: CompletionContext + context: CompletionContext, ) { const server = await this.realLanguageServerPromise; return server.provideCompletionItems(document, position, token, context); } + public async provideCodeLenses(document: TextDocument, token: CancellationToken) { const server = await this.realLanguageServerPromise; return server.provideCodeLenses(document, token); } + public async provideDocumentSymbols(document: TextDocument, token: CancellationToken) { const server = await this.realLanguageServerPromise; return server.provideDocumentSymbols(document, token); } + public async provideSignatureHelp( document: TextDocument, position: Position, token: CancellationToken, - context: SignatureHelpContext + context: SignatureHelpContext, ) { const server = await this.realLanguageServerPromise; return server.provideSignatureHelp(document, position, token, context); } + public dispose(): void { if (this.realLanguageServer) { this.realLanguageServer.dispose(); diff --git a/src/client/activation/serviceRegistry.ts b/src/client/activation/serviceRegistry.ts index 34350ba3e5f3..e31c618c4c68 100644 --- a/src/client/activation/serviceRegistry.ts +++ b/src/client/activation/serviceRegistry.ts @@ -3,9 +3,10 @@ import { registerTypes as registerDotNetTypes } from '../common/dotnet/serviceRegistry'; import { INugetRepository } from '../common/nuget/types'; -import { BANNER_NAME_PROPOSE_LS, IPythonExtensionBanner } from '../common/types'; +import { BANNER_NAME_PROPOSE_LS, BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS, IPythonExtensionBanner } from '../common/types'; import { IServiceManager } from '../ioc/types'; import { ProposePylanceBanner } from '../languageServices/proposeLanguageServerBanner'; +import { ProposePylanceBannerJedi } from '../languageServices/proposeLanguageServerBannerJedi'; import { AATesting } from './aaTesting'; import { ExtensionActivationManager } from './activationManager'; import { LanguageServerExtensionActivationService } from './activationService'; @@ -81,6 +82,12 @@ export function registerTypes(serviceManager: IServiceManager, languageServerTyp BANNER_NAME_PROPOSE_LS ); + serviceManager.addSingleton( + IPythonExtensionBanner, + ProposePylanceBannerJedi, + BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS + ); + if (languageServerType === LanguageServerType.Microsoft) { serviceManager.add( ILanguageServerAnalysisOptions, diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts index ca824afc08d8..10ddbeca46ea 100644 --- a/src/client/common/experiments/groups.ts +++ b/src/client/common/experiments/groups.ts @@ -55,7 +55,9 @@ export enum DeprecatePythonPath { // Experiment to offer switch to Pylance language server export enum TryPylance { - experiment = 'tryPylance' + experiment = 'tryPylance', + jediPrompt1 = 'tryPylancePromptText1', + jediPrompt2 = 'tryPylancePromptText2' } // Experiment for the content of the tip being displayed on first extension launch: diff --git a/src/client/common/types.ts b/src/client/common/types.ts index bedaf39e64ff..1ec1a772dae8 100644 --- a/src/client/common/types.ts +++ b/src/client/common/types.ts @@ -462,6 +462,7 @@ export interface IPythonExtensionBanner { showBanner(): Promise; } export const BANNER_NAME_PROPOSE_LS: string = 'ProposePylance'; +export const BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS: string = 'ProposePylanceJedi'; export type DeprecatedSettingAndValue = { setting: string; diff --git a/src/client/languageServices/proposeLanguageServerBannerJedi.ts b/src/client/languageServices/proposeLanguageServerBannerJedi.ts new file mode 100644 index 000000000000..5c8a494d77b0 --- /dev/null +++ b/src/client/languageServices/proposeLanguageServerBannerJedi.ts @@ -0,0 +1,116 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; +import { LanguageServerType } from '../activation/types'; +import { IApplicationEnvironment, IApplicationShell } from '../common/application/types'; +import { PYLANCE_EXTENSION_ID } from '../common/constants'; +import { TryPylance } from '../common/experiments/groups'; +import '../common/extensions'; +import { + IConfigurationService, + IExperimentService, + IExtensions, + IPersistentStateFactory, + // tslint:disable-next-line: prettier + IPythonExtensionBanner, +} from '../common/types'; +import { Common, Pylance } from '../common/utils/localize'; +import { sendTelemetryEvent } from '../telemetry'; +import { EventName } from '../telemetry/constants'; + +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 { + ShowBanner = 'TryPylanceBannerJedi' +} + +/* +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 +choose to do so. It is meant to be shown only to a subset of our users, +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 ProposePylanceBannerJedi implements IPythonExtensionBanner { + private disabledInCurrentSession = false; + + 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, + // eslint-disable-next-line comma-dangle + @inject(IExtensions) readonly extensions: IExtensions + ) {} + + public get enabled(): boolean { + const lsType = this.configuration.getSettings().languageServer ?? LanguageServerType.Jedi; + if (lsType !== LanguageServerType.Jedi) { + return false; + } + return this.persistentState.createGlobalPersistentState(ProposeLSStateKeys.ShowBanner, true).value; + } + + public async showBanner(): Promise { + if (!this.enabled) { + return; + } + + const show = await this.shouldShowBanner(); + if (!show) { + return; + } + + let promptContent: string | undefined; + if (await this.experiments.inExperiment(TryPylance.jediPrompt1)) { + promptContent = await this.experiments.getExperimentValue(TryPylance.jediPrompt1); + } else if (await this.experiments.inExperiment(TryPylance.jediPrompt2)) { + promptContent = await this.experiments.getExperimentValue(TryPylance.jediPrompt2); + } + + if (promptContent === undefined) { + return; + } + + const response = await this.appShell.showInformationMessage( + promptContent, + Pylance.tryItNow(), + Common.bannerLabelNo(), + Pylance.remindMeLater(), + ); + + let userAction: string; + if (response === Pylance.tryItNow()) { + this.appShell.openUrl(getPylanceExtensionUri(this.appEnv)); + userAction = 'yes'; + await this.disable(); + } else if (response === Common.bannerLabelNo()) { + await this.disable(); + userAction = 'no'; + } else { + this.disabledInCurrentSession = true; + userAction = 'later'; + } + sendTelemetryEvent(EventName.LANGUAGE_SERVER_TRY_PYLANCE, undefined, { userAction }); + } + + public async shouldShowBanner(): Promise { + // Do not prompt if Pylance is already installed. + if (this.extensions.getExtension(PYLANCE_EXTENSION_ID)) { + return false; + } + return this.enabled && !this.disabledInCurrentSession; + } + + public async disable(): Promise { + await this.persistentState + .createGlobalPersistentState(ProposeLSStateKeys.ShowBanner, false) + .updateValue(false); + } +} diff --git a/src/test/activation/serviceRegistry.unit.test.ts b/src/test/activation/serviceRegistry.unit.test.ts index bc02b3da6abc..ae3d3ef023f0 100644 --- a/src/test/activation/serviceRegistry.unit.test.ts +++ b/src/test/activation/serviceRegistry.unit.test.ts @@ -46,10 +46,15 @@ import { LanguageServerType } from '../../client/activation/types'; import { INugetRepository } from '../../client/common/nuget/types'; -import { BANNER_NAME_PROPOSE_LS, IPythonExtensionBanner } from '../../client/common/types'; +import { + BANNER_NAME_PROPOSE_LS, + BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS, + IPythonExtensionBanner +} from '../../client/common/types'; import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceManager } from '../../client/ioc/types'; import { ProposePylanceBanner } from '../../client/languageServices/proposeLanguageServerBanner'; +import { ProposePylanceBannerJedi } from '../../client/languageServices/proposeLanguageServerBannerJedi'; // tslint:disable:max-func-body-length @@ -90,6 +95,13 @@ suite('Unit Tests - Language Server Activation Service Registry', () => { BANNER_NAME_PROPOSE_LS ) ).once(); + verify( + serviceManager.addSingleton( + IPythonExtensionBanner, + ProposePylanceBannerJedi, + BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS + ) + ).once(); verify( serviceManager.addSingleton( ILanguageServerFolderService, From 8c6b70f3bd7137a964fd3929dd1c691647fb0e67 Mon Sep 17 00:00:00 2001 From: Bill Schnurr Date: Thu, 12 Nov 2020 10:13:05 -0800 Subject: [PATCH 2/4] test updates --- .../proposeLanguageServerBanner.ts | 3 +- .../proposeLanguageServerBannerJedi.ts | 11 +- src/client/telemetry/constants.ts | 4 +- src/client/telemetry/index.ts | 1 + ...seNewLanguageServerBannerJedi.unit.test.ts | 292 ++++++++++++++++++ 5 files changed, 305 insertions(+), 6 deletions(-) create mode 100644 src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts diff --git a/src/client/languageServices/proposeLanguageServerBanner.ts b/src/client/languageServices/proposeLanguageServerBanner.ts index 59096edc2271..e207d622c96c 100644 --- a/src/client/languageServices/proposeLanguageServerBanner.ts +++ b/src/client/languageServices/proposeLanguageServerBanner.ts @@ -86,7 +86,8 @@ export class ProposePylanceBanner implements IPythonExtensionBanner { this.disabledInCurrentSession = true; userAction = 'later'; } - sendTelemetryEvent(EventName.LANGUAGE_SERVER_TRY_PYLANCE, undefined, { userAction }); + const experimentName = TryPylance.experiment; + sendTelemetryEvent(EventName.LANGUAGE_SERVER_TRY_PYLANCE, undefined, { userAction, experimentName }); } public async shouldShowBanner(): Promise { diff --git a/src/client/languageServices/proposeLanguageServerBannerJedi.ts b/src/client/languageServices/proposeLanguageServerBannerJedi.ts index 5c8a494d77b0..65b8a5ab1c3c 100644 --- a/src/client/languageServices/proposeLanguageServerBannerJedi.ts +++ b/src/client/languageServices/proposeLanguageServerBannerJedi.ts @@ -25,7 +25,7 @@ export function getPylanceExtensionUri(appEnv: IApplicationEnvironment): string // persistent state names, exported to make use of in testing export enum ProposeLSStateKeys { - ShowBanner = 'TryPylanceBannerJedi' + ShowBannerJedi = 'TryPylanceBannerJedi' } /* @@ -54,7 +54,7 @@ export class ProposePylanceBannerJedi implements IPythonExtensionBanner { if (lsType !== LanguageServerType.Jedi) { return false; } - return this.persistentState.createGlobalPersistentState(ProposeLSStateKeys.ShowBanner, true).value; + return this.persistentState.createGlobalPersistentState(ProposeLSStateKeys.ShowBannerJedi, true).value; } public async showBanner(): Promise { @@ -67,11 +67,14 @@ export class ProposePylanceBannerJedi implements IPythonExtensionBanner { return; } + let experimentName = ''; let promptContent: string | undefined; if (await this.experiments.inExperiment(TryPylance.jediPrompt1)) { promptContent = await this.experiments.getExperimentValue(TryPylance.jediPrompt1); + experimentName = TryPylance.jediPrompt1; } else if (await this.experiments.inExperiment(TryPylance.jediPrompt2)) { promptContent = await this.experiments.getExperimentValue(TryPylance.jediPrompt2); + experimentName = TryPylance.jediPrompt2; } if (promptContent === undefined) { @@ -97,7 +100,7 @@ export class ProposePylanceBannerJedi implements IPythonExtensionBanner { this.disabledInCurrentSession = true; userAction = 'later'; } - sendTelemetryEvent(EventName.LANGUAGE_SERVER_TRY_PYLANCE, undefined, { userAction }); + sendTelemetryEvent(EventName.LANGUAGE_SERVER_TRY_PYLANCE, undefined, { userAction, experimentName }); } public async shouldShowBanner(): Promise { @@ -110,7 +113,7 @@ export class ProposePylanceBannerJedi implements IPythonExtensionBanner { public async disable(): Promise { await this.persistentState - .createGlobalPersistentState(ProposeLSStateKeys.ShowBanner, false) + .createGlobalPersistentState(ProposeLSStateKeys.ShowBannerJedi, false) .updateValue(false); } } diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index 9d9b21f48372..6d1fa9569bb2 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -3,6 +3,7 @@ 'use strict'; + export enum EventName { COMPLETION = 'COMPLETION', COMPLETION_ADD_BRACKETS = 'COMPLETION.ADD_BRACKETS', @@ -115,7 +116,8 @@ export enum EventName { HASHED_PACKAGE_NAME = 'HASHED_PACKAGE_NAME', HASHED_PACKAGE_PERF = 'HASHED_PACKAGE_PERF', - JEDI_MEMORY = 'JEDI_MEMORY' + JEDI_MEMORY = 'JEDI_MEMORY', + BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS = "BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS" } export enum PlatformErrors { diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 64d8c5d68422..d1680f8b6d7e 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1305,6 +1305,7 @@ export interface IEventNamePropertyMapping { * @type {string} */ userAction: string; + experimentName: string; }; /** * Telemetry captured for enabling reload. diff --git a/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts new file mode 100644 index 000000000000..9ead88c087b3 --- /dev/null +++ b/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts @@ -0,0 +1,292 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { assert, expect } from 'chai'; +import * as sinon from 'sinon'; +import { anything } from 'ts-mockito'; +import * as typemoq from 'typemoq'; +import { Extension } from 'vscode'; +import { LanguageServerType } from '../../../client/activation/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 { + IConfigurationService, + IExperimentService, + IExtensions, + IPersistentState, + IPersistentStateFactory, + IPythonSettings, +} from '../../../client/common/types'; +import { Common, Pylance } from '../../../client/common/utils/localize'; +import { + getPylanceExtensionUri, + ProposeLSStateKeys, + ProposePylanceBannerJedi, +} from '../../../client/languageServices/proposeLanguageServerBannerJedi'; + +import * as Telemetry from '../../../client/telemetry'; +import { EventName } from '../../../client/telemetry/constants'; + +interface IExperimentLsCombination { + experiment: TryPylance; + lsType: LanguageServerType; + shouldShowBanner: boolean; +} +const testData: IExperimentLsCombination[] = [ + { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.None, shouldShowBanner: true }, + { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Microsoft, shouldShowBanner: true }, + { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Node, shouldShowBanner: false }, + { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Jedi, shouldShowBanner: false }, + { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.None, shouldShowBanner: false }, + { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Microsoft, shouldShowBanner: false }, + { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Node, shouldShowBanner: false }, + { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Jedi, shouldShowBanner: false }, +]; + +suite('Propose Pylance Banner To Jedi', () => { + let config: typemoq.IMock; + let appShell: typemoq.IMock; + let appEnv: typemoq.IMock; + let settings: typemoq.IMock; + let sendTelemetryStub: sinon.SinonStub; + let telemetryEvent: + | { eventName: EventName; properties: { userAction: string; experimentName: string } } + | undefined; + + const yes = Pylance.tryItNow(); + const no = Common.bannerLabelNo(); + const later = Pylance.remindMeLater(); + + 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(); + appEnv.setup((x) => x.uriScheme).returns(() => 'scheme'); + + sendTelemetryStub = sinon + .stub(Telemetry, 'sendTelemetryEvent') + .callsFake((eventName: EventName, _, properties: { userAction: string; experimentName: string }) => { + telemetryEvent = { + eventName, + properties, + }; + }); + }); + + teardown(() => { + telemetryEvent = undefined; + sinon.restore(); + Telemetry._resetSharedProperties(); + }); + + testData.forEach((t) => { + test(`${t.experiment ? '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, appEnv.object, config.object, t.experiment, 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, appEnv.object, config.object, t.experiment, 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) => a.showInformationMessage( + anything(), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later), + )) + .verifiable(typemoq.Times.never()); + const testBanner = preparePopup( + false, + appShell.object, + appEnv.object, + config.object, + TryPylance.jediPrompt1, + false, + ); + await testBanner.showBanner(); + appShell.verifyAll(); + }); + test('Clicking No should disable the banner', async () => { + appShell + .setup((a) => a.showInformationMessage( + anything(), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later), + )) + .returns(async () => no) + .verifiable(typemoq.Times.once()); + appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never()); + + const testBanner = preparePopup( + true, + appShell.object, + appEnv.object, + config.object, + TryPylance.jediPrompt1, + false, + ); + await testBanner.showBanner(); + + expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled when user clicked No'); + appShell.verifyAll(); + + sinon.assert.calledOnce(sendTelemetryStub); + assert.deepEqual(telemetryEvent, { + eventName: EventName.LANGUAGE_SERVER_TRY_PYLANCE, + properties: { userAction: 'no', experimentName: TryPylance.jediPrompt1 }, + }); + }); + test('Clicking Later should disable banner in session', async () => { + appShell + .setup((a) => a.showInformationMessage( + anything(), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later), + )) + .returns(async () => later) + .verifiable(typemoq.Times.once()); + appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never()); + + const testBanner = preparePopup( + true, + appShell.object, + appEnv.object, + config.object, + TryPylance.jediPrompt1, + false, + ); + await testBanner.showBanner(); + + expect(testBanner.enabled).to.be.equal( + true, + 'Banner should not be permanently disabled when user clicked Later', + ); + appShell.verifyAll(); + + sinon.assert.calledOnce(sendTelemetryStub); + assert.deepEqual(telemetryEvent, { + eventName: EventName.BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS, + properties: { + userAction: 'later', + experimentName: TryPylance.jediPrompt1, + }, + }); + }); + test('Clicking Yes opens the extension marketplace entry', async () => { + appShell + .setup((a) => a.showInformationMessage( + anything(), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later), + )) + .returns(async () => yes) + .verifiable(typemoq.Times.once()); + appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.once()); + + const testBanner = preparePopup( + true, + appShell.object, + appEnv.object, + config.object, + TryPylance.jediPrompt1, + false, + ); + await testBanner.showBanner(); + + expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled after opening store URL'); + appShell.verifyAll(); + + sinon.assert.calledOnce(sendTelemetryStub); + assert.deepEqual(telemetryEvent, { + eventName: EventName.BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS, + properties: { + userAction: 'yes', + experimentName: TryPylance.jediPrompt1, + }, + }); + }); +}); + +function preparePopup( + enabledValue: boolean, + appShell: IApplicationShell, + appEnv: IApplicationEnvironment, + config: IConfigurationService, + experiment: TryPylance, + pylanceInstalled: boolean, +): ProposePylanceBannerJedi { + const myfactory = typemoq.Mock.ofType(); + const val = 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(() => enabledValue); + myfactory + .setup((a) => a.createGlobalPersistentState( + typemoq.It.isValue(ProposeLSStateKeys.ShowBannerJedi), + typemoq.It.isValue(true), + )) + .returns(() => val.object); + myfactory + .setup((a) => a.createGlobalPersistentState( + typemoq.It.isValue(ProposeLSStateKeys.ShowBannerJedi), + typemoq.It.isValue(false), + )) + .returns(() => val.object); + + const experiments = typemoq.Mock.ofType(); + experiments + .setup((x) => x.inExperiment(TryPylance.jediPrompt1)) + .returns(() => Promise.resolve(experiment === TryPylance.jediPrompt1)); + + experiments + .setup((x) => x.getExperimentValue(TryPylance.jediPrompt1)) + .returns(() => Promise.resolve('Sample value1')); + + experiments + .setup((x) => x.inExperiment(TryPylance.jediPrompt2)) + .returns(() => Promise.resolve(experiment === TryPylance.jediPrompt2)); + + experiments + .setup((x) => x.getExperimentValue(TryPylance.jediPrompt2)) + .returns(() => Promise.resolve('Sample value2')); + + 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 ProposePylanceBannerJedi( + appShell, + appEnv, + myfactory.object, + config, + experiments.object, + extensions.object, + ); +} From ee5d9f705230d45d7b04f407c6d0489b1c11f7ea Mon Sep 17 00:00:00 2001 From: Bill Schnurr Date: Thu, 12 Nov 2020 11:00:14 -0800 Subject: [PATCH 3/4] disabled eslint --- .../proposeLanguageServerBannerJedi.ts | 2 +- ...seNewLanguageServerBannerJedi.unit.test.ts | 108 ++++++++++-------- 2 files changed, 61 insertions(+), 49 deletions(-) diff --git a/src/client/languageServices/proposeLanguageServerBannerJedi.ts b/src/client/languageServices/proposeLanguageServerBannerJedi.ts index 65b8a5ab1c3c..f6e6f57fc33f 100644 --- a/src/client/languageServices/proposeLanguageServerBannerJedi.ts +++ b/src/client/languageServices/proposeLanguageServerBannerJedi.ts @@ -85,7 +85,7 @@ export class ProposePylanceBannerJedi implements IPythonExtensionBanner { promptContent, Pylance.tryItNow(), Common.bannerLabelNo(), - Pylance.remindMeLater(), + Pylance.remindMeLater() ); let userAction: string; diff --git a/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts index 9ead88c087b3..a6ae87c566fd 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts @@ -18,13 +18,13 @@ import { IExtensions, IPersistentState, IPersistentStateFactory, - IPythonSettings, + IPythonSettings } from '../../../client/common/types'; import { Common, Pylance } from '../../../client/common/utils/localize'; import { getPylanceExtensionUri, ProposeLSStateKeys, - ProposePylanceBannerJedi, + ProposePylanceBannerJedi } from '../../../client/languageServices/proposeLanguageServerBannerJedi'; import * as Telemetry from '../../../client/telemetry'; @@ -43,7 +43,7 @@ const testData: IExperimentLsCombination[] = [ { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.None, shouldShowBanner: false }, { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Microsoft, shouldShowBanner: false }, { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Node, shouldShowBanner: false }, - { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Jedi, shouldShowBanner: false }, + { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Jedi, shouldShowBanner: false } ]; suite('Propose Pylance Banner To Jedi', () => { @@ -73,7 +73,7 @@ suite('Propose Pylance Banner To Jedi', () => { .callsFake((eventName: EventName, _, properties: { userAction: string; experimentName: string }) => { telemetryEvent = { eventName, - properties, + properties }; }); }); @@ -104,12 +104,14 @@ suite('Propose Pylance Banner To Jedi', () => { }); test('Do not show banner when it is disabled', async () => { appShell - .setup((a) => a.showInformationMessage( - anything(), - typemoq.It.isValue(yes), - typemoq.It.isValue(no), - typemoq.It.isValue(later), - )) + .setup((a) => + a.showInformationMessage( + anything(), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later) + ) + ) .verifiable(typemoq.Times.never()); const testBanner = preparePopup( false, @@ -117,19 +119,21 @@ suite('Propose Pylance Banner To Jedi', () => { appEnv.object, config.object, TryPylance.jediPrompt1, - false, + false ); await testBanner.showBanner(); appShell.verifyAll(); }); test('Clicking No should disable the banner', async () => { appShell - .setup((a) => a.showInformationMessage( - anything(), - typemoq.It.isValue(yes), - typemoq.It.isValue(no), - typemoq.It.isValue(later), - )) + .setup((a) => + a.showInformationMessage( + anything(), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later) + ) + ) .returns(async () => no) .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never()); @@ -140,7 +144,7 @@ suite('Propose Pylance Banner To Jedi', () => { appEnv.object, config.object, TryPylance.jediPrompt1, - false, + false ); await testBanner.showBanner(); @@ -150,17 +154,19 @@ suite('Propose Pylance Banner To Jedi', () => { sinon.assert.calledOnce(sendTelemetryStub); assert.deepEqual(telemetryEvent, { eventName: EventName.LANGUAGE_SERVER_TRY_PYLANCE, - properties: { userAction: 'no', experimentName: TryPylance.jediPrompt1 }, + properties: { userAction: 'no', experimentName: TryPylance.jediPrompt1 } }); }); test('Clicking Later should disable banner in session', async () => { appShell - .setup((a) => a.showInformationMessage( - anything(), - typemoq.It.isValue(yes), - typemoq.It.isValue(no), - typemoq.It.isValue(later), - )) + .setup((a) => + a.showInformationMessage( + anything(), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later) + ) + ) .returns(async () => later) .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never()); @@ -171,13 +177,13 @@ suite('Propose Pylance Banner To Jedi', () => { appEnv.object, config.object, TryPylance.jediPrompt1, - false, + false ); await testBanner.showBanner(); expect(testBanner.enabled).to.be.equal( true, - 'Banner should not be permanently disabled when user clicked Later', + 'Banner should not be permanently disabled when user clicked Later' ); appShell.verifyAll(); @@ -186,18 +192,20 @@ suite('Propose Pylance Banner To Jedi', () => { eventName: EventName.BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS, properties: { userAction: 'later', - experimentName: TryPylance.jediPrompt1, - }, + experimentName: TryPylance.jediPrompt1 + } }); }); test('Clicking Yes opens the extension marketplace entry', async () => { appShell - .setup((a) => a.showInformationMessage( - anything(), - typemoq.It.isValue(yes), - typemoq.It.isValue(no), - typemoq.It.isValue(later), - )) + .setup((a) => + a.showInformationMessage( + anything(), + typemoq.It.isValue(yes), + typemoq.It.isValue(no), + typemoq.It.isValue(later) + ) + ) .returns(async () => yes) .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.once()); @@ -208,7 +216,7 @@ suite('Propose Pylance Banner To Jedi', () => { appEnv.object, config.object, TryPylance.jediPrompt1, - false, + false ); await testBanner.showBanner(); @@ -220,8 +228,8 @@ suite('Propose Pylance Banner To Jedi', () => { eventName: EventName.BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS, properties: { userAction: 'yes', - experimentName: TryPylance.jediPrompt1, - }, + experimentName: TryPylance.jediPrompt1 + } }); }); }); @@ -232,7 +240,7 @@ function preparePopup( appEnv: IApplicationEnvironment, config: IConfigurationService, experiment: TryPylance, - pylanceInstalled: boolean, + pylanceInstalled: boolean ): ProposePylanceBannerJedi { const myfactory = typemoq.Mock.ofType(); const val = typemoq.Mock.ofType>(); @@ -246,16 +254,20 @@ function preparePopup( }); val.setup((a) => a.value).returns(() => enabledValue); myfactory - .setup((a) => a.createGlobalPersistentState( - typemoq.It.isValue(ProposeLSStateKeys.ShowBannerJedi), - typemoq.It.isValue(true), - )) + .setup((a) => + a.createGlobalPersistentState( + typemoq.It.isValue(ProposeLSStateKeys.ShowBannerJedi), + typemoq.It.isValue(true) + ) + ) .returns(() => val.object); myfactory - .setup((a) => a.createGlobalPersistentState( - typemoq.It.isValue(ProposeLSStateKeys.ShowBannerJedi), - typemoq.It.isValue(false), - )) + .setup((a) => + a.createGlobalPersistentState( + typemoq.It.isValue(ProposeLSStateKeys.ShowBannerJedi), + typemoq.It.isValue(false) + ) + ) .returns(() => val.object); const experiments = typemoq.Mock.ofType(); @@ -287,6 +299,6 @@ function preparePopup( myfactory.object, config, experiments.object, - extensions.object, + extensions.object ); } From 836ce7c7d6aaa13b83279a9a94def1113eb93d7e Mon Sep 17 00:00:00 2001 From: Bill Schnurr Date: Thu, 12 Nov 2020 16:30:57 -0800 Subject: [PATCH 4/4] updating tests to pass. adding experimentName to old tryPlance test telemetry --- ...roposeNewLanguageServerBanner.unit.test.ts | 13 +++++--- ...seNewLanguageServerBannerJedi.unit.test.ts | 33 +++++++++++-------- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts index 7dcfda29a19d..24f7a286a6c9 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts @@ -49,7 +49,9 @@ suite('Propose Pylance Banner', () => { let appEnv: typemoq.IMock; let settings: typemoq.IMock; let sendTelemetryStub: sinon.SinonStub; - let telemetryEvent: { eventName: EventName; properties: { userAction: string } } | undefined; + let telemetryEvent: + | { eventName: EventName; properties: { userAction: string; experimentName: string } } + | undefined; const message = Pylance.proposePylanceMessage(); const yes = Pylance.tryItNow(); @@ -66,7 +68,7 @@ suite('Propose Pylance Banner', () => { sendTelemetryStub = sinon .stub(Telemetry, 'sendTelemetryEvent') - .callsFake((eventName: EventName, _, properties: { userAction: string }) => { + .callsFake((eventName: EventName, _, properties: { userAction: string; experimentName: string }) => { telemetryEvent = { eventName, properties @@ -136,7 +138,7 @@ suite('Propose Pylance Banner', () => { sinon.assert.calledOnce(sendTelemetryStub); assert.deepEqual(telemetryEvent, { eventName: EventName.LANGUAGE_SERVER_TRY_PYLANCE, - properties: { userAction: 'no' } + properties: { userAction: 'no', experimentName: TryPylance.experiment } }); }); test('Clicking Later should disable banner in session', async () => { @@ -166,7 +168,7 @@ suite('Propose Pylance Banner', () => { assert.deepEqual(telemetryEvent, { eventName: EventName.LANGUAGE_SERVER_TRY_PYLANCE, properties: { - userAction: 'later' + userAction: 'later', experimentName: TryPylance.experiment } }); }); @@ -194,7 +196,8 @@ suite('Propose Pylance Banner', () => { assert.deepEqual(telemetryEvent, { eventName: EventName.LANGUAGE_SERVER_TRY_PYLANCE, properties: { - userAction: 'yes' + userAction: 'yes', + experimentName: TryPylance.experiment } }); }); diff --git a/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts b/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts index a6ae87c566fd..9d3619b79501 100644 --- a/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts +++ b/src/test/testing/banners/proposeNewLanguageServerBannerJedi.unit.test.ts @@ -5,7 +5,6 @@ import { assert, expect } from 'chai'; import * as sinon from 'sinon'; -import { anything } from 'ts-mockito'; import * as typemoq from 'typemoq'; import { Extension } from 'vscode'; import { LanguageServerType } from '../../../client/activation/types'; @@ -31,22 +30,23 @@ import * as Telemetry from '../../../client/telemetry'; import { EventName } from '../../../client/telemetry/constants'; interface IExperimentLsCombination { - experiment: TryPylance; + experiment: TryPylance ; lsType: LanguageServerType; shouldShowBanner: boolean; } const testData: IExperimentLsCombination[] = [ - { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.None, shouldShowBanner: true }, - { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Microsoft, shouldShowBanner: true }, + // Expected Result is shouldShowBanner + { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.None, shouldShowBanner: false }, + { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Microsoft, shouldShowBanner: false }, { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Node, shouldShowBanner: false }, - { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Jedi, shouldShowBanner: false }, + { experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Jedi, shouldShowBanner: true }, { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.None, shouldShowBanner: false }, { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Microsoft, shouldShowBanner: false }, { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Node, shouldShowBanner: false }, - { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Jedi, shouldShowBanner: false } + { experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Jedi, shouldShowBanner: true } ]; -suite('Propose Pylance Banner To Jedi', () => { +suite('Jedi Propose Pylance Banner', () => { let config: typemoq.IMock; let appShell: typemoq.IMock; let appEnv: typemoq.IMock; @@ -56,6 +56,7 @@ suite('Propose Pylance Banner To Jedi', () => { | { eventName: EventName; properties: { userAction: string; experimentName: string } } | undefined; + const message = 'Sample value1'; const yes = Pylance.tryItNow(); const no = Common.bannerLabelNo(); const later = Pylance.remindMeLater(); @@ -106,7 +107,7 @@ suite('Propose Pylance Banner To Jedi', () => { appShell .setup((a) => a.showInformationMessage( - anything(), + typemoq.It.isValue(message), typemoq.It.isValue(yes), typemoq.It.isValue(no), typemoq.It.isValue(later) @@ -128,7 +129,7 @@ suite('Propose Pylance Banner To Jedi', () => { appShell .setup((a) => a.showInformationMessage( - anything(), + typemoq.It.isValue(message), typemoq.It.isValue(yes), typemoq.It.isValue(no), typemoq.It.isValue(later) @@ -138,6 +139,8 @@ suite('Propose Pylance Banner To Jedi', () => { .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never()); + settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Jedi); + const testBanner = preparePopup( true, appShell.object, @@ -161,7 +164,7 @@ suite('Propose Pylance Banner To Jedi', () => { appShell .setup((a) => a.showInformationMessage( - anything(), + typemoq.It.isValue(message), typemoq.It.isValue(yes), typemoq.It.isValue(no), typemoq.It.isValue(later) @@ -171,6 +174,8 @@ suite('Propose Pylance Banner To Jedi', () => { .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never()); + settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Jedi); + const testBanner = preparePopup( true, appShell.object, @@ -189,7 +194,7 @@ suite('Propose Pylance Banner To Jedi', () => { sinon.assert.calledOnce(sendTelemetryStub); assert.deepEqual(telemetryEvent, { - eventName: EventName.BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS, + eventName: EventName.LANGUAGE_SERVER_TRY_PYLANCE, properties: { userAction: 'later', experimentName: TryPylance.jediPrompt1 @@ -200,7 +205,7 @@ suite('Propose Pylance Banner To Jedi', () => { appShell .setup((a) => a.showInformationMessage( - anything(), + typemoq.It.isValue(message), typemoq.It.isValue(yes), typemoq.It.isValue(no), typemoq.It.isValue(later) @@ -210,6 +215,8 @@ suite('Propose Pylance Banner To Jedi', () => { .verifiable(typemoq.Times.once()); appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.once()); + settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Jedi); + const testBanner = preparePopup( true, appShell.object, @@ -225,7 +232,7 @@ suite('Propose Pylance Banner To Jedi', () => { sinon.assert.calledOnce(sendTelemetryStub); assert.deepEqual(telemetryEvent, { - eventName: EventName.BANNER_NAME_PROPOSE_LS_FOR_JEDI_USERS, + eventName: EventName.LANGUAGE_SERVER_TRY_PYLANCE, properties: { userAction: 'yes', experimentName: TryPylance.jediPrompt1