From 42ed5f399d1d93ddd2e5b5b07b92cabeda67e5ca Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 24 Jul 2018 14:01:47 +0100 Subject: [PATCH 1/7] Initial changes --- news/2 Fixes/2223.md | 1 + .../common/installer/pythonInstallation.ts | 5 +- .../configuration/interpreterSelector.ts | 17 +- src/client/interpreter/contracts.ts | 8 +- src/client/interpreter/interpreterService.ts | 10 +- src/client/interpreter/locators/helpers.ts | 52 ++++- src/client/interpreter/locators/index.ts | 36 +--- .../locators/services/currentPathService.ts | 5 +- src/client/interpreter/serviceRegistry.ts | 3 + .../locators/helpers.unit.test.ts | 177 ++++++++++++++++++ .../interpreters/locators/index.unit.test.ts | 163 ++++++++++++++++ 11 files changed, 422 insertions(+), 55 deletions(-) create mode 100644 news/2 Fixes/2223.md create mode 100644 src/test/interpreters/locators/helpers.unit.test.ts create mode 100644 src/test/interpreters/locators/index.unit.test.ts diff --git a/news/2 Fixes/2223.md b/news/2 Fixes/2223.md new file mode 100644 index 000000000000..8cdcfdc28d83 --- /dev/null +++ b/news/2 Fixes/2223.md @@ -0,0 +1 @@ +Ensure workspace `pipenv` environment is not labeled as a `virtual env`. \ No newline at end of file diff --git a/src/client/common/installer/pythonInstallation.ts b/src/client/common/installer/pythonInstallation.ts index 13af1b958104..248613439d85 100644 --- a/src/client/common/installer/pythonInstallation.ts +++ b/src/client/common/installer/pythonInstallation.ts @@ -3,7 +3,6 @@ 'use strict'; import { IInterpreterLocatorService, IInterpreterService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../interpreter/contracts'; -import { isMacDefaultPythonPath } from '../../interpreter/locators/helpers'; import { IServiceContainer } from '../../ioc/types'; import { IApplicationShell } from '../application/types'; import { IPlatformService } from '../platform/types'; @@ -25,8 +24,8 @@ export class PythonInstaller { const interpreters = await this.locator.getInterpreters(); if (interpreters.length > 0) { const platform = this.serviceContainer.get(IPlatformService); - if (platform.isMac && isMacDefaultPythonPath(settings.pythonPath)) { - const interpreterService = this.serviceContainer.get(IInterpreterService); + const interpreterService = this.serviceContainer.get(IInterpreterService); + if (platform.isMac && interpreterService.isMacDefaultPythonPath(settings.pythonPath)) { const interpreter = await interpreterService.getActiveInterpreter(); if (interpreter && interpreter.type === InterpreterType.Unknown) { await this.shell.showWarningMessage('Selected interpreter is macOS system Python which is not recommended. Please select different interpreter'); diff --git a/src/client/interpreter/configuration/interpreterSelector.ts b/src/client/interpreter/configuration/interpreterSelector.ts index 716dced6c0f7..4a490b865a39 100644 --- a/src/client/interpreter/configuration/interpreterSelector.ts +++ b/src/client/interpreter/configuration/interpreterSelector.ts @@ -4,7 +4,6 @@ import { ConfigurationTarget, Disposable, QuickPickItem, QuickPickOptions, Uri } import { IApplicationShell, ICommandManager, IDocumentManager, IWorkspaceService } from '../../common/application/types'; import * as settings from '../../common/configSettings'; import { Commands } from '../../common/constants'; -import { IFileSystem } from '../../common/platform/types'; import { IServiceContainer } from '../../ioc/types'; import { IInterpreterService, IShebangCodeLensProvider, PythonInterpreter, WorkspacePythonPath } from '../contracts'; import { IInterpreterSelector, IPythonPathUpdaterServiceManager } from './types'; @@ -20,14 +19,12 @@ export class InterpreterSelector implements IInterpreterSelector { private readonly workspaceService: IWorkspaceService; private readonly applicationShell: IApplicationShell; private readonly documentManager: IDocumentManager; - private readonly fileSystem: IFileSystem; constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { this.interpreterManager = serviceContainer.get(IInterpreterService); this.workspaceService = this.serviceContainer.get(IWorkspaceService); this.applicationShell = this.serviceContainer.get(IApplicationShell); this.documentManager = this.serviceContainer.get(IDocumentManager); - this.fileSystem = this.serviceContainer.get(IFileSystem); const commandManager = serviceContainer.get(ICommandManager); this.disposables.push(commandManager.registerCommand(Commands.Set_Interpreter, this.setInterpreter.bind(this))); @@ -38,8 +35,7 @@ export class InterpreterSelector implements IInterpreterSelector { } public async getSuggestions(resourceUri?: Uri) { - let interpreters = await this.interpreterManager.getInterpreters(resourceUri); - interpreters = await this.removeDuplicates(interpreters); + const interpreters = await this.interpreterManager.getInterpreters(resourceUri); // tslint:disable-next-line:no-non-null-assertion interpreters.sort((a, b) => a.displayName! > b.displayName! ? 1 : -1); return Promise.all(interpreters.map(item => this.suggestionToQuickPickItem(item, resourceUri))); @@ -74,17 +70,6 @@ export class InterpreterSelector implements IInterpreterSelector { }; } - private async removeDuplicates(interpreters: PythonInterpreter[]): Promise { - const result: PythonInterpreter[] = []; - interpreters.forEach(x => { - if (result.findIndex(a => a.displayName === x.displayName - && a.type === x.type && this.fileSystem.arePathsSame(path.dirname(a.path), path.dirname(x.path))) < 0) { - result.push(x); - } - }); - return result; - } - private async setInterpreter() { const setInterpreterGlobally = !Array.isArray(this.workspaceService.workspaceFolders) || this.workspaceService.workspaceFolders.length === 0; let configTarget = ConfigurationTarget.Global; diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index 6d23d28ea739..f535e47415d3 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -78,9 +78,10 @@ export interface IInterpreterService { getInterpreters(resource?: Uri): Promise; autoSetInterpreter(): Promise; getActiveInterpreter(resource?: Uri): Promise; - getInterpreterDetails(pythonPath: string): Promise>; + getInterpreterDetails(pythonPath: string): Promise>; refresh(): Promise; initialize(): void; + isMacDefaultPythonPath(pythonPath: string): Boolean; } export const IInterpreterDisplay = Symbol('IInterpreterDisplay'); @@ -103,3 +104,8 @@ export const IPipEnvService = Symbol('IPipEnvService'); export interface IPipEnvService { isRelatedPipEnvironment(dir: string, pythonPath: string): Promise; } + +export const IInterpreterLocatorHelper = Symbol('IInterpreterLocatorHelper'); +export interface IInterpreterLocatorHelper { + mergeInterpreters(interpreters: PythonInterpreter[]): PythonInterpreter[]; +} diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts index b0b69910241a..16fcf5ca3b30 100644 --- a/src/client/interpreter/interpreterService.ts +++ b/src/client/interpreter/interpreterService.ts @@ -53,7 +53,7 @@ export class InterpreterService implements Disposable, IInterpreterService { if (!activeWorkspace) { return; } - // Check pipenv first + // Check pipenv first. const pipenvService = this.serviceContainer.get(IInterpreterLocatorService, PIPENV_SERVICE); let interpreters = await pipenvService.getInterpreters(activeWorkspace.folderUri); if (interpreters.length > 0) { @@ -102,7 +102,7 @@ export class InterpreterService implements Disposable, IInterpreterService { return this.getInterpreterDetails(fullyQualifiedPath, resource); } - public async getInterpreterDetails(pythonPath: string, resource?: Uri): Promise { + public async getInterpreterDetails(pythonPath: string, resource?: Uri): Promise { const interpreters = await this.getInterpreters(resource); const interpreter = interpreters.find(i => utils.arePathsSame(i.path, pythonPath)); @@ -116,7 +116,7 @@ export class InterpreterService implements Disposable, IInterpreterService { virtualEnvManager.getEnvironmentName(pythonPath), virtualEnvManager.getEnvironmentType(pythonPath) ]); - if (details) { + if (!details) { return; } const dislayNameSuffix = virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; @@ -129,6 +129,10 @@ export class InterpreterService implements Disposable, IInterpreterService { type: type }; } + public isMacDefaultPythonPath(pythonPath: string) { + return pythonPath === 'python' || pythonPath === '/usr/bin/python'; + } + private async shouldAutoSetInterpreter(): Promise { const activeWorkspace = this.helper.getActiveWorkspaceUri(); if (!activeWorkspace) { diff --git a/src/client/interpreter/locators/helpers.ts b/src/client/interpreter/locators/helpers.ts index b5605e946384..77abdb13bbcf 100644 --- a/src/client/interpreter/locators/helpers.ts +++ b/src/client/interpreter/locators/helpers.ts @@ -1,7 +1,10 @@ +import { inject, injectable } from 'inversify'; import * as path from 'path'; import { getArchitectureDislayName } from '../../common/platform/registry'; +import { IFileSystem, IPlatformService } from '../../common/platform/types'; import { fsReaddirAsync, IS_WINDOWS } from '../../common/utils'; -import { PythonInterpreter } from '../contracts'; +import { IServiceContainer } from '../../ioc/types'; +import { IInterpreterLocatorHelper, IInterpreterService, InterpreterType, PythonInterpreter } from '../contracts'; const CheckPythonInterpreterRegEx = IS_WINDOWS ? /^python(\d+(.\d+)?)?\.exe$/ : /^python(\d+(.\d+)?)?$/; @@ -23,7 +26,50 @@ export function fixInterpreterDisplayName(item: PythonInterpreter) { } return item; } +@injectable() +export class InterpreterLocatorHelper implements IInterpreterLocatorHelper { + private readonly platform: IPlatformService; + private readonly fs: IFileSystem; + private readonly interpreterService: IInterpreterService; -export function isMacDefaultPythonPath(p: string) { - return p === 'python' || p === '/usr/bin/python'; + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + this.platform = serviceContainer.get(IPlatformService); + this.interpreterService = serviceContainer.get(IInterpreterService); + this.fs = serviceContainer.get(IFileSystem); + } + public mergeInterpreters(interpreters: PythonInterpreter[]) { + return interpreters + .map(item => { return { ...item }; }) + .map(fixInterpreterDisplayName) + .map(item => { item.path = path.normalize(item.path); return item; }) + .reduce((accumulator, current) => { + if (this.platform.isMac && this.interpreterService.isMacDefaultPythonPath(current.path)) { + return accumulator; + } + const currentVersion = Array.isArray(current.version_info) ? current.version_info.join('.') : undefined; + const existingItem = accumulator.find(item => { + if (this.fs.arePathsSame(item.path, current.path)) { + return true; + } + // If same version and same base path, then ignore. + // Could be Python 3.6 with path = python.exe, and Python 3.6 and path = python3.exe. + if (Array.isArray(item.version_info) && item.version_info.join('.') === currentVersion && + item.path && current.path && + this.fs.arePathsSame(path.basename(item.path), path.basename(current.path))) { + return true; + } + return false; + }); + if (!existingItem) { + accumulator.push(current); + } else { + // Preserve type information. + // Possible we identified environment as unknown, but a later provider has identified env type. + if (existingItem.type === InterpreterType.Unknown && current.type !== InterpreterType.Unknown) { + existingItem.type = current.type; + } + } + return accumulator; + }, []); + } } diff --git a/src/client/interpreter/locators/index.ts b/src/client/interpreter/locators/index.ts index 45fa5fb18999..c91772895e49 100644 --- a/src/client/interpreter/locators/index.ts +++ b/src/client/interpreter/locators/index.ts @@ -1,34 +1,33 @@ import { inject, injectable } from 'inversify'; import * as _ from 'lodash'; -import * as path from 'path'; import { Disposable, Uri } from 'vscode'; import { IPlatformService } from '../../common/platform/types'; import { IDisposableRegistry } from '../../common/types'; -import { arePathsSame } from '../../common/utils'; import { IServiceContainer } from '../../ioc/types'; import { CONDA_ENV_FILE_SERVICE, CONDA_ENV_SERVICE, CURRENT_PATH_SERVICE, GLOBAL_VIRTUAL_ENV_SERVICE, + IInterpreterLocatorHelper, IInterpreterLocatorService, - InterpreterType, KNOWN_PATH_SERVICE, PIPENV_SERVICE, PythonInterpreter, WINDOWS_REGISTRY_SERVICE, WORKSPACE_VIRTUAL_ENV_SERVICE } from '../contracts'; -import { fixInterpreterDisplayName, isMacDefaultPythonPath } from './helpers'; @injectable() export class PythonInterpreterLocatorService implements IInterpreterLocatorService { - private disposables: Disposable[] = []; - private platform: IPlatformService; + private readonly disposables: Disposable[] = []; + private readonly platform: IPlatformService; + private readonly interpreterLocatorHelper: IInterpreterLocatorHelper; constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { serviceContainer.get(IDisposableRegistry).push(this); this.platform = serviceContainer.get(IPlatformService); + this.interpreterLocatorHelper = serviceContainer.get(IInterpreterLocatorHelper); } public async getInterpreters(resource?: Uri): Promise { return this.getInterpretersPerResource(resource); @@ -41,27 +40,10 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi const promises = locators.map(async provider => provider.getInterpreters(resource)); const listOfInterpreters = await Promise.all(promises); - // tslint:disable-next-line:underscore-consistent-invocation - return _.flatten(listOfInterpreters) + const items = _.flatten(listOfInterpreters) .filter(item => !!item) - .map(item => item!) - .map(fixInterpreterDisplayName) - .map(item => { item.path = path.normalize(item.path); return item; }) - .reduce((accumulator, current) => { - if (this.platform.isMac && isMacDefaultPythonPath(current.path)) { - return accumulator; - } - const existingItem = accumulator.find(item => arePathsSame(item.path, current.path)); - if (!existingItem) { - accumulator.push(current); - } else { - // Preserve type information. - if (existingItem.type === InterpreterType.Unknown && current.type !== InterpreterType.Unknown) { - existingItem.type = current.type; - } - } - return accumulator; - }, []); + .map(item => item!); + return this.interpreterLocatorHelper.mergeInterpreters(items); } private getLocators(): IInterpreterLocatorService[] { const locators: IInterpreterLocatorService[] = []; @@ -74,6 +56,7 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi } locators.push(this.serviceContainer.get(IInterpreterLocatorService, CONDA_ENV_SERVICE)); locators.push(this.serviceContainer.get(IInterpreterLocatorService, CONDA_ENV_FILE_SERVICE)); + locators.push(this.serviceContainer.get(IInterpreterLocatorService, PIPENV_SERVICE)); locators.push(this.serviceContainer.get(IInterpreterLocatorService, GLOBAL_VIRTUAL_ENV_SERVICE)); locators.push(this.serviceContainer.get(IInterpreterLocatorService, WORKSPACE_VIRTUAL_ENV_SERVICE)); @@ -81,7 +64,6 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi locators.push(this.serviceContainer.get(IInterpreterLocatorService, KNOWN_PATH_SERVICE)); } locators.push(this.serviceContainer.get(IInterpreterLocatorService, CURRENT_PATH_SERVICE)); - locators.push(this.serviceContainer.get(IInterpreterLocatorService, PIPENV_SERVICE)); return locators; } diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts index a4e81dbed34d..d8b154a67fc5 100644 --- a/src/client/interpreter/locators/services/currentPathService.ts +++ b/src/client/interpreter/locators/services/currentPathService.ts @@ -35,9 +35,10 @@ export class CurrentPathService extends CacheableLocatorService { .then(listOfInterpreters => _.flatten(listOfInterpreters)) .then(interpreters => interpreters.filter(item => item.length > 0)) // tslint:disable-next-line:promise-function-async - .then(interpreters => Promise.all(interpreters.map(interpreter => this.getInterpreterDetails(interpreter, resource)))); + .then(interpreters => Promise.all(interpreters.map(interpreter => this.getInterpreterDetails(interpreter, resource)))) + .then(interpreters => interpreters.filter(item => !!item).map(item => item!)); } - private async getInterpreterDetails(interpreter: string, resource?: Uri): Promise { + private async getInterpreterDetails(interpreter: string, resource?: Uri): Promise { return Promise.all([ this.helper.getInterpreterInformation(interpreter), this.virtualEnvMgr.getEnvironmentName(interpreter), diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index f867bb2ce1ac..ddc70750f7a1 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -15,6 +15,7 @@ import { ICondaService, IInterpreterDisplay, IInterpreterHelper, + IInterpreterLocatorHelper, IInterpreterLocatorService, IInterpreterService, IInterpreterVersionService, @@ -33,6 +34,7 @@ import { ShebangCodeLensProvider } from './display/shebangCodeLensProvider'; import { InterpreterHelper } from './helpers'; import { InterpreterService } from './interpreterService'; import { InterpreterVersionService } from './interpreterVersion'; +import { InterpreterLocatorHelper } from './locators/helpers'; import { PythonInterpreterLocatorService } from './locators/index'; import { CondaEnvFileService } from './locators/services/condaEnvFileService'; import { CondaEnvService } from './locators/services/condaEnvService'; @@ -79,4 +81,5 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IInterpreterSelector, InterpreterSelector); serviceManager.addSingleton(IShebangCodeLensProvider, ShebangCodeLensProvider); serviceManager.addSingleton(IInterpreterHelper, InterpreterHelper); + serviceManager.addSingleton(IInterpreterLocatorHelper, InterpreterLocatorHelper); } diff --git a/src/test/interpreters/locators/helpers.unit.test.ts b/src/test/interpreters/locators/helpers.unit.test.ts new file mode 100644 index 000000000000..8609d9cf259b --- /dev/null +++ b/src/test/interpreters/locators/helpers.unit.test.ts @@ -0,0 +1,177 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +// tslint:disable:max-func-body-length + +import { expect } from 'chai'; +import * as path from 'path'; +import * as TypeMoq from 'typemoq'; +import { EnumEx } from '../../../client/common/enumUtils'; +import { Architecture, IFileSystem, IPlatformService } from '../../../client/common/platform/types'; +import { PythonVersionInfo } from '../../../client/common/process/types'; +import { IInterpreterLocatorHelper, IInterpreterService, InterpreterType, PythonInterpreter } from '../../../client/interpreter/contracts'; +import { InterpreterLocatorHelper } from '../../../client/interpreter/locators/helpers'; +import { IServiceContainer } from '../../../client/ioc/types'; + +enum OS { + Windows = 'Windows', + Linux = 'Linux', + Mac = 'Mac' +} + +suite('Interpreters - Locators Helper', () => { + let serviceContainer: TypeMoq.IMock; + let platform: TypeMoq.IMock; + let helper: IInterpreterLocatorHelper; + let fs: TypeMoq.IMock; + let interpreterService: TypeMoq.IMock; + setup(() => { + serviceContainer = TypeMoq.Mock.ofType(); + platform = TypeMoq.Mock.ofType(); + fs = TypeMoq.Mock.ofType(); + interpreterService = TypeMoq.Mock.ofType(); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPlatformService))).returns(() => platform.object); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fs.object); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInterpreterService))).returns(() => interpreterService.object); + + helper = new InterpreterLocatorHelper(serviceContainer.object); + }); + test('Ensure default Mac interpreters are excluded from the list of interpreters', async () => { + platform.setup(p => p.isWindows).returns(() => false); + platform.setup(p => p.isLinux).returns(() => false); + platform + .setup(p => p.isMac).returns(() => true) + .verifiable(TypeMoq.Times.atLeastOnce()); + fs + .setup(f => f.arePathsSame(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => false) + .verifiable(TypeMoq.Times.atLeastOnce()); + + const interpreters: PythonInterpreter[] = []; + const macInterpreterPath = path.join('users', 'python', 'bin', 'mac'); + ['conda', 'virtualenv', 'mac', 'pyenv'].forEach(name => { + const interpreter = { + architecture: Architecture.Unknown, + displayName: name, + path: path.join('users', 'python', 'bin', name), + sysPrefix: name, + sysVersion: name, + type: InterpreterType.Unknown, + version: name, + version_info: [0, 0, 0, 'alpha'] as PythonVersionInfo + }; + interpreters.push(interpreter); + + // Treat 'mac' as as mac interpreter. + interpreterService + .setup(i => i.isMacDefaultPythonPath(TypeMoq.It.isValue(interpreter.path))) + .returns(() => name === 'mac') + .verifiable(TypeMoq.Times.once()); + }); + + const expectedInterpreters = interpreters.filter(item => item.path !== macInterpreterPath); + + const items = helper.mergeInterpreters(interpreters); + + interpreterService.verifyAll(); + platform.verifyAll(); + fs.verifyAll(); + expect(items).to.be.lengthOf(3); + expect(items).to.be.deep.equal(expectedInterpreters); + }); + EnumEx.getNamesAndValues(OS).forEach(os => { + test(`Ensure duplicates are removed (same version and same interpreter directory on ${os.name})`, async () => { + interpreterService + .setup(i => i.isMacDefaultPythonPath(TypeMoq.It.isAny())) + .returns(() => false); + platform.setup(p => p.isWindows).returns(() => os.value === OS.Windows); + platform.setup(p => p.isLinux).returns(() => os.value === OS.Linux); + platform.setup(p => p.isMac).returns(() => os.value === OS.Mac); + fs + .setup(f => f.arePathsSame(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns((a, b) => { + if (a === b && a === path.join('users', 'python', 'bin', '3.6')) { + return true; + } + if (a === b && a === path.join('users', 'python', 'bin', '3.7')) { + return true; + } + return false; + }) + .verifiable(TypeMoq.Times.atLeastOnce()); + + const interpreters: PythonInterpreter[] = []; + const expectedInterpreters: PythonInterpreter[] = []; + // Unique python paths and versions. + ['3.6', '3.6', '2.7', '2.7'].forEach((name, index) => { + const interpreter = { + architecture: Architecture.Unknown, + displayName: name, + path: path.join('users', `python${name}`, 'bin', name + index.toString()), + sysPrefix: name, + sysVersion: name, + type: InterpreterType.Unknown, + version: name, + version_info: [3, parseInt(name.substr(-1), 10), 0, 'final'] as PythonVersionInfo + }; + interpreters.push(interpreter); + expectedInterpreters.push(interpreter); + }); + // These are duplicates. + ['3.6', '3.6', '3.7', '3.7'].forEach((name, index) => { + const interpreter = { + architecture: Architecture.Unknown, + displayName: name, + path: path.join('users', 'python', 'bin', 'python.exe'), + sysPrefix: name, + sysVersion: name, + type: InterpreterType.Unknown, + version: name, + version_info: [3, parseInt(name.substr(-1), 10), 0, 'final'] as PythonVersionInfo + }; + + const duplicateInterpreter = { + architecture: Architecture.Unknown, + displayName: name, + path: path.join('users', 'python', 'bin', `python${name}.exe`), + sysPrefix: name, + sysVersion: name, + type: InterpreterType.Unknown, + version: name, + version_info: [3, parseInt(name.substr(-1), 10), 0, 'final'] as PythonVersionInfo + }; + + interpreters.push(duplicateInterpreter); + interpreters.push(interpreter); + if (index % 2 === 1) { + expectedInterpreters.push(interpreter); + } + }); + // // Same versions but in the same directory. + // ['3.6', '2.7'].forEach(name => { + // const interpreter = { + // architecture: Architecture.Unknown, + // displayName: name, + // path: path.join('users', 'python', 'bin', '3.7'), + // sysPrefix: name, + // sysVersion: name, + // type: InterpreterType.Unknown, + // version: name, + // version_info: [2, 7, 0, 'final'] as PythonVersionInfo + // }; + // interpreters.push(interpreter); + // expectedInterpreters.push(interpreter); + // }); + + const items = helper.mergeInterpreters(interpreters); + + interpreterService.verifyAll(); + platform.verifyAll(); + fs.verifyAll(); + expect(items).to.be.lengthOf(expectedInterpreters.length); + expect(items).to.be.deep.equal(expectedInterpreters); + }); + }); +}); diff --git a/src/test/interpreters/locators/index.unit.test.ts b/src/test/interpreters/locators/index.unit.test.ts new file mode 100644 index 000000000000..7c4b812a346d --- /dev/null +++ b/src/test/interpreters/locators/index.unit.test.ts @@ -0,0 +1,163 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +// tslint:disable:max-func-body-length + +import { expect } from 'chai'; +import * as TypeMoq from 'typemoq'; +import { Uri } from 'vscode'; +import { EnumEx } from '../../../client/common/enumUtils'; +import { Architecture, IPlatformService } from '../../../client/common/platform/types'; +import { IDisposableRegistry } from '../../../client/common/types'; +import { CONDA_ENV_FILE_SERVICE, CONDA_ENV_SERVICE, CURRENT_PATH_SERVICE, GLOBAL_VIRTUAL_ENV_SERVICE, IInterpreterLocatorHelper, IInterpreterLocatorService, InterpreterType, KNOWN_PATH_SERVICE, PIPENV_SERVICE, PythonInterpreter, WINDOWS_REGISTRY_SERVICE, WORKSPACE_VIRTUAL_ENV_SERVICE } from '../../../client/interpreter/contracts'; +import { PythonInterpreterLocatorService } from '../../../client/interpreter/locators'; +import { IServiceContainer } from '../../../client/ioc/types'; + +enum OS { + Windows, Linux, Mac +} + +suite('Interpreters - Locators Index', () => { + let serviceContainer: TypeMoq.IMock; + let platform: TypeMoq.IMock; + let helper: TypeMoq.IMock; + let locator: IInterpreterLocatorService; + setup(() => { + serviceContainer = TypeMoq.Mock.ofType(); + platform = TypeMoq.Mock.ofType(); + helper = TypeMoq.Mock.ofType(); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IDisposableRegistry))).returns(() => []); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPlatformService))).returns(() => platform.object); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInterpreterLocatorHelper))).returns(() => helper.object); + + locator = new PythonInterpreterLocatorService(serviceContainer.object); + }); + [undefined, Uri.file('Something')].forEach(resource => { + EnumEx.getNamesAndValues(OS).forEach(os => { + const testSuffix = `(on ${os.name}, with${resource ? '' : 'out'} a resource)`; + test(`All Interpreter Sources are used ${testSuffix}`, async () => { + const locatorsTypes: string[] = []; + if (os.value === OS.Windows) { + locatorsTypes.push(WINDOWS_REGISTRY_SERVICE); + } + platform.setup(p => p.isWindows).returns(() => os.value === OS.Windows); + platform.setup(p => p.isLinux).returns(() => os.value === OS.Linux); + platform.setup(p => p.isMac).returns(() => os.value === OS.Mac); + + locatorsTypes.push(CONDA_ENV_SERVICE); + locatorsTypes.push(CONDA_ENV_FILE_SERVICE); + locatorsTypes.push(PIPENV_SERVICE); + locatorsTypes.push(GLOBAL_VIRTUAL_ENV_SERVICE); + locatorsTypes.push(WORKSPACE_VIRTUAL_ENV_SERVICE); + + if (os.value !== OS.Windows) { + locatorsTypes.push(KNOWN_PATH_SERVICE); + } + locatorsTypes.push(CURRENT_PATH_SERVICE); + + const locatorsWithInterpreters = locatorsTypes.map(typeName => { + const interpreter: PythonInterpreter = { + architecture: Architecture.Unknown, + displayName: typeName, + path: typeName, + sysPrefix: typeName, + sysVersion: typeName, + type: InterpreterType.Unknown, + version: typeName, + version_info: [0, 0, 0, 'alpha'] + }; + + const typeLocator = TypeMoq.Mock.ofType(); + typeLocator + .setup(l => l.getInterpreters(TypeMoq.It.isValue(resource))) + .returns(() => Promise.resolve([interpreter])) + .verifiable(TypeMoq.Times.once()); + + serviceContainer + .setup(c => c.get(TypeMoq.It.isValue(IInterpreterLocatorService), TypeMoq.It.isValue(typeName))) + .returns(() => typeLocator.object); + + return { + type: typeName, + locator: typeLocator, + interpreters: [interpreter] + }; + }); + + helper + .setup(h => h.mergeInterpreters(TypeMoq.It.isAny())) + .returns(() => locatorsWithInterpreters.map(item => item.interpreters[0])) + .verifiable(TypeMoq.Times.once()); + + await locator.getInterpreters(resource); + + locatorsWithInterpreters.forEach(item => item.locator.verifyAll()); + helper.verifyAll(); + }); + test(`Interpreter Sources are sorted correctly and merged ${testSuffix}`, async () => { + const locatorsTypes: string[] = []; + if (os.value === OS.Windows) { + locatorsTypes.push(WINDOWS_REGISTRY_SERVICE); + } + platform.setup(p => p.isWindows).returns(() => os.value === OS.Windows); + platform.setup(p => p.isLinux).returns(() => os.value === OS.Linux); + platform.setup(p => p.isMac).returns(() => os.value === OS.Mac); + + locatorsTypes.push(CONDA_ENV_SERVICE); + locatorsTypes.push(CONDA_ENV_FILE_SERVICE); + locatorsTypes.push(PIPENV_SERVICE); + locatorsTypes.push(GLOBAL_VIRTUAL_ENV_SERVICE); + locatorsTypes.push(WORKSPACE_VIRTUAL_ENV_SERVICE); + + if (os.value !== OS.Windows) { + locatorsTypes.push(KNOWN_PATH_SERVICE); + } + locatorsTypes.push(CURRENT_PATH_SERVICE); + + const locatorsWithInterpreters = locatorsTypes.map(typeName => { + const interpreter: PythonInterpreter = { + architecture: Architecture.Unknown, + displayName: typeName, + path: typeName, + sysPrefix: typeName, + sysVersion: typeName, + type: InterpreterType.Unknown, + version: typeName, + version_info: [0, 0, 0, 'alpha'] + }; + + const typeLocator = TypeMoq.Mock.ofType(); + typeLocator + .setup(l => l.getInterpreters(TypeMoq.It.isValue(resource))) + .returns(() => Promise.resolve([interpreter])) + .verifiable(TypeMoq.Times.once()); + + serviceContainer + .setup(c => c.get(TypeMoq.It.isValue(IInterpreterLocatorService), TypeMoq.It.isValue(typeName))) + .returns(() => typeLocator.object); + + return { + type: typeName, + locator: typeLocator, + interpreters: [interpreter] + }; + }); + + const expectedInterpreters = locatorsWithInterpreters.map(item => item.interpreters[0]); + helper + .setup(h => h.mergeInterpreters(TypeMoq.It.isAny())) + .returns(() => expectedInterpreters) + .verifiable(TypeMoq.Times.once()); + + const interpreters = await locator.getInterpreters(resource); + + locatorsWithInterpreters.forEach(item => item.locator.verifyAll()); + helper.verifyAll(); + expect(interpreters).to.be.lengthOf(locatorsTypes.length); + expect(interpreters).to.be.deep.equal(expectedInterpreters); + }); + }); + }); +}); From a52c7438fc6e890a5a89374d77ae88c720eac433 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Sun, 29 Jul 2018 10:16:18 +0100 Subject: [PATCH 2/7] Fixed and added more tests --- src/client/interpreter/locators/helpers.ts | 5 +- .../locators/helpers.unit.test.ts | 78 ++++++++++++------- 2 files changed, 51 insertions(+), 32 deletions(-) diff --git a/src/client/interpreter/locators/helpers.ts b/src/client/interpreter/locators/helpers.ts index 77abdb13bbcf..dabf0bb2c27f 100644 --- a/src/client/interpreter/locators/helpers.ts +++ b/src/client/interpreter/locators/helpers.ts @@ -48,14 +48,11 @@ export class InterpreterLocatorHelper implements IInterpreterLocatorHelper { } const currentVersion = Array.isArray(current.version_info) ? current.version_info.join('.') : undefined; const existingItem = accumulator.find(item => { - if (this.fs.arePathsSame(item.path, current.path)) { - return true; - } // If same version and same base path, then ignore. // Could be Python 3.6 with path = python.exe, and Python 3.6 and path = python3.exe. if (Array.isArray(item.version_info) && item.version_info.join('.') === currentVersion && item.path && current.path && - this.fs.arePathsSame(path.basename(item.path), path.basename(current.path))) { + this.fs.arePathsSame(path.dirname(item.path), path.dirname(current.path))) { return true; } return false; diff --git a/src/test/interpreters/locators/helpers.unit.test.ts b/src/test/interpreters/locators/helpers.unit.test.ts index 8609d9cf259b..9b07a93df791 100644 --- a/src/test/interpreters/locators/helpers.unit.test.ts +++ b/src/test/interpreters/locators/helpers.unit.test.ts @@ -91,15 +91,7 @@ suite('Interpreters - Locators Helper', () => { platform.setup(p => p.isMac).returns(() => os.value === OS.Mac); fs .setup(f => f.arePathsSame(TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns((a, b) => { - if (a === b && a === path.join('users', 'python', 'bin', '3.6')) { - return true; - } - if (a === b && a === path.join('users', 'python', 'bin', '3.7')) { - return true; - } - return false; - }) + .returns((a, b) => a === b) .verifiable(TypeMoq.Times.atLeastOnce()); const interpreters: PythonInterpreter[] = []; @@ -109,7 +101,7 @@ suite('Interpreters - Locators Helper', () => { const interpreter = { architecture: Architecture.Unknown, displayName: name, - path: path.join('users', `python${name}`, 'bin', name + index.toString()), + path: path.join('users', `python${name}${index}`, 'bin', name + index.toString()), sysPrefix: name, sysVersion: name, type: InterpreterType.Unknown, @@ -119,7 +111,7 @@ suite('Interpreters - Locators Helper', () => { interpreters.push(interpreter); expectedInterpreters.push(interpreter); }); - // These are duplicates. + // Same versions, but different executables. ['3.6', '3.6', '3.7', '3.7'].forEach((name, index) => { const interpreter = { architecture: Architecture.Unknown, @@ -140,30 +132,15 @@ suite('Interpreters - Locators Helper', () => { sysVersion: name, type: InterpreterType.Unknown, version: name, - version_info: [3, parseInt(name.substr(-1), 10), 0, 'final'] as PythonVersionInfo + version_info: interpreter.version_info }; - interpreters.push(duplicateInterpreter); interpreters.push(interpreter); + interpreters.push(duplicateInterpreter); if (index % 2 === 1) { expectedInterpreters.push(interpreter); } }); - // // Same versions but in the same directory. - // ['3.6', '2.7'].forEach(name => { - // const interpreter = { - // architecture: Architecture.Unknown, - // displayName: name, - // path: path.join('users', 'python', 'bin', '3.7'), - // sysPrefix: name, - // sysVersion: name, - // type: InterpreterType.Unknown, - // version: name, - // version_info: [2, 7, 0, 'final'] as PythonVersionInfo - // }; - // interpreters.push(interpreter); - // expectedInterpreters.push(interpreter); - // }); const items = helper.mergeInterpreters(interpreters); @@ -174,4 +151,49 @@ suite('Interpreters - Locators Helper', () => { expect(items).to.be.deep.equal(expectedInterpreters); }); }); + EnumEx.getNamesAndValues(OS).forEach(os => { + test(`Ensure interpreter types are identified from other locators (${os.name})`, async () => { + interpreterService + .setup(i => i.isMacDefaultPythonPath(TypeMoq.It.isAny())) + .returns(() => false); + platform.setup(p => p.isWindows).returns(() => os.value === OS.Windows); + platform.setup(p => p.isLinux).returns(() => os.value === OS.Linux); + platform.setup(p => p.isMac).returns(() => os.value === OS.Mac); + fs + .setup(f => f.arePathsSame(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns((a, b) => a === b && a === path.join('users', 'python', 'bin')) + .verifiable(TypeMoq.Times.atLeastOnce()); + + const interpreters: PythonInterpreter[] = []; + const expectedInterpreters: PythonInterpreter[] = []; + ['3.6', '3.6'].forEach((name, index) => { + // Ensure the type in the first item is 'Unknown', + // and type in second item is known (e.g. Conda). + const type = index === 0 ? InterpreterType.Unknown : InterpreterType.PipEnv; + const interpreter = { + architecture: Architecture.Unknown, + displayName: name, + path: path.join('users', 'python', 'bin', 'python.exe'), + sysPrefix: name, + sysVersion: name, + type, + version: name, + version_info: [3, parseInt(name.substr(-1), 10), 0, 'final'] as PythonVersionInfo + }; + interpreters.push(interpreter); + + if (index === 1) { + expectedInterpreters.push(interpreter); + } + }); + + const items = helper.mergeInterpreters(interpreters); + + interpreterService.verifyAll(); + platform.verifyAll(); + fs.verifyAll(); + expect(items).to.be.lengthOf(1); + expect(items).to.be.deep.equal(expectedInterpreters); + }); + }); }); From a67be09e046d10d3a20a92bdf1a75b1aec4e63bd Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Sun, 29 Jul 2018 17:24:13 -0500 Subject: [PATCH 3/7] Refactor to move method into helper class --- .../common/installer/pythonInstallation.ts | 7 ++++--- src/client/interpreter/contracts.ts | 2 +- src/client/interpreter/helpers.ts | 14 ++++++++----- src/client/interpreter/interpreterService.ts | 4 ---- src/client/interpreter/locators/helpers.ts | 8 ++++---- .../locators/helpers.unit.test.ts | 20 +++++++++---------- 6 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/client/common/installer/pythonInstallation.ts b/src/client/common/installer/pythonInstallation.ts index 248613439d85..d70b1e96bcb7 100644 --- a/src/client/common/installer/pythonInstallation.ts +++ b/src/client/common/installer/pythonInstallation.ts @@ -2,7 +2,7 @@ // Licensed under the MIT License. 'use strict'; -import { IInterpreterLocatorService, IInterpreterService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../interpreter/contracts'; +import { IInterpreterHelper, IInterpreterLocatorService, IInterpreterService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; import { IApplicationShell } from '../application/types'; import { IPlatformService } from '../platform/types'; @@ -24,8 +24,9 @@ export class PythonInstaller { const interpreters = await this.locator.getInterpreters(); if (interpreters.length > 0) { const platform = this.serviceContainer.get(IPlatformService); - const interpreterService = this.serviceContainer.get(IInterpreterService); - if (platform.isMac && interpreterService.isMacDefaultPythonPath(settings.pythonPath)) { + const helper = this.serviceContainer.get(IInterpreterHelper); + if (platform.isMac && helper.isMacDefaultPythonPath(settings.pythonPath)) { + const interpreterService = this.serviceContainer.get(IInterpreterService); const interpreter = await interpreterService.getActiveInterpreter(); if (interpreter && interpreter.type === InterpreterType.Unknown) { await this.shell.showWarningMessage('Selected interpreter is macOS system Python which is not recommended. Please select different interpreter'); diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index f535e47415d3..f4dd60ba52a9 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -81,7 +81,6 @@ export interface IInterpreterService { getInterpreterDetails(pythonPath: string): Promise>; refresh(): Promise; initialize(): void; - isMacDefaultPythonPath(pythonPath: string): Boolean; } export const IInterpreterDisplay = Symbol('IInterpreterDisplay'); @@ -98,6 +97,7 @@ export const IInterpreterHelper = Symbol('IInterpreterHelper'); export interface IInterpreterHelper { getActiveWorkspaceUri(): WorkspacePythonPath | undefined; getInterpreterInformation(pythonPath: string): Promise>; + isMacDefaultPythonPath(pythonPath: string): Boolean; } export const IPipEnvService = Symbol('IPipEnvService'); diff --git a/src/client/interpreter/helpers.ts b/src/client/interpreter/helpers.ts index 00535634a590..ee0278b9ef59 100644 --- a/src/client/interpreter/helpers.ts +++ b/src/client/interpreter/helpers.ts @@ -2,7 +2,7 @@ import { inject, injectable } from 'inversify'; import { ConfigurationTarget } from 'vscode'; import { IDocumentManager, IWorkspaceService } from '../common/application/types'; import { IFileSystem } from '../common/platform/types'; -import { IPythonExecutionFactory } from '../common/process/types'; +import { InterpreterInfomation, IPythonExecutionFactory } from '../common/process/types'; import { IPersistentStateFactory } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { IInterpreterHelper, PythonInterpreter, WorkspacePythonPath } from './contracts'; @@ -33,7 +33,7 @@ export class InterpreterHelper implements IInterpreterHelper { if (!workspaceService.hasWorkspaceFolders) { return; } - if (workspaceService.workspaceFolders.length === 1) { + if (Array.isArray(workspaceService.workspaceFolders) && workspaceService.workspaceFolders.length === 1) { return { folderUri: workspaceService.workspaceFolders[0].uri, configTarget: ConfigurationTarget.Workspace }; } if (documentManager.activeTextEditor) { @@ -44,15 +44,16 @@ export class InterpreterHelper implements IInterpreterHelper { } } public async getInterpreterInformation(pythonPath: string): Promise> { - const fileHash = await this.fs.getFileHash(pythonPath).catch(() => ''); + let fileHash = await this.fs.getFileHash(pythonPath).catch(() => ''); + fileHash = fileHash ? fileHash : ''; const store = this.persistentFactory.createGlobalPersistentState(pythonPath, undefined, EXPITY_DURATION); - if (store.value && store.value.fileHash === fileHash) { + if (store.value && (!fileHash || store.value.fileHash === fileHash)) { return store.value; } const processService = await this.serviceContainer.get(IPythonExecutionFactory).create({ pythonPath }); try { - const info = await processService.getInterpreterInformation().catch(() => undefined); + const info = await processService.getInterpreterInformation().catch(() => undefined); if (!info) { return; } @@ -67,4 +68,7 @@ export class InterpreterHelper implements IInterpreterHelper { return {}; } } + public isMacDefaultPythonPath(pythonPath: string) { + return pythonPath === 'python' || pythonPath === '/usr/bin/python'; + } } diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts index 16fcf5ca3b30..250989859ade 100644 --- a/src/client/interpreter/interpreterService.ts +++ b/src/client/interpreter/interpreterService.ts @@ -129,10 +129,6 @@ export class InterpreterService implements Disposable, IInterpreterService { type: type }; } - public isMacDefaultPythonPath(pythonPath: string) { - return pythonPath === 'python' || pythonPath === '/usr/bin/python'; - } - private async shouldAutoSetInterpreter(): Promise { const activeWorkspace = this.helper.getActiveWorkspaceUri(); if (!activeWorkspace) { diff --git a/src/client/interpreter/locators/helpers.ts b/src/client/interpreter/locators/helpers.ts index dabf0bb2c27f..1032f38c11c5 100644 --- a/src/client/interpreter/locators/helpers.ts +++ b/src/client/interpreter/locators/helpers.ts @@ -4,7 +4,7 @@ import { getArchitectureDislayName } from '../../common/platform/registry'; import { IFileSystem, IPlatformService } from '../../common/platform/types'; import { fsReaddirAsync, IS_WINDOWS } from '../../common/utils'; import { IServiceContainer } from '../../ioc/types'; -import { IInterpreterLocatorHelper, IInterpreterService, InterpreterType, PythonInterpreter } from '../contracts'; +import { IInterpreterHelper, IInterpreterLocatorHelper, InterpreterType, PythonInterpreter } from '../contracts'; const CheckPythonInterpreterRegEx = IS_WINDOWS ? /^python(\d+(.\d+)?)?\.exe$/ : /^python(\d+(.\d+)?)?$/; @@ -30,11 +30,11 @@ export function fixInterpreterDisplayName(item: PythonInterpreter) { export class InterpreterLocatorHelper implements IInterpreterLocatorHelper { private readonly platform: IPlatformService; private readonly fs: IFileSystem; - private readonly interpreterService: IInterpreterService; + private readonly helper: IInterpreterHelper; constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { this.platform = serviceContainer.get(IPlatformService); - this.interpreterService = serviceContainer.get(IInterpreterService); + this.helper = serviceContainer.get(IInterpreterHelper); this.fs = serviceContainer.get(IFileSystem); } public mergeInterpreters(interpreters: PythonInterpreter[]) { @@ -43,7 +43,7 @@ export class InterpreterLocatorHelper implements IInterpreterLocatorHelper { .map(fixInterpreterDisplayName) .map(item => { item.path = path.normalize(item.path); return item; }) .reduce((accumulator, current) => { - if (this.platform.isMac && this.interpreterService.isMacDefaultPythonPath(current.path)) { + if (this.platform.isMac && this.helper.isMacDefaultPythonPath(current.path)) { return accumulator; } const currentVersion = Array.isArray(current.version_info) ? current.version_info.join('.') : undefined; diff --git a/src/test/interpreters/locators/helpers.unit.test.ts b/src/test/interpreters/locators/helpers.unit.test.ts index 9b07a93df791..6826152402c8 100644 --- a/src/test/interpreters/locators/helpers.unit.test.ts +++ b/src/test/interpreters/locators/helpers.unit.test.ts @@ -11,7 +11,7 @@ import * as TypeMoq from 'typemoq'; import { EnumEx } from '../../../client/common/enumUtils'; import { Architecture, IFileSystem, IPlatformService } from '../../../client/common/platform/types'; import { PythonVersionInfo } from '../../../client/common/process/types'; -import { IInterpreterLocatorHelper, IInterpreterService, InterpreterType, PythonInterpreter } from '../../../client/interpreter/contracts'; +import { IInterpreterHelper, IInterpreterLocatorHelper, InterpreterType, PythonInterpreter } from '../../../client/interpreter/contracts'; import { InterpreterLocatorHelper } from '../../../client/interpreter/locators/helpers'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -26,15 +26,15 @@ suite('Interpreters - Locators Helper', () => { let platform: TypeMoq.IMock; let helper: IInterpreterLocatorHelper; let fs: TypeMoq.IMock; - let interpreterService: TypeMoq.IMock; + let interpreterServiceHelper: TypeMoq.IMock; setup(() => { serviceContainer = TypeMoq.Mock.ofType(); platform = TypeMoq.Mock.ofType(); fs = TypeMoq.Mock.ofType(); - interpreterService = TypeMoq.Mock.ofType(); + interpreterServiceHelper = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPlatformService))).returns(() => platform.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fs.object); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInterpreterService))).returns(() => interpreterService.object); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInterpreterHelper))).returns(() => interpreterServiceHelper.object); helper = new InterpreterLocatorHelper(serviceContainer.object); }); @@ -65,7 +65,7 @@ suite('Interpreters - Locators Helper', () => { interpreters.push(interpreter); // Treat 'mac' as as mac interpreter. - interpreterService + interpreterServiceHelper .setup(i => i.isMacDefaultPythonPath(TypeMoq.It.isValue(interpreter.path))) .returns(() => name === 'mac') .verifiable(TypeMoq.Times.once()); @@ -75,7 +75,7 @@ suite('Interpreters - Locators Helper', () => { const items = helper.mergeInterpreters(interpreters); - interpreterService.verifyAll(); + interpreterServiceHelper.verifyAll(); platform.verifyAll(); fs.verifyAll(); expect(items).to.be.lengthOf(3); @@ -83,7 +83,7 @@ suite('Interpreters - Locators Helper', () => { }); EnumEx.getNamesAndValues(OS).forEach(os => { test(`Ensure duplicates are removed (same version and same interpreter directory on ${os.name})`, async () => { - interpreterService + interpreterServiceHelper .setup(i => i.isMacDefaultPythonPath(TypeMoq.It.isAny())) .returns(() => false); platform.setup(p => p.isWindows).returns(() => os.value === OS.Windows); @@ -144,7 +144,7 @@ suite('Interpreters - Locators Helper', () => { const items = helper.mergeInterpreters(interpreters); - interpreterService.verifyAll(); + interpreterServiceHelper.verifyAll(); platform.verifyAll(); fs.verifyAll(); expect(items).to.be.lengthOf(expectedInterpreters.length); @@ -153,7 +153,7 @@ suite('Interpreters - Locators Helper', () => { }); EnumEx.getNamesAndValues(OS).forEach(os => { test(`Ensure interpreter types are identified from other locators (${os.name})`, async () => { - interpreterService + interpreterServiceHelper .setup(i => i.isMacDefaultPythonPath(TypeMoq.It.isAny())) .returns(() => false); platform.setup(p => p.isWindows).returns(() => os.value === OS.Windows); @@ -189,7 +189,7 @@ suite('Interpreters - Locators Helper', () => { const items = helper.mergeInterpreters(interpreters); - interpreterService.verifyAll(); + interpreterServiceHelper.verifyAll(); platform.verifyAll(); fs.verifyAll(); expect(items).to.be.lengthOf(1); From 0a32bc8213c9f5b6ef99052a1305e6402bf92fec Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 30 Jul 2018 07:27:08 -0700 Subject: [PATCH 4/7] Fix tests --- ...ctor.test.ts => interpreterSelector.unit.test.ts} | 2 -- src/test/install/pythonInstallation.test.ts | 12 ++++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) rename src/test/configuration/{interpreterSelector.test.ts => interpreterSelector.unit.test.ts} (96%) diff --git a/src/test/configuration/interpreterSelector.test.ts b/src/test/configuration/interpreterSelector.unit.test.ts similarity index 96% rename from src/test/configuration/interpreterSelector.test.ts rename to src/test/configuration/interpreterSelector.unit.test.ts index 9d222f5e1df6..afbef5893581 100644 --- a/src/test/configuration/interpreterSelector.test.ts +++ b/src/test/configuration/interpreterSelector.unit.test.ts @@ -80,8 +80,6 @@ suite('Interpreters - selector', () => { const initial: PythonInterpreter[] = [ { displayName: '1', path: 'c:/path1/path1', type: InterpreterType.Unknown }, { displayName: '2', path: 'c:/path1/path1', type: InterpreterType.Unknown }, - { displayName: '1', path: 'c:/path1/path1', type: InterpreterType.Unknown }, - { displayName: '2', path: 'c:/path2/path2', type: InterpreterType.Unknown }, { displayName: '2', path: 'c:/path2/path2', type: InterpreterType.Unknown }, { displayName: '2 (virtualenv)', path: 'c:/path2/path2', type: InterpreterType.VirtualEnv }, { displayName: '3', path: 'c:/path2/path2', type: InterpreterType.Unknown }, diff --git a/src/test/install/pythonInstallation.test.ts b/src/test/install/pythonInstallation.test.ts index b6131cc9c077..6652e1368e0c 100644 --- a/src/test/install/pythonInstallation.test.ts +++ b/src/test/install/pythonInstallation.test.ts @@ -7,9 +7,10 @@ import { Container } from 'inversify'; import * as TypeMoq from 'typemoq'; import { IApplicationShell } from '../../client/common/application/types'; import { PythonInstaller } from '../../client/common/installer/pythonInstallation'; -import { Architecture, IPlatformService } from '../../client/common/platform/types'; -import { IPythonSettings } from '../../client/common/types'; -import { IInterpreterLocatorService, IInterpreterService, InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; +import { Architecture, IFileSystem, IPlatformService } from '../../client/common/platform/types'; +import { IPersistentStateFactory, IPythonSettings } from '../../client/common/types'; +import { IInterpreterHelper, IInterpreterLocatorService, IInterpreterService, InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; +import { InterpreterHelper } from '../../client/interpreter/helpers'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceContainer } from '../../client/ioc/types'; @@ -56,7 +57,10 @@ class TestContext { interpreterService .setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); - + const helper = new InterpreterHelper(this.serviceContainer); + this.serviceManager.addSingletonInstance(IFileSystem, TypeMoq.Mock.ofType().object); + this.serviceManager.addSingletonInstance(IPersistentStateFactory, TypeMoq.Mock.ofType().object); + this.serviceManager.addSingletonInstance(IInterpreterHelper, helper); this.serviceManager.addSingletonInstance(IPlatformService, this.platform.object); this.serviceManager.addSingletonInstance(IApplicationShell, this.appShell.object); this.serviceManager.addSingletonInstance(IInterpreterLocatorService, this.locator.object); From ba134a04b61c209bd5e0b758119480c3bc308d58 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 30 Jul 2018 07:33:28 -0700 Subject: [PATCH 5/7] Remove commented code and import vscode only when necessary --- src/test/constants.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/test/constants.ts b/src/test/constants.ts index 268352e7f20a..e0af60460b5e 100644 --- a/src/test/constants.ts +++ b/src/test/constants.ts @@ -1,11 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { workspace } from 'vscode'; import { PythonSettings } from '../client/common/configSettings'; -// import { IS_APPVEYOR, IS_CI_SERVER, IS_CI_SERVER_TEST_DEBUGGER, -// IS_TRAVIS, IS_VSTS, MOCHA_CI_PROPERTIES, MOCHA_CI_REPORTFILE, -// MOCHA_REPORTER_JUNIT } from './ciConstants'; import { IS_CI_SERVER, IS_CI_SERVER_TEST_DEBUGGER, IS_TRAVIS } from './ciConstants'; export const TEST_TIMEOUT = 25000; @@ -14,11 +10,10 @@ export const IS_MULTI_ROOT_TEST = isMultitrootTest(); // If running on CI server, then run debugger tests ONLY if the corresponding flag is enabled. export const TEST_DEBUGGER = IS_CI_SERVER ? IS_CI_SERVER_TEST_DEBUGGER : true; -// export { IS_APPVEYOR, IS_CI_SERVER, IS_CI_SERVER_TEST_DEBUGGER, -// IS_TRAVIS, IS_VSTS, MOCHA_CI_PROPERTIES, MOCHA_CI_REPORTFILE, -// MOCHA_REPORTER_JUNIT }; - function isMultitrootTest() { + // tslint:disable-next-line:no-require-imports + const vscode = require('vscode'); + const workspace = vscode.workspace; return Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 1; } From 423c05824f005d8e94627439167d81202657f0ee Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 30 Jul 2018 09:16:24 -0700 Subject: [PATCH 6/7] Fixed tests --- ...llation.test.ts => pythonInstallation.unit.test.ts} | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) rename src/test/install/{pythonInstallation.test.ts => pythonInstallation.unit.test.ts} (96%) diff --git a/src/test/install/pythonInstallation.test.ts b/src/test/install/pythonInstallation.unit.test.ts similarity index 96% rename from src/test/install/pythonInstallation.test.ts rename to src/test/install/pythonInstallation.unit.test.ts index 6652e1368e0c..f1d2e9eee6f8 100644 --- a/src/test/install/pythonInstallation.test.ts +++ b/src/test/install/pythonInstallation.unit.test.ts @@ -14,7 +14,6 @@ import { InterpreterHelper } from '../../client/interpreter/helpers'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceContainer } from '../../client/ioc/types'; -import { closeActiveWindows, initialize, initializeTest } from '../initialize'; const info: PythonInterpreter = { architecture: Architecture.Unknown, @@ -57,9 +56,9 @@ class TestContext { interpreterService .setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); - const helper = new InterpreterHelper(this.serviceContainer); this.serviceManager.addSingletonInstance(IFileSystem, TypeMoq.Mock.ofType().object); this.serviceManager.addSingletonInstance(IPersistentStateFactory, TypeMoq.Mock.ofType().object); + const helper = new InterpreterHelper(this.serviceContainer); this.serviceManager.addSingletonInstance(IInterpreterHelper, helper); this.serviceManager.addSingletonInstance(IPlatformService, this.platform.object); this.serviceManager.addSingletonInstance(IApplicationShell, this.appShell.object); @@ -74,13 +73,6 @@ class TestContext { // tslint:disable-next-line:max-func-body-length suite('Installation', () => { - suiteSetup(async () => { - await initialize(); - }); - setup(initializeTest); - suiteTeardown(closeActiveWindows); - teardown(closeActiveWindows); - test('Disable checks', async () => { const c = new TestContext(false); let showErrorMessageCalled = false; From 3c3dd0307061a16ea0cee3776c200db7a47d881c Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 1 Aug 2018 12:35:53 -0700 Subject: [PATCH 7/7] Added tests --- .../interpreters/interpreterService.test.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/test/interpreters/interpreterService.test.ts b/src/test/interpreters/interpreterService.test.ts index f6033919d9b0..e0a66d750c6a 100644 --- a/src/test/interpreters/interpreterService.test.ts +++ b/src/test/interpreters/interpreterService.test.ts @@ -24,6 +24,7 @@ import { WorkspacePythonPath } from '../../client/interpreter/contracts'; import { InterpreterService } from '../../client/interpreter/interpreterService'; +import { IVirtualEnvironmentManager } from '../../client/interpreter/virtualEnvs/types'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; @@ -255,4 +256,35 @@ suite('Interpreters service', () => { interpreterDisplay.verify(i => i.refresh(TypeMoq.It.isValue(undefined)), TypeMoq.Times.never()); }); + [undefined, Uri.file('some workspace')] + .forEach(resource => { + test(`Ensure undefined is returned if we're unable to retrieve interpreter info (Resource is ${resource})`, async () => { + const pythonPath = 'SOME VALUE'; + const service = new InterpreterService(serviceContainer); + locator + .setup(l => l.getInterpreters(TypeMoq.It.isValue(resource))) + .returns(() => Promise.resolve([])) + .verifiable(TypeMoq.Times.once()); + helper + .setup(h => h.getInterpreterInformation(TypeMoq.It.isValue(pythonPath))) + .returns(() => Promise.resolve(undefined)) + .verifiable(TypeMoq.Times.once()); + const virtualEnvMgr = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IVirtualEnvironmentManager, virtualEnvMgr.object); + virtualEnvMgr + .setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))) + .returns(() => Promise.resolve('')) + .verifiable(TypeMoq.Times.once()); + virtualEnvMgr + .setup(v => v.getEnvironmentType(TypeMoq.It.isValue(pythonPath))) + .returns(() => Promise.resolve(InterpreterType.Unknown)) + .verifiable(TypeMoq.Times.once()); + + const details = await service.getInterpreterDetails(pythonPath, resource); + + locator.verifyAll(); + helper.verifyAll(); + expect(details).to.be.equal(undefined, 'Not undefined'); + }); + }); });