Skip to content

Cherry pick Add new tryPylance banner experiments (#14759) #14774

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+136 −48
Closed

Cherry pick Add new tryPylance banner experiments (#14759) #14774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/client/activation/jedi/multiplexingActivator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -13,9 +13,16 @@ import {
} 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,
IConfigurationService,
IExperimentService,
IPythonExtensionBanner,
Resource
} from '../../common/types';
import { IServiceManager } from '../../ioc/types';
import { PythonEnvironment } from '../../pythonEnvironments/info';
import { JediExtensionActivator } from '../jedi';
Expand All @@ -38,7 +45,10 @@ export class MultiplexingJediLanguageServerActivator implements ILanguageServerA

constructor(
@inject(IServiceManager) private readonly manager: IServiceManager,
@inject(IExperimentService) experimentService: IExperimentService
@inject(IExperimentService) experimentService: IExperimentService,
@inject(IPythonExtensionBanner)
@named(BANNER_NAME_PROPOSE_LS)
private proposePylancePopup: IPythonExtensionBanner
) {
// Check experiment service to see if using new Jedi LSP protocol
this.realLanguageServerPromise = experimentService.inExperiment(JediLSP.experiment).then((inExperiment) => {
Expand All @@ -56,6 +66,9 @@ export class MultiplexingJediLanguageServerActivator implements ILanguageServerA
}
public async start(resource: Resource, interpreter: PythonEnvironment | undefined): Promise<void> {
const realServer = await this.realLanguageServerPromise;
if (!isTestExecution()) {
this.proposePylancePopup.showBanner().ignoreErrors();
}
return realServer.start(resource, interpreter);
}
public activate(): void {
Expand Down
4 changes: 3 additions & 1 deletion src/client/common/experiments/groups.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
47 changes: 30 additions & 17 deletions src/client/languageServices/proposeLanguageServerBanner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,6 @@ export class ProposePylanceBanner implements IPythonExtensionBanner {
) {}

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<boolean>(ProposeLSStateKeys.ShowBanner, true).value;
}

Expand All @@ -62,13 +58,13 @@ export class ProposePylanceBanner implements IPythonExtensionBanner {
return;
}

const show = await this.shouldShowBanner();
if (!show) {
const message = await this.getPromptMessage();
if (!message) {
return;
}

const response = await this.appShell.showInformationMessage(
Pylance.proposePylanceMessage(),
message,
Pylance.tryItNow(),
Common.bannerLabelNo(),
Pylance.remindMeLater()
Expand All @@ -89,19 +85,36 @@ export class ProposePylanceBanner implements IPythonExtensionBanner {
sendTelemetryEvent(EventName.LANGUAGE_SERVER_TRY_PYLANCE, undefined, { userAction });
}

public async shouldShowBanner(): Promise<boolean> {
// 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;
}

public async disable(): Promise<void> {
await this.persistentState
.createGlobalPersistentState<boolean>(ProposeLSStateKeys.ShowBanner, false)
.updateValue(false);
}

public async getPromptMessage(): Promise<string | undefined> {
if (this.disabledInCurrentSession) {
return undefined;
}

// Do not prompt if Pylance is already installed.
if (this.extensions.getExtension(PYLANCE_EXTENSION_ID)) {
return undefined;
}

const lsType = this.configuration.getSettings().languageServer ?? LanguageServerType.Jedi;

if (lsType === LanguageServerType.Jedi) {
if (await this.experiments.inExperiment(TryPylance.jediPrompt1)) {
return this.experiments.getExperimentValue<string>(TryPylance.jediPrompt1);
} else if (await this.experiments.inExperiment(TryPylance.jediPrompt2)) {
return this.experiments.getExperimentValue<string>(TryPylance.jediPrompt2);
}
} else if (lsType === LanguageServerType.Microsoft || lsType === LanguageServerType.None) {
if (await this.experiments.inExperiment(TryPylance.experiment)) {
return Pylance.proposePylanceMessage();
}
}

return undefined;
}
}
114 changes: 87 additions & 27 deletions src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,38 @@ import * as Telemetry from '../../../client/telemetry';
import { EventName } from '../../../client/telemetry/constants';

interface IExperimentLsCombination {
inExperiment: boolean;
experiment?: TryPylance;
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 }
{ experiment: undefined, lsType: LanguageServerType.None, shouldShowBanner: false },
{ experiment: undefined, lsType: LanguageServerType.Microsoft, shouldShowBanner: false },
{ experiment: undefined, lsType: LanguageServerType.Node, shouldShowBanner: false },
{ experiment: undefined, lsType: LanguageServerType.Jedi, shouldShowBanner: false },

{ experiment: TryPylance.experiment, lsType: LanguageServerType.None, shouldShowBanner: true },
{ experiment: TryPylance.experiment, lsType: LanguageServerType.Microsoft, shouldShowBanner: true },
{ experiment: TryPylance.experiment, lsType: LanguageServerType.Node, shouldShowBanner: false },
{ experiment: TryPylance.experiment, lsType: LanguageServerType.Jedi, shouldShowBanner: false },

{ 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: 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: true }
];

const expectedMessages = {
[TryPylance.experiment]: Pylance.proposePylanceMessage(),
[TryPylance.jediPrompt1]: 'Message for jediPrompt1',
[TryPylance.jediPrompt2]: 'Message for jediPrompt2'
};

suite('Propose Pylance Banner', () => {
let config: typemoq.IMock<IConfigurationService>;
let appShell: typemoq.IMock<IApplicationShell>;
Expand All @@ -51,7 +68,6 @@ suite('Propose Pylance Banner', () => {
let sendTelemetryStub: sinon.SinonStub;
let telemetryEvent: { eventName: EventName; properties: { userAction: string } } | undefined;

const message = Pylance.proposePylanceMessage();
const yes = Pylance.tryItNow();
const no = Common.bannerLabelNo();
const later = Pylance.remindMeLater();
Expand Down Expand Up @@ -81,43 +97,59 @@ suite('Propose Pylance Banner', () => {
});

testData.forEach((t) => {
test(`${t.inExperiment ? 'In' : 'Not in'} experiment and "python.languageServer": "${t.lsType}" should ${
test(`${t.experiment} 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.inExperiment, false);
const actual = await testBanner.shouldShowBanner();
expect(actual).to.be.equal(t.shouldShowBanner, `shouldShowBanner() returned ${actual}`);
const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, t.experiment, false);
const message = await testBanner.getPromptMessage();
if (t.experiment) {
expect(message).to.be.equal(
t.shouldShowBanner ? expectedMessages[t.experiment] : undefined,
`getPromptMessage() returned ${message}`
);
} else {
expect(message).to.be.equal(undefined, `message should be undefined`);
}
});
});
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.inExperiment, true);
const actual = await testBanner.shouldShowBanner();
expect(actual).to.be.equal(false, `shouldShowBanner() returned ${actual}`);
const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, t.experiment, true);
const message = await testBanner.getPromptMessage();
expect(message).to.be.equal(undefined, `getPromptMessage() returned ${message}`);
});
});
test('Do not show banner when it is disabled', async () => {
settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Microsoft);
appShell
.setup((a) =>
a.showInformationMessage(
typemoq.It.isValue(message),
typemoq.It.isValue(expectedMessages[TryPylance.experiment]),
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, true, false);
const testBanner = preparePopup(
false,
appShell.object,
appEnv.object,
config.object,
TryPylance.experiment,
false
);
await testBanner.showBanner();
appShell.verifyAll();
});
test('Clicking No should disable the banner', async () => {
settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Microsoft);
appShell
.setup((a) =>
a.showInformationMessage(
typemoq.It.isValue(message),
typemoq.It.isValue(expectedMessages[TryPylance.experiment]),
typemoq.It.isValue(yes),
typemoq.It.isValue(no),
typemoq.It.isValue(later)
Expand All @@ -127,7 +159,14 @@ suite('Propose Pylance Banner', () => {
.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, true, false);
const testBanner = preparePopup(
true,
appShell.object,
appEnv.object,
config.object,
TryPylance.experiment,
false
);
await testBanner.showBanner();

expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled when user clicked No');
Expand All @@ -140,10 +179,11 @@ suite('Propose Pylance Banner', () => {
});
});
test('Clicking Later should disable banner in session', async () => {
settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Microsoft);
appShell
.setup((a) =>
a.showInformationMessage(
typemoq.It.isValue(message),
typemoq.It.isValue(expectedMessages[TryPylance.experiment]),
typemoq.It.isValue(yes),
typemoq.It.isValue(no),
typemoq.It.isValue(later)
Expand All @@ -153,7 +193,14 @@ suite('Propose Pylance Banner', () => {
.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, true, false);
const testBanner = preparePopup(
true,
appShell.object,
appEnv.object,
config.object,
TryPylance.experiment,
false
);
await testBanner.showBanner();

expect(testBanner.enabled).to.be.equal(
Expand All @@ -171,10 +218,11 @@ suite('Propose Pylance Banner', () => {
});
});
test('Clicking Yes opens the extension marketplace entry', async () => {
settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Microsoft);
appShell
.setup((a) =>
a.showInformationMessage(
typemoq.It.isValue(message),
typemoq.It.isValue(expectedMessages[TryPylance.experiment]),
typemoq.It.isValue(yes),
typemoq.It.isValue(no),
typemoq.It.isValue(later)
Expand All @@ -184,7 +232,14 @@ suite('Propose Pylance Banner', () => {
.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, true, false);
const testBanner = preparePopup(
true,
appShell.object,
appEnv.object,
config.object,
TryPylance.experiment,
false
);
await testBanner.showBanner();

expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled after opening store URL');
Expand All @@ -205,7 +260,7 @@ function preparePopup(
appShell: IApplicationShell,
appEnv: IApplicationEnvironment,
config: IConfigurationService,
inExperiment: boolean,
experiment: TryPylance | undefined,
pylanceInstalled: boolean
): ProposePylanceBanner {
const myfactory = typemoq.Mock.ofType<IPersistentStateFactory>();
Expand Down Expand Up @@ -237,7 +292,12 @@ function preparePopup(
});

const experiments = typemoq.Mock.ofType<IExperimentService>();
experiments.setup((x) => x.inExperiment(TryPylance.experiment)).returns(() => Promise.resolve(inExperiment));
Object.values(TryPylance).forEach((exp) => {
experiments.setup((x) => x.inExperiment(exp)).returns(() => Promise.resolve(exp === experiment));
if (exp !== TryPylance.experiment) {
experiments.setup((x) => x.getExperimentValue(exp)).returns(() => Promise.resolve(expectedMessages[exp]));
}
});

const extensions = typemoq.Mock.ofType<IExtensions>();
// tslint:disable-next-line: no-any
Expand Down