From b9b5e2592dd3f02e0dbeb9711058ede250cc753d Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Thu, 10 Oct 2019 11:25:48 -0700 Subject: [PATCH 1/3] Don't prompt inheritEnv on windows --- package.nls.json | 2 +- src/client/common/utils/localize.ts | 2 +- .../virtualEnvs/condaInheritEnvPrompt.ts | 7 +- .../condaInheritEnvPrompt.unit.test.ts | 104 ++++++++++++++++-- 4 files changed, 102 insertions(+), 13 deletions(-) diff --git a/package.nls.json b/package.nls.json index 56b345fc09e5..214b992680d4 100644 --- a/package.nls.json +++ b/package.nls.json @@ -126,7 +126,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 f8e407d90067..da6e41fb2c5e 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..cc394816bd10 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) @@ -144,6 +184,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 +215,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) @@ -179,9 +227,7 @@ suite('Conda Inherit Env Prompt', async () => { .setup(ws => ws.getConfiguration('terminal', resource)) .returns(() => workspaceConfig.object) .verifiable(TypeMoq.Times.once()); - workspaceConfig - .setup(ws => ws.inspect('integrated.inheritEnv')) - .returns(() => settings as any); + workspaceConfig.setup(ws => ws.inspect('integrated.inheritEnv')).returns(() => settings as any); const result = await condaInheritEnvPrompt.shouldShowPrompt(resource); expect(result).to.equal(true, 'Prompt should be shown'); expect(condaInheritEnvPrompt.hasPromptBeenShownInCurrentSession).to.equal(true, 'Should be true'); @@ -196,6 +242,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 +253,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 +277,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 +299,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 +311,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 +329,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 +352,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 37a39e34978bf4f775f4738b97e3c81eda0fdf24 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Thu, 10 Oct 2019 11:28:00 -0700 Subject: [PATCH 2/3] Add verification for undefined workspace config --- .../virtualEnvs/condaInheritEnvPrompt.unit.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts b/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts index cc394816bd10..9d7913b67a0c 100644 --- a/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts +++ b/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts @@ -147,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'); From bd549d84a6c9c3b9164b14f059016c2c1edf64a3 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Thu, 10 Oct 2019 11:31:16 -0700 Subject: [PATCH 3/3] Why did i commit that formatting change --- .../virtualEnvs/condaInheritEnvPrompt.unit.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts b/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts index 9d7913b67a0c..9f4981fb1ab8 100644 --- a/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts +++ b/src/test/interpreters/virtualEnvs/condaInheritEnvPrompt.unit.test.ts @@ -228,7 +228,9 @@ suite('Conda Inherit Env Prompt', async () => { .setup(ws => ws.getConfiguration('terminal', resource)) .returns(() => workspaceConfig.object) .verifiable(TypeMoq.Times.once()); - workspaceConfig.setup(ws => ws.inspect('integrated.inheritEnv')).returns(() => settings as any); + workspaceConfig + .setup(ws => ws.inspect('integrated.inheritEnv')) + .returns(() => settings as any); const result = await condaInheritEnvPrompt.shouldShowPrompt(resource); expect(result).to.equal(true, 'Prompt should be shown'); expect(condaInheritEnvPrompt.hasPromptBeenShownInCurrentSession).to.equal(true, 'Should be true');