From 3a36a8940abc4c4c75e631d932417aa1d369039c Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 3 Oct 2019 10:59:30 -0700 Subject: [PATCH 1/4] Added button to get more info related to inheritEnv setting when prompt is dispalyed (#7742) * Added button to get more info related to inheritEnv setting when prompt is displayed * Use aka.ms URL --- package.nls.json | 1 + src/client/common/utils/localize.ts | 1 + .../virtualEnvs/condaInheritEnvPrompt.ts | 11 ++- src/client/telemetry/index.ts | 3 +- .../condaInheritEnvPrompt.unit.test.ts | 73 ++++++++++++++++--- 5 files changed, 74 insertions(+), 15 deletions(-) diff --git a/package.nls.json b/package.nls.json index 1c70d070fa5c..ed684a6544db 100644 --- a/package.nls.json +++ b/package.nls.json @@ -125,6 +125,7 @@ "Logging.CurrentWorkingDirectory": "cwd:", "Common.doNotShowAgain": "Do not show again", "Common.reload": "Reload", + "Common.moreInfo": "More Info", "OutputChannelNames.languageServer": "Python Language Server", "OutputChannelNames.python": "Python", "OutputChannelNames.pythonTest": "Python Test Log", diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 883d86a28241..7137b4c97e07 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -33,6 +33,7 @@ export namespace Common { export const notNow = localize('Common.notNow', 'Not now'); export const doNotShowAgain = localize('Common.doNotShowAgain', 'Do not show again'); export const reload = localize('Common.reload', 'Reload'); + export const moreInfo = localize('Common.moreInfo', 'More Info'); } export namespace LanguageService { diff --git a/src/client/interpreter/virtualEnvs/condaInheritEnvPrompt.ts b/src/client/interpreter/virtualEnvs/condaInheritEnvPrompt.ts index 0dfec6eb5780..ea5dc824dd43 100644 --- a/src/client/interpreter/virtualEnvs/condaInheritEnvPrompt.ts +++ b/src/client/interpreter/virtualEnvs/condaInheritEnvPrompt.ts @@ -6,8 +6,8 @@ import { ConfigurationTarget, Uri } from 'vscode'; import { IExtensionActivationService } from '../../activation/types'; import { IApplicationShell, IWorkspaceService } from '../../common/application/types'; import { traceDecorators, traceError } from '../../common/logger'; -import { IPersistentStateFactory } from '../../common/types'; -import { InteractiveShiftEnterBanner, Interpreters } from '../../common/utils/localize'; +import { IBrowserService, IPersistentStateFactory } from '../../common/types'; +import { Common, InteractiveShiftEnterBanner, Interpreters } from '../../common/utils/localize'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; import { IInterpreterService, InterpreterType } from '../contracts'; @@ -19,6 +19,7 @@ export class CondaInheritEnvPrompt implements IExtensionActivationService { constructor( @inject(IInterpreterService) private readonly interpreterService: IInterpreterService, @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, + @inject(IBrowserService) private browserService: IBrowserService, @inject(IApplicationShell) private readonly appShell: IApplicationShell, @inject(IPersistentStateFactory) private readonly persistentStateFactory: IPersistentStateFactory, @optional() public hasPromptBeenShownInCurrentSession: boolean = false @@ -43,8 +44,8 @@ export class CondaInheritEnvPrompt implements IExtensionActivationService { if (!notificationPromptEnabled.value) { return; } - const prompts = [InteractiveShiftEnterBanner.bannerLabelYes(), InteractiveShiftEnterBanner.bannerLabelNo()]; - const telemetrySelections: ['Yes', 'No'] = ['Yes', 'No']; + const prompts = [InteractiveShiftEnterBanner.bannerLabelYes(), InteractiveShiftEnterBanner.bannerLabelNo(), Common.moreInfo()]; + const telemetrySelections: ['Yes', 'No', 'More Info'] = ['Yes', 'No', 'More Info']; const selection = await this.appShell.showInformationMessage(Interpreters.condaInheritEnvMessage(), ...prompts); sendTelemetryEvent(EventName.CONDA_INHERIT_ENV_PROMPT, undefined, { selection: selection ? telemetrySelections[prompts.indexOf(selection)] : undefined }); if (!selection) { @@ -54,6 +55,8 @@ export class CondaInheritEnvPrompt implements IExtensionActivationService { await this.workspaceService.getConfiguration('terminal').update('integrated.inheritEnv', false, ConfigurationTarget.Global); } else if (selection === prompts[1]) { await notificationPromptEnabled.updateValue(false); + } else if (selection === prompts[2]) { + this.browserService.launch('https://aka.ms/AA66i8f'); } } diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 80b0956bb734..9c48e49a4b7b 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -939,8 +939,9 @@ export interface IEventNamePropertyMapping { /** * `Yes` When 'Yes' option is selected * `No` When 'No' option is selected + * `More info` When 'More Info' option is selected */ - selection: 'Yes' | 'No' | undefined; + selection: 'Yes' | 'No' | 'More Info' | undefined; }; /** * Telemetry event sent with details when user clicks a button in the virtual environment prompt. diff --git a/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts b/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts index 8939ee595c57..0b469ae9300b 100644 --- a/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts +++ b/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts @@ -10,9 +10,9 @@ import * as TypeMoq from 'typemoq'; import { ConfigurationTarget, Uri, WorkspaceConfiguration } from 'vscode'; import { IApplicationShell, IWorkspaceService } from '../../../client/common/application/types'; import { PersistentStateFactory } from '../../../client/common/persistentState'; -import { IPersistentState, IPersistentStateFactory } from '../../../client/common/types'; +import { IBrowserService, IPersistentState, IPersistentStateFactory } from '../../../client/common/types'; import { createDeferred, createDeferredFromPromise, sleep } from '../../../client/common/utils/async'; -import { InteractiveShiftEnterBanner, Interpreters } from '../../../client/common/utils/localize'; +import { Common, InteractiveShiftEnterBanner, Interpreters } from '../../../client/common/utils/localize'; import { IInterpreterService, InterpreterType } from '../../../client/interpreter/contracts'; import { CondaInheritEnvPrompt, condaInheritEnvPromptKey } from '../../../client/interpreter/virtualEnvs/condaInheritEnvPrompt'; @@ -24,6 +24,7 @@ suite('Conda Inherit Env Prompt', async () => { let workspaceService: TypeMoq.IMock; let appShell: TypeMoq.IMock; let interpreterService: TypeMoq.IMock; + let browserService: TypeMoq.IMock; let persistentStateFactory: IPersistentStateFactory; let notificationPromptEnabled: TypeMoq.IMock>; let condaInheritEnvPrompt: CondaInheritEnvPrompt; @@ -37,12 +38,13 @@ suite('Conda Inherit Env Prompt', async () => { setup(() => { workspaceService = TypeMoq.Mock.ofType(); appShell = TypeMoq.Mock.ofType(); + browserService = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); persistentStateFactory = mock(PersistentStateFactory); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); }); test('Returns false if prompt has already been shown in the current session', async () => { - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, appShell.object, instance(persistentStateFactory), true); + condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory), true); const workspaceConfig = TypeMoq.Mock.ofType(); interpreterService .setup(is => is.getActiveInterpreter(resource)) @@ -191,6 +193,7 @@ suite('Conda Inherit Env Prompt', async () => { setup(() => { workspaceService = TypeMoq.Mock.ofType(); appShell = TypeMoq.Mock.ofType(); + browserService = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); persistentStateFactory = mock(PersistentStateFactory); }); @@ -203,7 +206,7 @@ suite('Conda Inherit Env Prompt', async () => { const initializeInBackgroundDeferred = createDeferred(); initializeInBackground = sinon.stub(CondaInheritEnvPrompt.prototype, 'initializeInBackground'); initializeInBackground.callsFake(() => initializeInBackgroundDeferred.promise); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); const promise = condaInheritEnvPrompt.activate(resource); const deferred = createDeferredFromPromise(promise); @@ -220,7 +223,7 @@ suite('Conda Inherit Env Prompt', async () => { test('Ignores errors raised by initializeInBackground()', async () => { initializeInBackground = sinon.stub(CondaInheritEnvPrompt.prototype, 'initializeInBackground'); initializeInBackground.rejects(new Error('Kaboom')); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); await condaInheritEnvPrompt.activate(resource); assert.ok(initializeInBackground.calledOnce); }); @@ -232,6 +235,7 @@ suite('Conda Inherit Env Prompt', async () => { setup(() => { workspaceService = TypeMoq.Mock.ofType(); appShell = TypeMoq.Mock.ofType(); + browserService = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); persistentStateFactory = mock(PersistentStateFactory); }); @@ -245,7 +249,7 @@ suite('Conda Inherit Env Prompt', async () => { shouldShowPrompt.callsFake(() => Promise.resolve(true)); promptAndUpdate = sinon.stub(CondaInheritEnvPrompt.prototype, 'promptAndUpdate'); promptAndUpdate.callsFake(() => Promise.resolve(undefined)); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); await condaInheritEnvPrompt.initializeInBackground(resource); assert.ok(shouldShowPrompt.calledOnce); assert.ok(promptAndUpdate.calledOnce); @@ -256,7 +260,7 @@ suite('Conda Inherit Env Prompt', async () => { shouldShowPrompt.callsFake(() => Promise.resolve(false)); promptAndUpdate = sinon.stub(CondaInheritEnvPrompt.prototype, 'promptAndUpdate'); promptAndUpdate.callsFake(() => Promise.resolve(undefined)); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); await condaInheritEnvPrompt.initializeInBackground(resource); assert.ok(shouldShowPrompt.calledOnce); assert.ok(promptAndUpdate.notCalled); @@ -264,15 +268,16 @@ suite('Conda Inherit Env Prompt', async () => { }); suite('Method promptAndUpdate()', () => { - const prompts = [InteractiveShiftEnterBanner.bannerLabelYes(), InteractiveShiftEnterBanner.bannerLabelNo()]; + const prompts = [InteractiveShiftEnterBanner.bannerLabelYes(), InteractiveShiftEnterBanner.bannerLabelNo(), Common.moreInfo()]; setup(() => { workspaceService = TypeMoq.Mock.ofType(); appShell = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); persistentStateFactory = mock(PersistentStateFactory); + browserService = TypeMoq.Mock.ofType(); notificationPromptEnabled = TypeMoq.Mock.ofType>(); when(persistentStateFactory.createGlobalPersistentState(condaInheritEnvPromptKey, true)).thenReturn(notificationPromptEnabled.object); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); }); test('Does not display prompt if it is disabled', async () => { @@ -311,11 +316,16 @@ suite('Conda Inherit Env Prompt', async () => { .setup(n => n.updateValue(false)) .returns(() => Promise.resolve(undefined)) .verifiable(TypeMoq.Times.never()); + browserService + .setup(b => b.launch('https://aka.ms/AA66i8f')) + .returns(() => undefined) + .verifiable(TypeMoq.Times.never()); await condaInheritEnvPrompt.promptAndUpdate(); verify(persistentStateFactory.createGlobalPersistentState(condaInheritEnvPromptKey, true)).once(); verifyAll(); workspaceConfig.verifyAll(); notificationPromptEnabled.verifyAll(); + browserService.verifyAll(); }); test('Update terminal settings if `Yes` is selected', async () => { const workspaceConfig = TypeMoq.Mock.ofType(); @@ -339,11 +349,16 @@ suite('Conda Inherit Env Prompt', async () => { .setup(n => n.updateValue(false)) .returns(() => Promise.resolve(undefined)) .verifiable(TypeMoq.Times.never()); + browserService + .setup(b => b.launch('https://aka.ms/AA66i8f')) + .returns(() => undefined) + .verifiable(TypeMoq.Times.never()); await condaInheritEnvPrompt.promptAndUpdate(); verify(persistentStateFactory.createGlobalPersistentState(condaInheritEnvPromptKey, true)).once(); verifyAll(); workspaceConfig.verifyAll(); notificationPromptEnabled.verifyAll(); + browserService.verifyAll(); }); test('Disable notification prompt if `No` is selected', async () => { const workspaceConfig = TypeMoq.Mock.ofType(); @@ -367,11 +382,49 @@ suite('Conda Inherit Env Prompt', async () => { .setup(n => n.updateValue(false)) .returns(() => Promise.resolve(undefined)) .verifiable(TypeMoq.Times.once()); + browserService + .setup(b => b.launch('https://aka.ms/AA66i8f')) + .returns(() => undefined) + .verifiable(TypeMoq.Times.never()); + await condaInheritEnvPrompt.promptAndUpdate(); + verify(persistentStateFactory.createGlobalPersistentState(condaInheritEnvPromptKey, true)).once(); + verifyAll(); + workspaceConfig.verifyAll(); + notificationPromptEnabled.verifyAll(); + browserService.verifyAll(); + }); + test('Launch browser if `More info` option is selected', async () => { + const workspaceConfig = TypeMoq.Mock.ofType(); + notificationPromptEnabled + .setup(n => n.value) + .returns(() => true) + .verifiable(TypeMoq.Times.once()); + appShell + .setup(a => a.showInformationMessage(Interpreters.condaInheritEnvMessage(), ...prompts)) + .returns(() => Promise.resolve(Common.moreInfo())) + .verifiable(TypeMoq.Times.once()); + workspaceService + .setup(ws => ws.getConfiguration('terminal')) + .returns(() => workspaceConfig.object) + .verifiable(TypeMoq.Times.never()); + workspaceConfig + .setup(wc => wc.update('integrated.inheritEnv', false, ConfigurationTarget.Global)) + .returns(() => Promise.resolve()) + .verifiable(TypeMoq.Times.never()); + notificationPromptEnabled + .setup(n => n.updateValue(false)) + .returns(() => Promise.resolve(undefined)) + .verifiable(TypeMoq.Times.never()); + browserService + .setup(b => b.launch('https://aka.ms/AA66i8f')) + .returns(() => undefined) + .verifiable(TypeMoq.Times.once()); await condaInheritEnvPrompt.promptAndUpdate(); verify(persistentStateFactory.createGlobalPersistentState(condaInheritEnvPromptKey, true)).once(); verifyAll(); workspaceConfig.verifyAll(); notificationPromptEnabled.verifyAll(); + browserService.verifyAll(); }); }); }); From ee74969e09433ea2b5e61aa3962f6166bfdbfdc2 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Date: Thu, 10 Oct 2019 14:36:40 -0700 Subject: [PATCH 2/4] Don't prompt to update inheritEnv when on Windows (#7901) * Don't prompt inheritEnv on windows * Add verification for undefined workspace config * Why did i commit that formatting change --- package.nls.json | 2 +- src/client/common/utils/localize.ts | 2 +- .../virtualEnvs/condaInheritEnvPrompt.ts | 7 +- .../condaInheritEnvPrompt.unit.test.ts | 103 ++++++++++++++++-- 4 files changed, 103 insertions(+), 11 deletions(-) diff --git a/package.nls.json b/package.nls.json index ed684a6544db..e5b6aeccaaf2 100644 --- a/package.nls.json +++ b/package.nls.json @@ -121,7 +121,7 @@ "Experiments.inGroup": "User belongs to experiment group '{0}'", "Interpreters.RefreshingInterpreters": "Refreshing Python Interpreters", "Interpreters.LoadingInterpreters": "Loading Python Interpreters", - "Interpreters.condaInheritEnvMessage": "We noticed you're using a conda environment. If you are experiencing issues with this environment in the integrated terminal, we suggest the \"terminal.integrated.inheritEnv\" setting to be changed to false. Would you like to update this setting?", + "Interpreters.condaInheritEnvMessage": "We noticed you're using a conda environment. If you are experiencing issues with this environment in the integrated terminal, we recommend that you let the Python extension change \"terminal.integrated.inheritEnv\" to false in your user settings.", "Logging.CurrentWorkingDirectory": "cwd:", "Common.doNotShowAgain": "Do not show again", "Common.reload": "Reload", diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 7137b4c97e07..2331ec5766fc 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -61,7 +61,7 @@ export namespace Experiments { export namespace Interpreters { export const loading = localize('Interpreters.LoadingInterpreters', 'Loading Python Interpreters'); export const refreshing = localize('Interpreters.RefreshingInterpreters', 'Refreshing Python Interpreters'); - export const condaInheritEnvMessage = localize('Interpreters.condaInheritEnvMessage', 'We noticed you\'re using a conda environment. If you are experiencing issues with this environment in the integrated terminal, we suggest the \"terminal.integrated.inheritEnv\" setting to be changed to false. Would you like to update this setting?'); + export const condaInheritEnvMessage = localize('Interpreters.condaInheritEnvMessage', 'We noticed you\'re using a conda environment. If you are experiencing issues with this environment in the integrated terminal, we recommend that you let the Python extension change \"terminal.integrated.inheritEnv\" to false in your user settings.'); export const environmentPromptMessage = localize('Interpreters.environmentPromptMessage', 'We noticed a new virtual environment has been created. Do you want to select it for the workspace folder?'); export const selectInterpreterTip = localize('Interpreters.selectInterpreterTip', 'Tip: you can change the Python interpreter used by the Python extension by clicking on the Python version in the status bar'); } diff --git a/src/client/interpreter/virtualEnvs/condaInheritEnvPrompt.ts b/src/client/interpreter/virtualEnvs/condaInheritEnvPrompt.ts index ea5dc824dd43..214827f6b760 100644 --- a/src/client/interpreter/virtualEnvs/condaInheritEnvPrompt.ts +++ b/src/client/interpreter/virtualEnvs/condaInheritEnvPrompt.ts @@ -6,6 +6,7 @@ import { ConfigurationTarget, Uri } from 'vscode'; import { IExtensionActivationService } from '../../activation/types'; import { IApplicationShell, IWorkspaceService } from '../../common/application/types'; import { traceDecorators, traceError } from '../../common/logger'; +import { IPlatformService } from '../../common/platform/types'; import { IBrowserService, IPersistentStateFactory } from '../../common/types'; import { Common, InteractiveShiftEnterBanner, Interpreters } from '../../common/utils/localize'; import { sendTelemetryEvent } from '../../telemetry'; @@ -22,8 +23,9 @@ export class CondaInheritEnvPrompt implements IExtensionActivationService { @inject(IBrowserService) private browserService: IBrowserService, @inject(IApplicationShell) private readonly appShell: IApplicationShell, @inject(IPersistentStateFactory) private readonly persistentStateFactory: IPersistentStateFactory, + @inject(IPlatformService) private readonly platformService: IPlatformService, @optional() public hasPromptBeenShownInCurrentSession: boolean = false - ) { } + ) {} public async activate(resource: Uri): Promise { this.initializeInBackground(resource).ignoreErrors(); @@ -65,6 +67,9 @@ export class CondaInheritEnvPrompt implements IExtensionActivationService { if (this.hasPromptBeenShownInCurrentSession) { return false; } + if (this.platformService.isWindows) { + return false; + } const interpreter = await this.interpreterService.getActiveInterpreter(resource); if (!interpreter || interpreter.type !== InterpreterType.Conda) { return false; diff --git a/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts b/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts index 0b469ae9300b..9f4981fb1ab8 100644 --- a/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts +++ b/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts @@ -10,6 +10,7 @@ import * as TypeMoq from 'typemoq'; import { ConfigurationTarget, Uri, WorkspaceConfiguration } from 'vscode'; import { IApplicationShell, IWorkspaceService } from '../../../client/common/application/types'; import { PersistentStateFactory } from '../../../client/common/persistentState'; +import { IPlatformService } from '../../../client/common/platform/types'; import { IBrowserService, IPersistentState, IPersistentStateFactory } from '../../../client/common/types'; import { createDeferred, createDeferredFromPromise, sleep } from '../../../client/common/utils/async'; import { Common, InteractiveShiftEnterBanner, Interpreters } from '../../../client/common/utils/localize'; @@ -24,6 +25,7 @@ suite('Conda Inherit Env Prompt', async () => { let workspaceService: TypeMoq.IMock; let appShell: TypeMoq.IMock; let interpreterService: TypeMoq.IMock; + let platformService: TypeMoq.IMock; let browserService: TypeMoq.IMock; let persistentStateFactory: IPersistentStateFactory; let notificationPromptEnabled: TypeMoq.IMock>; @@ -41,10 +43,26 @@ suite('Conda Inherit Env Prompt', async () => { browserService = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); persistentStateFactory = mock(PersistentStateFactory); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); + platformService = TypeMoq.Mock.ofType(); + condaInheritEnvPrompt = new CondaInheritEnvPrompt( + interpreterService.object, + workspaceService.object, + browserService.object, + appShell.object, + instance(persistentStateFactory), + platformService.object + ); }); test('Returns false if prompt has already been shown in the current session', async () => { - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory), true); + condaInheritEnvPrompt = new CondaInheritEnvPrompt( + interpreterService.object, + workspaceService.object, + browserService.object, + appShell.object, + instance(persistentStateFactory), + platformService.object, + true + ); const workspaceConfig = TypeMoq.Mock.ofType(); interpreterService .setup(is => is.getActiveInterpreter(resource)) @@ -59,11 +77,25 @@ suite('Conda Inherit Env Prompt', async () => { expect(condaInheritEnvPrompt.hasPromptBeenShownInCurrentSession).to.equal(true, 'Should be true'); verifyAll(); }); + test('Returns false if on Windows', async () => { + platformService + .setup(ps => ps.isWindows) + .returns(() => true) + .verifiable(TypeMoq.Times.once()); + const result = await condaInheritEnvPrompt.shouldShowPrompt(resource); + expect(result).to.equal(false, 'Prompt should not be shown'); + expect(condaInheritEnvPrompt.hasPromptBeenShownInCurrentSession).to.equal(false, 'Should be false'); + verifyAll(); + }); test('Returns false if active interpreter is not of type Conda', async () => { const interpreter = { type: InterpreterType.Pipenv }; const workspaceConfig = TypeMoq.Mock.ofType(); + platformService + .setup(ps => ps.isWindows) + .returns(() => false) + .verifiable(TypeMoq.Times.once()); interpreterService .setup(is => is.getActiveInterpreter(resource)) .returns(() => Promise.resolve(interpreter) as any) @@ -79,6 +111,10 @@ suite('Conda Inherit Env Prompt', async () => { }); test('Returns false if no active interpreter is present', async () => { const workspaceConfig = TypeMoq.Mock.ofType(); + platformService + .setup(ps => ps.isWindows) + .returns(() => false) + .verifiable(TypeMoq.Times.once()); interpreterService .setup(is => is.getActiveInterpreter(resource)) .returns(() => Promise.resolve(undefined)) @@ -97,6 +133,10 @@ suite('Conda Inherit Env Prompt', async () => { type: InterpreterType.Conda }; const workspaceConfig = TypeMoq.Mock.ofType(); + platformService + .setup(ps => ps.isWindows) + .returns(() => false) + .verifiable(TypeMoq.Times.once()); interpreterService .setup(is => is.getActiveInterpreter(resource)) .returns(() => Promise.resolve(interpreter) as any) @@ -107,7 +147,8 @@ suite('Conda Inherit Env Prompt', async () => { .verifiable(TypeMoq.Times.once()); workspaceConfig .setup(ws => ws.inspect('integrated.inheritEnv')) - .returns(() => undefined); + .returns(() => undefined) + .verifiable(TypeMoq.Times.once()); const result = await condaInheritEnvPrompt.shouldShowPrompt(resource); expect(result).to.equal(false, 'Prompt should not be shown'); expect(condaInheritEnvPrompt.hasPromptBeenShownInCurrentSession).to.equal(false, 'Should be false'); @@ -144,6 +185,10 @@ suite('Conda Inherit Env Prompt', async () => { type: InterpreterType.Conda }; const workspaceConfig = TypeMoq.Mock.ofType(); + platformService + .setup(ps => ps.isWindows) + .returns(() => false) + .verifiable(TypeMoq.Times.once()); interpreterService .setup(is => is.getActiveInterpreter(resource)) .returns(() => Promise.resolve(interpreter) as any) @@ -171,6 +216,10 @@ suite('Conda Inherit Env Prompt', async () => { workspaceFolderValue: undefined }; const workspaceConfig = TypeMoq.Mock.ofType(); + platformService + .setup(ps => ps.isWindows) + .returns(() => false) + .verifiable(TypeMoq.Times.once()); interpreterService .setup(is => is.getActiveInterpreter(resource)) .returns(() => Promise.resolve(interpreter) as any) @@ -196,6 +245,7 @@ suite('Conda Inherit Env Prompt', async () => { browserService = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); persistentStateFactory = mock(PersistentStateFactory); + platformService = TypeMoq.Mock.ofType(); }); teardown(() => { @@ -206,7 +256,14 @@ suite('Conda Inherit Env Prompt', async () => { const initializeInBackgroundDeferred = createDeferred(); initializeInBackground = sinon.stub(CondaInheritEnvPrompt.prototype, 'initializeInBackground'); initializeInBackground.callsFake(() => initializeInBackgroundDeferred.promise); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt( + interpreterService.object, + workspaceService.object, + browserService.object, + appShell.object, + instance(persistentStateFactory), + platformService.object + ); const promise = condaInheritEnvPrompt.activate(resource); const deferred = createDeferredFromPromise(promise); @@ -223,7 +280,14 @@ suite('Conda Inherit Env Prompt', async () => { test('Ignores errors raised by initializeInBackground()', async () => { initializeInBackground = sinon.stub(CondaInheritEnvPrompt.prototype, 'initializeInBackground'); initializeInBackground.rejects(new Error('Kaboom')); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt( + interpreterService.object, + workspaceService.object, + browserService.object, + appShell.object, + instance(persistentStateFactory), + platformService.object + ); await condaInheritEnvPrompt.activate(resource); assert.ok(initializeInBackground.calledOnce); }); @@ -238,6 +302,7 @@ suite('Conda Inherit Env Prompt', async () => { browserService = TypeMoq.Mock.ofType(); interpreterService = TypeMoq.Mock.ofType(); persistentStateFactory = mock(PersistentStateFactory); + platformService = TypeMoq.Mock.ofType(); }); teardown(() => { @@ -249,7 +314,14 @@ suite('Conda Inherit Env Prompt', async () => { shouldShowPrompt.callsFake(() => Promise.resolve(true)); promptAndUpdate = sinon.stub(CondaInheritEnvPrompt.prototype, 'promptAndUpdate'); promptAndUpdate.callsFake(() => Promise.resolve(undefined)); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt( + interpreterService.object, + workspaceService.object, + browserService.object, + appShell.object, + instance(persistentStateFactory), + platformService.object + ); await condaInheritEnvPrompt.initializeInBackground(resource); assert.ok(shouldShowPrompt.calledOnce); assert.ok(promptAndUpdate.calledOnce); @@ -260,7 +332,14 @@ suite('Conda Inherit Env Prompt', async () => { shouldShowPrompt.callsFake(() => Promise.resolve(false)); promptAndUpdate = sinon.stub(CondaInheritEnvPrompt.prototype, 'promptAndUpdate'); promptAndUpdate.callsFake(() => Promise.resolve(undefined)); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt( + interpreterService.object, + workspaceService.object, + browserService.object, + appShell.object, + instance(persistentStateFactory), + platformService.object + ); await condaInheritEnvPrompt.initializeInBackground(resource); assert.ok(shouldShowPrompt.calledOnce); assert.ok(promptAndUpdate.notCalled); @@ -276,8 +355,16 @@ suite('Conda Inherit Env Prompt', async () => { persistentStateFactory = mock(PersistentStateFactory); browserService = TypeMoq.Mock.ofType(); notificationPromptEnabled = TypeMoq.Mock.ofType>(); + platformService = TypeMoq.Mock.ofType(); when(persistentStateFactory.createGlobalPersistentState(condaInheritEnvPromptKey, true)).thenReturn(notificationPromptEnabled.object); - condaInheritEnvPrompt = new CondaInheritEnvPrompt(interpreterService.object, workspaceService.object, browserService.object, appShell.object, instance(persistentStateFactory)); + condaInheritEnvPrompt = new CondaInheritEnvPrompt( + interpreterService.object, + workspaceService.object, + browserService.object, + appShell.object, + instance(persistentStateFactory), + platformService.object + ); }); test('Does not display prompt if it is disabled', async () => { From 13831096865739e754449f16bf8f29c91164111c Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Date: Thu, 10 Oct 2019 14:36:47 -0700 Subject: [PATCH 3/4] Don't use the PTVSD version in the folder name for the wheel experiment (#7876) --- news/2 Fixes/7836.md | 1 + pythonFiles/install_ptvsd.py | 5 ++- pythonFiles/ptvsd_folder_name.py | 18 +-------- pythonFiles/tests/__init__.py | 1 - .../debug_adapter/test_ptvsd_folder_name.py | 38 +++---------------- .../test_ptvsd_folder_name_functional.py | 11 +----- 6 files changed, 13 insertions(+), 61 deletions(-) create mode 100644 news/2 Fixes/7836.md diff --git a/news/2 Fixes/7836.md b/news/2 Fixes/7836.md new file mode 100644 index 000000000000..aea13d8ebb88 --- /dev/null +++ b/news/2 Fixes/7836.md @@ -0,0 +1 @@ +Do not use the PTVSD package version in the folder name for the wheel experiment. \ No newline at end of file diff --git a/pythonFiles/install_ptvsd.py b/pythonFiles/install_ptvsd.py index 5521b4e8dd4d..2fbdb0379fc8 100644 --- a/pythonFiles/install_ptvsd.py +++ b/pythonFiles/install_ptvsd.py @@ -40,7 +40,10 @@ def install_ptvsd(): # Download only if it's a 3.7 wheel. if not wheel_info["python_version"].endswith(("37", "3.7")): continue - filename = wheel_info["filename"].rpartition(".")[0] # Trim the file extension. + + # Trim the file extension and remove the ptvsd version from the folder name. + filename = wheel_info["filename"].rpartition(".")[0] + filename = filename.replace(f"{version}-", "") ptvsd_path = path.join(PYTHONFILES, filename) with urllib.request.urlopen(wheel_info["url"]) as wheel_response: diff --git a/pythonFiles/ptvsd_folder_name.py b/pythonFiles/ptvsd_folder_name.py index 8b8dce53c52a..2d6a56473939 100644 --- a/pythonFiles/ptvsd_folder_name.py +++ b/pythonFiles/ptvsd_folder_name.py @@ -6,11 +6,9 @@ ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) PYTHONFILES = os.path.join(ROOT, "pythonFiles", "lib", "python") -REQUIREMENTS = os.path.join(ROOT, "requirements.txt") sys.path.insert(0, PYTHONFILES) -from packaging.requirements import Requirement from packaging.tags import sys_tags sys.path.remove(PYTHONFILES) @@ -19,23 +17,9 @@ def ptvsd_folder_name(): """Return the folder name for the bundled PTVSD wheel compatible with the new debug adapter.""" - with open(REQUIREMENTS, "r", encoding="utf-8") as reqsfile: - for line in reqsfile: - pkgreq = Requirement(line) - if pkgreq.name == "ptvsd": - specs = pkgreq.specifier - try: - spec, = specs - version = spec.version - except: - # Fallpack to use base PTVSD path. - print(PYTHONFILES, end="") - return - break - try: for tag in sys_tags(): - folder_name = f"ptvsd-{version}-{tag.interpreter}-{tag.abi}-{tag.platform}" + folder_name = f"ptvsd-{tag.interpreter}-{tag.abi}-{tag.platform}" folder_path = os.path.join(PYTHONFILES, folder_name) if os.path.exists(folder_path): print(folder_path, end="") diff --git a/pythonFiles/tests/__init__.py b/pythonFiles/tests/__init__.py index a400865e05bd..e2e6976acd68 100644 --- a/pythonFiles/tests/__init__.py +++ b/pythonFiles/tests/__init__.py @@ -10,4 +10,3 @@ DEBUG_ADAPTER_ROOT = os.path.join(SRC_ROOT, "debug_adapter") PYTHONFILES = os.path.join(SRC_ROOT, "lib", "python") -REQUIREMENTS = os.path.join(PROJECT_ROOT, "requirements.txt") diff --git a/pythonFiles/tests/debug_adapter/test_ptvsd_folder_name.py b/pythonFiles/tests/debug_adapter/test_ptvsd_folder_name.py index e823d74d8e95..4bebd2eb5b48 100644 --- a/pythonFiles/tests/debug_adapter/test_ptvsd_folder_name.py +++ b/pythonFiles/tests/debug_adapter/test_ptvsd_folder_name.py @@ -15,55 +15,28 @@ from packaging.tags import sys_tags from ptvsd_folder_name import ptvsd_folder_name -from .. import PYTHONFILES, REQUIREMENTS - - -def open_requirements_with_ptvsd(): - return patch( - "ptvsd_folder_name.open", mock_open(read_data="jedi==0.15.1\nptvsd==5.0.0") - ) - - -def open_requirements_without_ptvsd(): - return patch("ptvsd_folder_name.open", mock_open(read_data="jedi==0.15.1\n")) +from .. import PYTHONFILES class TestPtvsdFolderName: """Unit tests for the script retrieving the PTVSD folder name for the PTVSD wheels experiment.""" - def test_requirement_exists_folder_exists(self, capsys): + def test_folder_exists(self, capsys): # Return the first constructed folder path as existing. patcher = patch("os.path.exists") mock_exists = patcher.start() mock_exists.side_effect = lambda p: True tag = next(sys_tags()) - folder = "ptvsd-5.0.0-{}-{}-{}".format(tag.interpreter, tag.abi, tag.platform) + folder = "ptvsd-{}-{}-{}".format(tag.interpreter, tag.abi, tag.platform) - with open_requirements_with_ptvsd(): - ptvsd_folder_name() + ptvsd_folder_name() patcher.stop() expected = os.path.join(PYTHONFILES, folder) captured = capsys.readouterr() assert captured.out == expected - def test_ptvsd_requirement_once(self): - reqs = [ - line - for line in open(REQUIREMENTS, "r", encoding="utf-8") - if re.match("ptvsd==", line) - ] - assert len(reqs) == 1 - - def test_no_ptvsd_requirement(self, capsys): - with open_requirements_without_ptvsd() as p: - ptvsd_folder_name() - - expected = PYTHONFILES - captured = capsys.readouterr() - assert captured.out == expected - def test_no_wheel_folder(self, capsys): # Return none of of the constructed paths as existing, # ptvsd_folder_name() should return the path to default ptvsd. @@ -71,8 +44,7 @@ def test_no_wheel_folder(self, capsys): mock_no_exist = patcher.start() mock_no_exist.side_effect = lambda p: False - with open_requirements_with_ptvsd() as p: - ptvsd_folder_name() + ptvsd_folder_name() patcher.stop() expected = PYTHONFILES diff --git a/pythonFiles/tests/debug_adapter/test_ptvsd_folder_name_functional.py b/pythonFiles/tests/debug_adapter/test_ptvsd_folder_name_functional.py index 32231a74f744..db7512e84f51 100644 --- a/pythonFiles/tests/debug_adapter/test_ptvsd_folder_name_functional.py +++ b/pythonFiles/tests/debug_adapter/test_ptvsd_folder_name_functional.py @@ -13,22 +13,15 @@ import subprocess from packaging.requirements import Requirement -from .. import PYTHONFILES, REQUIREMENTS, SRC_ROOT +from .. import PYTHONFILES, SRC_ROOT ARGV = ["python", os.path.join(SRC_ROOT, "ptvsd_folder_name.py")] -PREFIX = "ptvsd==" - -with open(REQUIREMENTS, "r", encoding="utf-8") as reqsfile: - for line in reqsfile: - if line.startswith(PREFIX): - VERSION = line[len(PREFIX) :].strip() - break def ptvsd_paths(*platforms): paths = set() for platform in platforms: - folder = "ptvsd-{}-cp37-cp37m-{}".format(VERSION, platform) + folder = "ptvsd-cp37-cp37m-{}".format(platform) paths.add(os.path.join(PYTHONFILES, folder)) return paths From 1f53413a88b529fc055c63d37df85af0bbf2a314 Mon Sep 17 00:00:00 2001 From: Luciana Abud <45497113+luabud@users.noreply.github.com> Date: Tue, 15 Oct 2019 15:35:22 -0700 Subject: [PATCH 4/4] Disable new debug adapter experiments (#7996) * Disabling new debug adapter experiments * Apply suggestions from code review for turning off debug adapter experiment --- experiments.json | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/experiments.json b/experiments.json index 5133c4cb8a71..c6327eeba69d 100644 --- a/experiments.json +++ b/experiments.json @@ -32,26 +32,26 @@ { "name": "DebugAdapterFactory - control", "salt": "DebugAdapterFactory", - "min": 80, - "max": 90 + "min": 0, + "max": 0 }, { "name": "DebugAdapterFactory - experiment", "salt": "DebugAdapterFactory", - "min": 10, - "max": 20 + "min": 0, + "max": 0 }, { "name": "PtvsdWheels37 - control", "salt": "DebugAdapterFactory", - "min": 87, - "max": 90 + "min": 0, + "max": 0 }, { "name": "PtvsdWheels37 - experiment", "salt": "DebugAdapterFactory", - "min": 10, - "max": 13 + "min": 0, + "max": 0 }, { "name": "LS - enabled",