From f50826a13b8d78bde67fc8e4f42889171fdcd2b4 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 17 Apr 2018 12:57:31 -0700 Subject: [PATCH 1/5] Handle and log errors --- .../locators/services/pipEnvService.ts | 13 ++++++++++--- src/test/interpreters/pipEnvService.test.ts | 16 ++++++++++++++-- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index c4914bace250..699ca7baea37 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -92,13 +92,20 @@ export class PipEnvService extends CacheableLocatorService { private async invokePipenv(arg: string, rootPath: string): Promise { try { const result = await this.process.exec(execName, [arg], { cwd: rootPath }); - if (result && result.stdout) { - return result.stdout.trim(); + if (result) { + const stdout = result.stdout ? result.stdout.trim() : ''; + const stderr = result.stderr ? result.stderr.trim() : ''; + if (stderr.length > 0 && stdout.length === 0) { + throw new Error(stderr); + } + return stdout; } // tslint:disable-next-line:no-empty } catch (error) { + console.error(error); + const errorMessage = error.message || error; const appShell = this.serviceContainer.get(IApplicationShell); - appShell.showWarningMessage(`Workspace contains pipfile but attempt to run 'pipenv --venv' failed with ${error}. Make sure pipenv is on the PATH.`); + appShell.showWarningMessage(`Workspace contains pipfile but attempt to run 'pipenv --venv' failed with ${errorMessage}. Make sure pipenv is on the PATH.`); } } } diff --git a/src/test/interpreters/pipEnvService.test.ts b/src/test/interpreters/pipEnvService.test.ts index 5fe4cc4f5324..4de1d53e5d0f 100644 --- a/src/test/interpreters/pipEnvService.test.ts +++ b/src/test/interpreters/pipEnvService.test.ts @@ -3,6 +3,8 @@ 'use strict'; +// tslint:disable:max-func-body-length + import { expect } from 'chai'; import * as path from 'path'; import * as TypeMoq from 'typemoq'; @@ -20,7 +22,6 @@ enum OS { Mac, Windows, Linux } -// tslint:disable-next-line:max-func-body-length suite('Interpreters - PipEnv', () => { const rootWorkspace = Uri.file(path.join('usr', 'desktop', 'wkspc1')).fsPath; EnumEx.getNamesAndValues(OS).forEach(os => { @@ -81,7 +82,7 @@ suite('Interpreters - PipEnv', () => { expect(environments).to.be.deep.equal([]); fileSystem.verifyAll(); }); - test(`Should display wanring message if there is a \'PipFile\' but \'pipenv --venv\' failes ${testSuffix}`, async () => { + test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' failes ${testSuffix}`, async () => { const env = {}; currentProcess.setup(c => c.env).returns(() => env); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.reject('')); @@ -92,6 +93,17 @@ suite('Interpreters - PipEnv', () => { expect(environments).to.be.deep.equal([]); appShell.verifyAll(); }); + test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' failes with stderr ${testSuffix}`, async () => { + const env = {}; + currentProcess.setup(c => c.env).returns(() => env); + processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stderr: 'PipEnv Failed', stdout: '' })); + fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))).returns(() => Promise.resolve(true)); + appShell.setup(a => a.showWarningMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('')).verifiable(TypeMoq.Times.once()); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.deep.equal([]); + appShell.verifyAll(); + }); test(`Should return interpreter information${testSuffix}`, async () => { const env = {}; const venvDir = 'one'; From 8afb2efa32d76111e12aca0d2ab6bf1f59936085 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 17 Apr 2018 12:58:38 -0700 Subject: [PATCH 2/5] :memo: news entry --- news/2 Fixes/1254.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/1254.md diff --git a/news/2 Fixes/1254.md b/news/2 Fixes/1254.md new file mode 100644 index 000000000000..06f1f135aa71 --- /dev/null +++ b/news/2 Fixes/1254.md @@ -0,0 +1 @@ +Dislay errors returned by the PipEnv command when identifying the corresonding environment. From 54572fab68c7d4bfdff52b9a67f71f387691224e Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 18 Apr 2018 10:58:32 -0700 Subject: [PATCH 3/5] Use env vars and tests --- .../interpreter/locators/services/pipEnvService.ts | 6 +++++- src/test/interpreters/pipEnvService.test.ts | 14 +++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index 699ca7baea37..935e075308e8 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -8,6 +8,7 @@ import { IApplicationShell, IWorkspaceService } from '../../../common/applicatio import { IFileSystem } from '../../../common/platform/types'; import { IProcessService } from '../../../common/process/types'; import { ICurrentProcess } from '../../../common/types'; +import { IEnvironmentVariablesProvider } from '../../../common/variables/types'; import { getPythonExecutable } from '../../../debugger/Common/Utils'; import { IServiceContainer } from '../../../ioc/types'; import { IInterpreterVersionService, InterpreterType, PythonInterpreter } from '../../contracts'; @@ -22,6 +23,7 @@ export class PipEnvService extends CacheableLocatorService { private readonly process: IProcessService; private readonly workspace: IWorkspaceService; private readonly fs: IFileSystem; + private readonly envVarsProvider: IEnvironmentVariablesProvider; constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { super('PipEnvService', serviceContainer); @@ -29,6 +31,7 @@ export class PipEnvService extends CacheableLocatorService { this.process = this.serviceContainer.get(IProcessService); this.workspace = this.serviceContainer.get(IWorkspaceService); this.fs = this.serviceContainer.get(IFileSystem); + this.envVarsProvider = this.serviceContainer.get(IEnvironmentVariablesProvider); } // tslint:disable-next-line:no-empty public dispose() { } @@ -91,7 +94,8 @@ export class PipEnvService extends CacheableLocatorService { private async invokePipenv(arg: string, rootPath: string): Promise { try { - const result = await this.process.exec(execName, [arg], { cwd: rootPath }); + const env = await this.envVarsProvider.getEnvironmentVariables(Uri.file(rootPath)); + const result = await this.process.exec(execName, [arg], { cwd: rootPath, env }); if (result) { const stdout = result.stdout ? result.stdout.trim() : ''; const stderr = result.stderr ? result.stderr.trim() : ''; diff --git a/src/test/interpreters/pipEnvService.test.ts b/src/test/interpreters/pipEnvService.test.ts index 4de1d53e5d0f..65545cfc7b53 100644 --- a/src/test/interpreters/pipEnvService.test.ts +++ b/src/test/interpreters/pipEnvService.test.ts @@ -3,7 +3,7 @@ 'use strict'; -// tslint:disable:max-func-body-length +// tslint:disable:max-func-body-length no-any import { expect } from 'chai'; import * as path from 'path'; @@ -14,6 +14,7 @@ import { EnumEx } from '../../client/common/enumUtils'; import { IFileSystem } from '../../client/common/platform/types'; import { IProcessService } from '../../client/common/process/types'; import { ICurrentProcess, IPersistentState, IPersistentStateFactory } from '../../client/common/types'; +import { IEnvironmentVariablesProvider } from '../../client/common/variables/types'; import { IInterpreterLocatorService, IInterpreterVersionService } from '../../client/interpreter/contracts'; import { PipEnvService } from '../../client/interpreter/locators/services/pipEnvService'; import { IServiceContainer } from '../../client/ioc/types'; @@ -36,6 +37,7 @@ suite('Interpreters - PipEnv', () => { let fileSystem: TypeMoq.IMock; let appShell: TypeMoq.IMock; let persistentStateFactory: TypeMoq.IMock; + let envVarsProvider: TypeMoq.IMock; setup(() => { serviceContainer = TypeMoq.Mock.ofType(); const workspaceService = TypeMoq.Mock.ofType(); @@ -45,6 +47,7 @@ suite('Interpreters - PipEnv', () => { appShell = TypeMoq.Mock.ofType(); currentProcess = TypeMoq.Mock.ofType(); persistentStateFactory = TypeMoq.Mock.ofType(); + envVarsProvider = TypeMoq.Mock.ofType(); // tslint:disable-next-line:no-any const persistentState = TypeMoq.Mock.ofType>(); @@ -65,6 +68,7 @@ suite('Interpreters - PipEnv', () => { serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fileSystem.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell))).returns(() => appShell.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory))).returns(() => persistentStateFactory.object); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IEnvironmentVariablesProvider))).returns(() => envVarsProvider.object); pipEnvService = new PipEnvService(serviceContainer.object); }); @@ -75,6 +79,7 @@ suite('Interpreters - PipEnv', () => { }); test(`Should return an empty list if there is a \'PipFile\'${testSuffix}`, async () => { const env = {}; + envVarsProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})).verifiable(TypeMoq.Times.once()); currentProcess.setup(c => c.env).returns(() => env); fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))).returns(() => Promise.resolve(false)).verifiable(TypeMoq.Times.once()); const environments = await pipEnvService.getInterpreters(resource); @@ -92,9 +97,11 @@ suite('Interpreters - PipEnv', () => { expect(environments).to.be.deep.equal([]); appShell.verifyAll(); + appShell.verifyAll(); }); test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' failes with stderr ${testSuffix}`, async () => { const env = {}; + envVarsProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})).verifiable(TypeMoq.Times.once()); currentProcess.setup(c => c.env).returns(() => env); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stderr: 'PipEnv Failed', stdout: '' })); fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))).returns(() => Promise.resolve(true)); @@ -102,11 +109,13 @@ suite('Interpreters - PipEnv', () => { const environments = await pipEnvService.getInterpreters(resource); expect(environments).to.be.deep.equal([]); + envVarsProvider.verifyAll(); appShell.verifyAll(); }); test(`Should return interpreter information${testSuffix}`, async () => { const env = {}; const venvDir = 'one'; + envVarsProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})).verifiable(TypeMoq.Times.once()); currentProcess.setup(c => c.env).returns(() => env); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: venvDir })); interpreterVersionService.setup(v => v.getVersion(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('xyz')); @@ -116,6 +125,7 @@ suite('Interpreters - PipEnv', () => { expect(environments).to.be.lengthOf(1); fileSystem.verifyAll(); + envVarsProvider.verifyAll(); }); test(`Should return interpreter information using PipFile defined in Env variable${testSuffix}`, async () => { const envPipFile = 'XYZ'; @@ -123,6 +133,7 @@ suite('Interpreters - PipEnv', () => { PIPENV_PIPFILE: envPipFile }; const venvDir = 'one'; + envVarsProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})).verifiable(TypeMoq.Times.once()); currentProcess.setup(c => c.env).returns(() => env); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: venvDir })); interpreterVersionService.setup(v => v.getVersion(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('xyz')); @@ -133,6 +144,7 @@ suite('Interpreters - PipEnv', () => { expect(environments).to.be.lengthOf(1); fileSystem.verifyAll(); + envVarsProvider.verifyAll(); }); }); }); From 302db9ef649a103eeac78504ca004a60a38ea207 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 18 Apr 2018 10:59:51 -0700 Subject: [PATCH 4/5] :memo: news entry --- news/3 Code Health/1248.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/3 Code Health/1248.md diff --git a/news/3 Code Health/1248.md b/news/3 Code Health/1248.md new file mode 100644 index 000000000000..0a8db4aa2ee7 --- /dev/null +++ b/news/3 Code Health/1248.md @@ -0,0 +1 @@ +Ensure custom environment variables defined in `.env` file are passed onto the `pipenv` command. From 5e6529ba22573cecc554fb9b29956bab66ec6918 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 18 Apr 2018 11:01:12 -0700 Subject: [PATCH 5/5] rename --- news/3 Code Health/{1248.md => 1428.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename news/3 Code Health/{1248.md => 1428.md} (100%) diff --git a/news/3 Code Health/1248.md b/news/3 Code Health/1428.md similarity index 100% rename from news/3 Code Health/1248.md rename to news/3 Code Health/1428.md