Skip to content

Commit

Permalink
Log env info when the existence of pipenv cannot be determined (#1962)
Browse files Browse the repository at this point in the history 
Fixes #1338
  • Loading branch information
@DonJayamanne
DonJayamanne authored Jun 14, 2018
1 parent 749578a commit a93ce84
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 9 deletions.
1 change: 1 addition & 0 deletions news/3 Code Health/1338.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Log relevant environment information when the existence of `pipenv` cannot be determined.
11 changes: 10 additions & 1 deletion src/client/interpreter/locators/services/pipEnvService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { inject, injectable } from 'inversify';
import * as path from 'path';
import { Uri } from 'vscode';
import { IApplicationShell, IWorkspaceService } from '../../../common/application/types';
import { IFileSystem } from '../../../common/platform/types';
import { IFileSystem, IPlatformService } from '../../../common/platform/types';
import { IProcessServiceFactory } from '../../../common/process/types';
import { ICurrentProcess, ILogger } from '../../../common/types';
import { IServiceContainer } from '../../../ioc/types';
Expand Down Expand Up @@ -126,7 +126,16 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
}
// tslint:disable-next-line:no-empty
} catch (error) {
const platformService = this.serviceContainer.get<IPlatformService>(IPlatformService);
const currentProc = this.serviceContainer.get<ICurrentProcess>(ICurrentProcess);
const enviromentVariableValues = {
LC_ALL: currentProc.env.LC_ALL,
LANG: currentProc.env.LANG
};
enviromentVariableValues[platformService.pathVariableName] = currentProc.env[platformService.pathVariableName];

this.logger.logWarning('Error in invoking PipEnv', error);
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.`);
Expand Down
19 changes: 11 additions & 8 deletions src/test/interpreters/pipEnvService.test.ts → ...t/interpreters/pipEnvService.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import * as TypeMoq from 'typemoq';
import { Uri, WorkspaceFolder } from 'vscode';
import { IApplicationShell, IWorkspaceService } from '../../client/common/application/types';
import { EnumEx } from '../../client/common/enumUtils';
import { IFileSystem } from '../../client/common/platform/types';
import { IFileSystem, IPlatformService } from '../../client/common/platform/types';
import { IProcessService, IProcessServiceFactory } from '../../client/common/process/types';
import { ICurrentProcess, ILogger, IPersistentState, IPersistentStateFactory } from '../../client/common/types';
import { IEnvironmentVariablesProvider } from '../../client/common/variables/types';
Expand Down Expand Up @@ -40,6 +40,7 @@ suite('Interpreters - PipEnv', () => {
let envVarsProvider: TypeMoq.IMock<IEnvironmentVariablesProvider>;
let procServiceFactory: TypeMoq.IMock<IProcessServiceFactory>;
let logger: TypeMoq.IMock<ILogger>;
let platformService: TypeMoq.IMock<IPlatformService>;
setup(() => {
serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
Expand All @@ -52,6 +53,7 @@ suite('Interpreters - PipEnv', () => {
envVarsProvider = TypeMoq.Mock.ofType<IEnvironmentVariablesProvider>();
procServiceFactory = TypeMoq.Mock.ofType<IProcessServiceFactory>();
logger = TypeMoq.Mock.ofType<ILogger>();
platformService = TypeMoq.Mock.ofType<IPlatformService>();
processService.setup((x: any) => x.then).returns(() => undefined);
procServiceFactory.setup(p => p.create(TypeMoq.It.isAny())).returns(() => Promise.resolve(processService.object));

Expand All @@ -76,6 +78,7 @@ suite('Interpreters - PipEnv', () => {
serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory))).returns(() => persistentStateFactory.object);
serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IEnvironmentVariablesProvider))).returns(() => envVarsProvider.object);
serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ILogger))).returns(() => logger.object);
serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPlatformService))).returns(() => platformService.object);

pipEnvService = new PipEnvService(serviceContainer.object);
});
Expand All @@ -84,7 +87,7 @@ suite('Interpreters - PipEnv', () => {
const environments = pipEnvService.getInterpreters(resource);
expect(environments).to.be.eventually.deep.equal([]);
});
test(`Should return an empty list if there is a \'PipFile\'${testSuffix}`, async () => {
test(`Should return an empty list if there is no \'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);
Expand All @@ -97,10 +100,10 @@ suite('Interpreters - PipEnv', () => {
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(''));
processService.setup(p => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), 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());
logger.setup(l => l.logWarning(TypeMoq.It.isAny(), TypeMoq.It.isAny())).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([]);
Expand All @@ -110,10 +113,10 @@ suite('Interpreters - PipEnv', () => {
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: '' }));
processService.setup(p => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stderr: 'PipEnv Failed', 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());
logger.setup(l => l.logWarning(TypeMoq.It.isAny(), TypeMoq.It.isAny())).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([]);
Expand All @@ -125,7 +128,7 @@ suite('Interpreters - PipEnv', () => {
const pythonPath = '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: pythonPath }));
processService.setup(p => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: pythonPath }));
interpreterHelper.setup(v => v.getInterpreterInformation(TypeMoq.It.isAny())).returns(() => Promise.resolve({ version: 'xyz' }));
fileSystem.setup(fs => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))).returns(() => Promise.resolve(true)).verifiable();
fileSystem.setup(fs => fs.fileExists(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(true)).verifiable();
Expand All @@ -142,7 +145,7 @@ suite('Interpreters - PipEnv', () => {
const pythonPath = '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: pythonPath }));
processService.setup(p => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: pythonPath }));
interpreterHelper.setup(v => v.getInterpreterInformation(TypeMoq.It.isAny())).returns(() => Promise.resolve({ version: 'xyz' }));
fileSystem.setup(fs => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))).returns(() => Promise.resolve(false)).verifiable(TypeMoq.Times.never());
fileSystem.setup(fs => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, envPipFile)))).returns(() => Promise.resolve(true)).verifiable(TypeMoq.Times.once());
Expand Down

0 comments on commit a93ce84

Please sign in to comment.