fix incorrect argument in pipenv error message (#5254)

This adds some additional checks when detecting the python interpreter specified by pipenv. The checks allow us to give more useful error messages to the user.

The first check runs pipenv --version just to see if the pipenv command can be found on the system. If not, the user is notified and a suggestion on how to fix the problem is made.

The second check tests to see if the virtual environment used by pipenv has been setup yet. If not, it is suggested to the user to set it up manually.

The old error message that showed the full output of the failed pipenv command has been removed (it is still loged though).

news/2 Fixes/4866.md

@@ -0,0 +1 @@
+Improve pipenv error messages (thanks [David Lechner](https://github.com/dlech))

src/client/interpreter/locators/services/pipEnvService.ts

@@ -98,6 +98,25 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
             return;
         }
         try {
+            // call pipenv --version just to see if pipenv is in the PATH
+            const version = await this.invokePipenv('--version', cwd);
+            if (version === undefined) {
+                const appShell = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
+                appShell.showWarningMessage(
+                    `Workspace contains Pipfile but '${this.executable}' was not found. Make sure '${this.executable}' is on the PATH.`
+                );
+                return;
+            }
+            // The --py command will fail if the virtual environment has not been setup yet.
+            // so call pipenv --venv to check for the virtual environment first.
+            const venv = await this.invokePipenv('--venv', cwd);
+            if (venv === undefined) {
+                const appShell = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
+                appShell.showWarningMessage(
+                    'Workspace contains Pipfile but the associated virtual environment has not been setup. Setup the virtual environment manually if needed.'
+                );
+                return;
+            }
             const pythonPath = await this.invokePipenv('--py', cwd);
             return pythonPath && (await this.fs.fileExists(pythonPath)) ? pythonPath : undefined;
             // tslint:disable-next-line:no-empty
@@ -106,11 +125,6 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
             if (ignoreErrors) {
                 return;
             }
-            const errorMessage = error.message || error;
-            const appShell = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
-            appShell.showWarningMessage(
-                `Workspace contains pipfile but attempt to run 'pipenv --py' failed with ${errorMessage}. Make sure pipenv is on the PATH.`
-            );
         }
     }
     private async checkIfPipFileExists(cwd: string): Promise<boolean> {
@@ -153,11 +167,6 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
             this.logger.logWarning(
                 `Relevant Environment Variables ${JSON.stringify(enviromentVariableValues, undefined, 4)}`
             );
-            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 '${errorMessage}'. Make sure pipenv is on the PATH.`
-            );
         }
     }
 }

src/test/interpreters/pipEnvService.unit.test.ts

@@ -123,25 +123,28 @@ suite('Interpreters - PipEnv', () => {
                 expect(environments).to.be.deep.equal([]);
                 fileSystem.verifyAll();
             });
-            test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' failes ${testSuffix}`, async () => {
+            test(`Should display warning message if there is a \'PipFile\' but \'pipenv --version\' fails ${testSuffix}`, async () => {
                 const env = {};
                 currentProcess.setup(c => c.env).returns(() => env);
-                processService.setup(p => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.reject(''));
+                processService.setup(p => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny())).returns(() => Promise.reject(''));
                 fileSystem.setup(fs => fs.fileExists(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 warningMessage = 'Workspace contains Pipfile but \'pipenv\' was not found. Make sure \'pipenv\' is on the PATH.';
+                appShell.setup(a => a.showWarningMessage(warningMessage)).returns(() => Promise.resolve('')).verifiable(TypeMoq.Times.once());
                 logger.setup(l => l.logWarning(TypeMoq.It.isAny(), TypeMoq.It.isAny())).verifiable(TypeMoq.Times.exactly(2));
                 const environments = await pipEnvService.getInterpreters(resource);
 
                 expect(environments).to.be.deep.equal([]);
                 appShell.verifyAll();
                 logger.verifyAll();
             });
-            test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' failes with stderr ${testSuffix}`, async () => {
+            test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' fails with stderr ${testSuffix}`, async () => {
                 const env = {};
                 currentProcess.setup(c => c.env).returns(() => env);
-                processService.setup(p => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stderr: 'PipEnv Failed', stdout: '' }));
+                processService.setup(p => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stderr: '', stdout: 'pipenv, version 2018.11.26' }));
+                processService.setup(p => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--venv']), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stderr: 'Aborted!', stdout: '' }));
                 fileSystem.setup(fs => fs.fileExists(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 warningMessage = 'Workspace contains Pipfile but the associated virtual environment has not been setup. Setup the virtual environment manually if needed.';
+                appShell.setup(a => a.showWarningMessage(warningMessage)).returns(() => Promise.resolve('')).verifiable(TypeMoq.Times.once());
                 logger.setup(l => l.logWarning(TypeMoq.It.isAny(), TypeMoq.It.isAny())).verifiable(TypeMoq.Times.exactly(2));
                 const environments = await pipEnvService.getInterpreters(resource);