Capture and display errors returned by pipenv command (#1430)

Fixes #1254
Fixes #1428

news/2 Fixes/1254.md

@@ -0,0 +1 @@
+Dislay errors returned by the PipEnv command when identifying the corresonding environment.

news/3 Code Health/1428.md

@@ -0,0 +1 @@
+Ensure custom environment variables defined in `.env` file are passed onto the `pipenv` command.

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,13 +23,15 @@ 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);
         this.versionService = this.serviceContainer.get<IInterpreterVersionService>(IInterpreterVersionService);
         this.process = this.serviceContainer.get<IProcessService>(IProcessService);
         this.workspace = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
         this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
+        this.envVarsProvider = this.serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider);
     }
     // tslint:disable-next-line:no-empty
     public dispose() { }
@@ -91,14 +94,22 @@ export class PipEnvService extends CacheableLocatorService {
 
     private async invokePipenv(arg: string, rootPath: string): Promise<string | undefined> {
         try {
-            const result = await this.process.exec(execName, [arg], { cwd: rootPath });
-            if (result && result.stdout) {
-                return result.stdout.trim();
+            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() : '';
+                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>(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.`);
         }
     }
 }

src/test/interpreters/pipEnvService.test.ts

@@ -3,6 +3,8 @@
 
 'use strict';
 
+// tslint:disable:max-func-body-length no-any
+
 import { expect } from 'chai';
 import * as path from 'path';
 import * as TypeMoq from 'typemoq';
@@ -12,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';
@@ -20,7 +23,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 => {
@@ -35,6 +37,7 @@ suite('Interpreters - PipEnv', () => {
             let fileSystem: TypeMoq.IMock<IFileSystem>;
             let appShell: TypeMoq.IMock<IApplicationShell>;
             let persistentStateFactory: TypeMoq.IMock<IPersistentStateFactory>;
+            let envVarsProvider: TypeMoq.IMock<IEnvironmentVariablesProvider>;
             setup(() => {
                 serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
                 const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
@@ -44,6 +47,7 @@ suite('Interpreters - PipEnv', () => {
                 appShell = TypeMoq.Mock.ofType<IApplicationShell>();
                 currentProcess = TypeMoq.Mock.ofType<ICurrentProcess>();
                 persistentStateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
+                envVarsProvider = TypeMoq.Mock.ofType<IEnvironmentVariablesProvider>();
 
                 // tslint:disable-next-line:no-any
                 const persistentState = TypeMoq.Mock.ofType<IPersistentState<any>>();
@@ -64,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);
             });
@@ -74,14 +79,15 @@ 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);
 
                 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(''));
@@ -91,10 +97,25 @@ 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));
+                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([]);
+                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'));
@@ -104,13 +125,15 @@ 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';
                 const env = {
                     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'));
@@ -121,6 +144,7 @@ suite('Interpreters - PipEnv', () => {
 
                 expect(environments).to.be.lengthOf(1);
                 fileSystem.verifyAll();
+                envVarsProvider.verifyAll();
             });
         });
     });