Log Conda not existing message as an information instead of an error (#…

…1820)

Fixes #1817
Fixes #1821

news/3 Code Health/1817.md

@@ -0,0 +1 @@
+Log Conda not existing message as an information instead of an error.

news/3 Code Health/1821.md

@@ -0,0 +1 @@
+Make use of `ILogger` to log messages instead of using `console.error`.

src/client/common/logger.ts

@@ -1,3 +1,5 @@
+// tslint:disable:no-console
+
 import { injectable } from 'inversify';
 import { ILogger } from './types';
 
@@ -19,6 +21,13 @@ export class Logger implements ILogger {
             console.warn(`${PREFIX}${message}`);
         }
     }
+    public logInformation(message: string, ex?: Error) {
+        if (ex) {
+            console.info(`${PREFIX}${message}`, ex);
+        } else {
+            console.info(`${PREFIX}${message}`);
+        }
+    }
 }
 // tslint:disable-next-line:no-any
 export function error(title: string = '', message: any) {

src/client/common/types.ts

@@ -41,6 +41,7 @@ export const ILogger = Symbol('ILogger');
 export interface ILogger {
     logError(message: string, error?: Error);
     logWarning(message: string, error?: Error);
+    logInformation(message: string, error?: Error);
 }
 
 export enum InstallerResponse {

src/client/debugger/configProviders/configurationProviderUtils.ts

@@ -9,6 +9,7 @@ import { Uri } from 'vscode';
 import { IApplicationShell } from '../../common/application/types';
 import { IFileSystem } from '../../common/platform/types';
 import { IPythonExecutionFactory } from '../../common/process/types';
+import { ILogger } from '../../common/types';
 import { IServiceContainer } from '../../ioc/types';
 import { IConfigurationProviderUtils } from './types';
 
@@ -18,9 +19,11 @@ const PSERVE_SCRIPT_FILE_NAME = 'pserve.py';
 export class ConfigurationProviderUtils implements IConfigurationProviderUtils {
     private readonly executionFactory: IPythonExecutionFactory;
     private readonly fs: IFileSystem;
+    private readonly logger: ILogger;
     constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
         this.executionFactory = this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory);
         this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
+        this.logger = this.serviceContainer.get<ILogger>(ILogger);
     }
     public async getPyramidStartupScriptFilePath(resource?: Uri): Promise<string | undefined> {
         try {
@@ -30,7 +33,7 @@ export class ConfigurationProviderUtils implements IConfigurationProviderUtils {
             return await this.fs.fileExists(pserveFilePath) ? pserveFilePath : undefined;
         } catch (ex) {
             const message = 'Unable to locate \'pserve.py\' required for debugging of Pyramid applications.';
-            console.error(message, ex);
+            this.logger.logError(message, ex);
             const app = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
             app.showErrorMessage(message);
             return;

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

@@ -142,7 +142,7 @@ export class CondaService implements ICondaService {
             // Failed because either:
             //   1. conda is not installed.
             //   2. `conda env list has changed signature.
-            this.logger.logError('Failed to get conda environment list from conda', ex);
+            this.logger.logInformation('Failed to get conda environment list from conda', ex);
         }
     }
     public getInterpreterPath(condaEnvironmentPath: string): string {

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

@@ -7,8 +7,7 @@ import { Uri } from 'vscode';
 import { IApplicationShell, IWorkspaceService } from '../../../common/application/types';
 import { IFileSystem } from '../../../common/platform/types';
 import { IProcessServiceFactory } from '../../../common/process/types';
-import { ICurrentProcess } from '../../../common/types';
-import { IEnvironmentVariablesProvider } from '../../../common/variables/types';
+import { ICurrentProcess, ILogger } from '../../../common/types';
 import { IServiceContainer } from '../../../ioc/types';
 import { IInterpreterHelper, InterpreterType, IPipEnvService, PythonInterpreter } from '../../contracts';
 import { CacheableLocatorService } from './cacheableLocatorService';
@@ -22,15 +21,15 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
     private readonly processServiceFactory: IProcessServiceFactory;
     private readonly workspace: IWorkspaceService;
     private readonly fs: IFileSystem;
-    private readonly envVarsProvider: IEnvironmentVariablesProvider;
+    private readonly logger: ILogger;
 
     constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
         super('PipEnvService', serviceContainer);
         this.helper = this.serviceContainer.get<IInterpreterHelper>(IInterpreterHelper);
         this.processServiceFactory = this.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
         this.workspace = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
         this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
-        this.envVarsProvider = this.serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider);
+        this.logger = this.serviceContainer.get<ILogger>(ILogger);
     }
     // tslint:disable-next-line:no-empty
     public dispose() { }
@@ -127,7 +126,7 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
             }
             // tslint:disable-next-line:no-empty
         } catch (error) {
-            console.error(error);
+            this.logger.logWarning('Error in invoking PipEnv', 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 '${errorMessage}'. Make sure pipenv is on the PATH.`);

src/test/debugger/configProvider/provider.test.ts

@@ -13,7 +13,7 @@ import { IApplicationShell, IDocumentManager, IWorkspaceService } from '../../..
 import { PYTHON_LANGUAGE } from '../../../client/common/constants';
 import { IFileSystem, IPlatformService } from '../../../client/common/platform/types';
 import { IPythonExecutionFactory, IPythonExecutionService } from '../../../client/common/process/types';
-import { IConfigurationService, IPythonSettings } from '../../../client/common/types';
+import { IConfigurationService, ILogger, IPythonSettings } from '../../../client/common/types';
 import { PythonDebugConfigurationProvider, PythonV2DebugConfigurationProvider } from '../../../client/debugger';
 import { DebugOptions, LaunchRequestArguments } from '../../../client/debugger/Common/Contracts';
 import { PythonLaunchDebugConfiguration } from '../../../client/debugger/configProviders/baseProvider';
@@ -32,6 +32,7 @@ import { IServiceContainer } from '../../../client/ioc/types';
         let fileSystem: TypeMoq.IMock<IFileSystem>;
         let appShell: TypeMoq.IMock<IApplicationShell>;
         let pythonExecutionService: TypeMoq.IMock<IPythonExecutionService>;
+        let logger: TypeMoq.IMock<ILogger>;
         setup(() => {
             serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
             debugProvider = new provider.class(serviceContainer.object);
@@ -46,6 +47,7 @@ import { IServiceContainer } from '../../../client/ioc/types';
             platformService = TypeMoq.Mock.ofType<IPlatformService>();
             fileSystem = TypeMoq.Mock.ofType<IFileSystem>();
             appShell = TypeMoq.Mock.ofType<IApplicationShell>();
+            logger = TypeMoq.Mock.ofType<ILogger>();
 
             pythonExecutionService = TypeMoq.Mock.ofType<IPythonExecutionService>();
             pythonExecutionService.setup((x: any) => x.then).returns(() => undefined);
@@ -58,6 +60,7 @@ import { IServiceContainer } from '../../../client/ioc/types';
             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(IConfigurationProviderUtils))).returns(() => new ConfigurationProviderUtils(serviceContainer.object));
+            serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ILogger))).returns(() => logger.object);
 
             const settings = TypeMoq.Mock.ofType<IPythonSettings>();
             settings.setup(s => s.pythonPath).returns(() => pythonPath);
@@ -350,6 +353,8 @@ import { IServiceContainer } from '../../../client/ioc/types';
                 .verifiable(TypeMoq.Times.exactly(pyramidExists && addPyramidDebugOption ? 1 : 0));
             appShell.setup(a => a.showErrorMessage(TypeMoq.It.isAny()))
                 .verifiable(TypeMoq.Times.exactly(pyramidExists || !addPyramidDebugOption ? 0 : 1));
+            logger.setup(a => a.logError(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+                .verifiable(TypeMoq.Times.exactly(pyramidExists || !addPyramidDebugOption ? 0 : 1));
             const options = addPyramidDebugOption ? { debugOptions: [DebugOptions.Pyramid], pyramid: true } : {};
 
             const debugConfig = await debugProvider.resolveDebugConfiguration!(workspaceFolder, options as any as DebugConfiguration);
@@ -366,6 +371,7 @@ import { IServiceContainer } from '../../../client/ioc/types';
             pythonExecutionService.verifyAll();
             fileSystem.verifyAll();
             appShell.verifyAll();
+            logger.verifyAll();
         }
         test('Program is set for Pyramid (windows)', async () => {
             await testPyramidConfiguration(true, false, false);

src/test/interpreters/condaService.test.ts

@@ -37,8 +37,9 @@ suite('Interpreters Conda Service', () => {
     let registryInterpreterLocatorService: TypeMoq.IMock<IInterpreterLocatorService>;
     let serviceContainer: TypeMoq.IMock<IServiceContainer>;
     let procServiceFactory: TypeMoq.IMock<IProcessServiceFactory>;
+    let logger: TypeMoq.IMock<ILogger>;
     setup(async () => {
-        const logger = TypeMoq.Mock.ofType<ILogger>();
+        logger = TypeMoq.Mock.ofType<ILogger>();
         processService = TypeMoq.Mock.ofType<IProcessService>();
         platformService = TypeMoq.Mock.ofType<IPlatformService>();
         registryInterpreterLocatorService = TypeMoq.Mock.ofType<IInterpreterLocatorService>();
@@ -415,7 +416,6 @@ suite('Interpreters Conda Service', () => {
     test('Returns conda environments when conda exists', async () => {
         const stateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
         serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory))).returns(() => stateFactory.object);
-        // tslint:disable-next-line:no-any
         const state = new MockState(undefined);
         stateFactory.setup(s => s.createGlobalPersistentState(TypeMoq.It.isValue('CONDA_ENVIRONMENTS'), TypeMoq.It.isValue(undefined))).returns(() => state);
 
@@ -425,10 +425,24 @@ suite('Interpreters Conda Service', () => {
         assert.equal(environments, undefined, 'Conda environments do not match');
     });
 
+    test('Logs information message when conda does not exist', async () => {
+        const stateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
+        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory))).returns(() => stateFactory.object);
+        const state = new MockState(undefined);
+        stateFactory.setup(s => s.createGlobalPersistentState(TypeMoq.It.isValue('CONDA_ENVIRONMENTS'), TypeMoq.It.isValue(undefined))).returns(() => state);
+
+        processService.setup(p => p.exec(TypeMoq.It.isValue('conda'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny())).returns(() => Promise.reject(new Error('Not Found')));
+        processService.setup(p => p.exec(TypeMoq.It.isValue('conda'), TypeMoq.It.isValue(['env', 'list']), TypeMoq.It.isAny())).returns(() => Promise.reject(new Error('Not Found')));
+        logger.setup(l => l.logInformation(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+            .verifiable(TypeMoq.Times.once());
+        const environments = await condaService.getCondaEnvironments(true);
+        assert.equal(environments, undefined, 'Conda environments do not match');
+        logger.verifyAll();
+    });
+
     test('Returns cached conda environments', async () => {
         const stateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
         serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory))).returns(() => stateFactory.object);
-        // tslint:disable-next-line:no-any
         const state = new MockState({ data: 'CachedInfo' });
         stateFactory.setup(s => s.createGlobalPersistentState(TypeMoq.It.isValue('CONDA_ENVIRONMENTS'), TypeMoq.It.isValue(undefined))).returns(() => state);
 
@@ -441,7 +455,6 @@ suite('Interpreters Conda Service', () => {
     test('Subsequent list of environments will be retrieved from cache', async () => {
         const stateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
         serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory))).returns(() => stateFactory.object);
-        // tslint:disable-next-line:no-any
         const state = new MockState(undefined);
         stateFactory.setup(s => s.createGlobalPersistentState(TypeMoq.It.isValue('CONDA_ENVIRONMENTS'), TypeMoq.It.isValue(undefined))).returns(() => state);
 

src/test/interpreters/pipEnvService.test.ts

@@ -13,7 +13,7 @@ import { IApplicationShell, IWorkspaceService } from '../../client/common/applic
 import { EnumEx } from '../../client/common/enumUtils';
 import { IFileSystem } from '../../client/common/platform/types';
 import { IProcessService, IProcessServiceFactory } from '../../client/common/process/types';
-import { ICurrentProcess, IPersistentState, IPersistentStateFactory } from '../../client/common/types';
+import { ICurrentProcess, ILogger, IPersistentState, IPersistentStateFactory } from '../../client/common/types';
 import { IEnvironmentVariablesProvider } from '../../client/common/variables/types';
 import { IInterpreterHelper, IInterpreterLocatorService } from '../../client/interpreter/contracts';
 import { PipEnvService } from '../../client/interpreter/locators/services/pipEnvService';
@@ -39,6 +39,7 @@ suite('Interpreters - PipEnv', () => {
             let persistentStateFactory: TypeMoq.IMock<IPersistentStateFactory>;
             let envVarsProvider: TypeMoq.IMock<IEnvironmentVariablesProvider>;
             let procServiceFactory: TypeMoq.IMock<IProcessServiceFactory>;
+            let logger: TypeMoq.IMock<ILogger>;
             setup(() => {
                 serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
                 const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
@@ -50,6 +51,7 @@ suite('Interpreters - PipEnv', () => {
                 persistentStateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
                 envVarsProvider = TypeMoq.Mock.ofType<IEnvironmentVariablesProvider>();
                 procServiceFactory = TypeMoq.Mock.ofType<IProcessServiceFactory>();
+                logger = TypeMoq.Mock.ofType<ILogger>();
                 processService.setup((x: any) => x.then).returns(() => undefined);
                 procServiceFactory.setup(p => p.create(TypeMoq.It.isAny())).returns(() => Promise.resolve(processService.object));
 
@@ -73,6 +75,7 @@ suite('Interpreters - PipEnv', () => {
                 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);
+                serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ILogger))).returns(() => logger.object);
 
                 pipEnvService = new PipEnvService(serviceContainer.object);
             });
@@ -97,22 +100,25 @@ suite('Interpreters - PipEnv', () => {
                 processService.setup(p => p.exec(TypeMoq.It.isAny(), 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());
                 const environments = await pipEnvService.getInterpreters(resource);
 
                 expect(environments).to.be.deep.equal([]);
                 appShell.verifyAll();
-                appShell.verifyAll();
+                logger.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.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());
                 const environments = await pipEnvService.getInterpreters(resource);
 
                 expect(environments).to.be.deep.equal([]);
                 appShell.verifyAll();
+                logger.verifyAll();
             });
             test(`Should return interpreter information${testSuffix}`, async () => {
                 const env = {};