Check if interpreter file exists before displaying in a list (#1352)

Fixes #1305

Author: DonJayamanne

news/2 Fixes/1305.md

@@ -0,0 +1 @@
+Ensure interpreter file exists on the file system before including into list of interpreters.

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

@@ -2,28 +2,32 @@ import { inject, injectable } from 'inversify';
 import * as _ from 'lodash';
 import * as path from 'path';
 import { Uri } from 'vscode';
-import { PythonSettings } from '../../../common/configSettings';
+import { IFileSystem } from '../../../common/platform/types';
 import { IProcessService } from '../../../common/process/types';
+import { IConfigurationService } from '../../../common/types';
 import { IServiceContainer } from '../../../ioc/types';
 import { IInterpreterVersionService, InterpreterType, PythonInterpreter } from '../../contracts';
 import { IVirtualEnvironmentManager } from '../../virtualEnvs/types';
 import { CacheableLocatorService } from './cacheableLocatorService';
 
 @injectable()
 export class CurrentPathService extends CacheableLocatorService {
+    private readonly fs: IFileSystem;
     public constructor(@inject(IVirtualEnvironmentManager) private virtualEnvMgr: IVirtualEnvironmentManager,
         @inject(IInterpreterVersionService) private versionProvider: IInterpreterVersionService,
         @inject(IProcessService) private processService: IProcessService,
         @inject(IServiceContainer) serviceContainer: IServiceContainer) {
         super('CurrentPathService', serviceContainer);
+        this.fs = serviceContainer.get<IFileSystem>(IFileSystem);
     }
     // tslint:disable-next-line:no-empty
     public dispose() { }
     protected getInterpretersImplementation(resource?: Uri): Promise<PythonInterpreter[]> {
         return this.suggestionsFromKnownPaths();
     }
     private async suggestionsFromKnownPaths(resource?: Uri) {
-        const currentPythonInterpreter = this.getInterpreter(PythonSettings.getInstance(resource).pythonPath, '').then(interpreter => [interpreter]);
+        const configSettings = this.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(resource);
+        const currentPythonInterpreter = this.getInterpreter(configSettings.pythonPath, '').then(interpreter => [interpreter]);
         const python = this.getInterpreter('python', '').then(interpreter => [interpreter]);
         const python2 = this.getInterpreter('python2', '').then(interpreter => [interpreter]);
         const python3 = this.getInterpreter('python3', '').then(interpreter => [interpreter]);
@@ -49,9 +53,15 @@ export class CurrentPathService extends CacheableLocatorService {
             });
     }
     private async getInterpreter(pythonPath: string, defaultValue: string) {
-        return this.processService.exec(pythonPath, ['-c', 'import sys;print(sys.executable)'], {})
-            .then(output => output.stdout.trim())
-            .then(value => value.length === 0 ? defaultValue : value)
-            .catch(() => defaultValue);    // Ignore exceptions in getting the executable.
+        try {
+            const output = await this.processService.exec(pythonPath, ['-c', 'import sys;print(sys.executable)'], {});
+            const executablePath = output.stdout.trim();
+            if (executablePath.length > 0 && await this.fs.fileExistsAsync(executablePath)) {
+                return executablePath;
+            }
+            return defaultValue;
+        } catch {
+            return defaultValue;    // Ignore exceptions in getting the executable.
+        }
     }
 }

src/test/interpreters/currentPathService.test.ts

@@ -0,0 +1,78 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+'use strict';
+
+import { expect } from 'chai';
+import * as TypeMoq from 'typemoq';
+import { IFileSystem } from '../../client/common/platform/types';
+import { IProcessService } from '../../client/common/process/types';
+import { IConfigurationService, IPersistentState, IPersistentStateFactory, IPythonSettings } from '../../client/common/types';
+import { IInterpreterVersionService, InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts';
+import { CurrentPathService } from '../../client/interpreter/locators/services/currentPathService';
+import { IVirtualEnvironmentManager } from '../../client/interpreter/virtualEnvs/types';
+import { IServiceContainer } from '../../client/ioc/types';
+
+// tslint:disable-next-line:max-func-body-length
+suite('Interpreters CurrentPath Service', () => {
+    let processService: TypeMoq.IMock<IProcessService>;
+    let fileSystem: TypeMoq.IMock<IFileSystem>;
+    let serviceContainer: TypeMoq.IMock<IServiceContainer>;
+    let virtualEnvironmentManager: TypeMoq.IMock<IVirtualEnvironmentManager>;
+    let interpreterVersionService: TypeMoq.IMock<IInterpreterVersionService>;
+    let pythonSettings: TypeMoq.IMock<IPythonSettings>;
+    let currentPathService: CurrentPathService;
+    let persistentState: TypeMoq.IMock<IPersistentState<PythonInterpreter[]>>;
+    setup(async () => {
+        processService = TypeMoq.Mock.ofType<IProcessService>();
+        virtualEnvironmentManager = TypeMoq.Mock.ofType<IVirtualEnvironmentManager>();
+        interpreterVersionService = TypeMoq.Mock.ofType<IInterpreterVersionService>();
+        const configurationService = TypeMoq.Mock.ofType<IConfigurationService>();
+        pythonSettings = TypeMoq.Mock.ofType<IPythonSettings>();
+        configurationService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object);
+        const persistentStateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
+        persistentState = TypeMoq.Mock.ofType<IPersistentState<PythonInterpreter[]>>();
+        // tslint:disable-next-line:no-any
+        persistentState.setup(p => p.value).returns(() => undefined as any);
+        persistentState.setup(p => p.updateValue(TypeMoq.It.isAny())).returns(() => Promise.resolve());
+        fileSystem = TypeMoq.Mock.ofType<IFileSystem>();
+        persistentStateFactory.setup(p => p.createGlobalPersistentState(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => persistentState.object);
+
+        serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
+        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IProcessService), TypeMoq.It.isAny())).returns(() => processService.object);
+        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IVirtualEnvironmentManager), TypeMoq.It.isAny())).returns(() => virtualEnvironmentManager.object);
+        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInterpreterVersionService), TypeMoq.It.isAny())).returns(() => interpreterVersionService.object);
+        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IFileSystem), TypeMoq.It.isAny())).returns(() => fileSystem.object);
+        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory), TypeMoq.It.isAny())).returns(() => persistentStateFactory.object);
+        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IConfigurationService), TypeMoq.It.isAny())).returns(() => configurationService.object);
+
+        currentPathService = new CurrentPathService(virtualEnvironmentManager.object, interpreterVersionService.object, processService.object, serviceContainer.object);
+    });
+
+    test('Interpreters that do not exist on the file system are not excluded from the list', async () => {
+        // Specific test for 1305
+        const version = 'mockVersion';
+        const envName = 'mockEnvName';
+        interpreterVersionService.setup(v => v.getVersion(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(version));
+        virtualEnvironmentManager.setup(v => v.getEnvironmentName(TypeMoq.It.isAny())).returns(() => Promise.resolve(envName));
+
+        const execArgs = ['-c', 'import sys;print(sys.executable)'];
+        pythonSettings.setup(p => p.pythonPath).returns(() => 'root:Python');
+        processService.setup(p => p.exec(TypeMoq.It.isValue('root:Python'), TypeMoq.It.isValue(execArgs), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: 'c:/root:python' })).verifiable(TypeMoq.Times.once());
+        processService.setup(p => p.exec(TypeMoq.It.isValue('python'), TypeMoq.It.isValue(execArgs), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: 'c:/python1' })).verifiable(TypeMoq.Times.once());
+        processService.setup(p => p.exec(TypeMoq.It.isValue('python2'), TypeMoq.It.isValue(execArgs), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: 'c:/python2' })).verifiable(TypeMoq.Times.once());
+        processService.setup(p => p.exec(TypeMoq.It.isValue('python3'), TypeMoq.It.isValue(execArgs), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: 'c:/python3' })).verifiable(TypeMoq.Times.once());
+
+        fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue('c:/root:python'))).returns(() => Promise.resolve(true)).verifiable(TypeMoq.Times.once());
+        fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue('c:/python1'))).returns(() => Promise.resolve(false)).verifiable(TypeMoq.Times.once());
+        fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue('c:/python2'))).returns(() => Promise.resolve(false)).verifiable(TypeMoq.Times.once());
+        fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue('c:/python3'))).returns(() => Promise.resolve(true)).verifiable(TypeMoq.Times.once());
+
+        const interpreters = await currentPathService.getInterpreters();
+        processService.verifyAll();
+        fileSystem.verifyAll();
+        expect(interpreters).to.be.of.length(2);
+        expect(interpreters).to.deep.include({ displayName: `${version} (${envName})`, path: 'c:/root:python', type: InterpreterType.VirtualEnv });
+        expect(interpreters).to.deep.include({ displayName: `${version} (${envName})`, path: 'c:/python3', type: InterpreterType.VirtualEnv });
+    });
+});