From 141a0d1fbbb7397bad575968491aa5f8ec26712b Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Thu, 7 May 2020 14:13:38 -0700 Subject: [PATCH 1/2] Show the prompt again if user clicks on more info (#11664) * Show the prompt again if user clicks on more info * Review feedback * Use Learn more as text for the link. --- package.nls.json | 6 +++--- .../diagnostics/checks/pythonPathDeprecated.ts | 8 -------- src/client/common/utils/localize.ts | 6 +++--- .../checks/pythonPathDeprecated.unit.test.ts | 16 +--------------- 4 files changed, 7 insertions(+), 29 deletions(-) diff --git a/package.nls.json b/package.nls.json index 77622a77439c..46aa8b8583e5 100644 --- a/package.nls.json +++ b/package.nls.json @@ -240,9 +240,9 @@ "DataScience.liveShareServiceFailure": "Failure starting '{0}' service during live share connection.", "DataScience.documentMismatch": "Cannot run cells, duplicate documents for {0} found.", "DataScience.pythonInteractiveCreateFailed": "Failure to create a 'Python Interactive' window. Try reinstalling the Python extension.", - "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your settings.json.", - "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your .code-workspace file.", - "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your settings.json and .code-workspace file.", + "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? [Learn more](https://aka.ms/AA7jfor).", + "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? [Learn more](https://aka.ms/AA7jfor).", + "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? [Learn more](https://aka.ms/AA7jfor).", "diagnostics.warnSourceMaps": "Source map support is enabled in the Python Extension, this will adversely impact performance of the extension.", "diagnostics.disableSourceMaps": "Disable Source Map Support", "diagnostics.warnBeforeEnablingSourceMaps": "Enabling source map support in the Python Extension will adversely impact performance of the extension.", diff --git a/src/client/application/diagnostics/checks/pythonPathDeprecated.ts b/src/client/application/diagnostics/checks/pythonPathDeprecated.ts index 6ba46dd11042..80dd0d5b0cbc 100644 --- a/src/client/application/diagnostics/checks/pythonPathDeprecated.ts +++ b/src/client/application/diagnostics/checks/pythonPathDeprecated.ts @@ -9,7 +9,6 @@ import { IWorkspaceService } from '../../../common/application/types'; import { DeprecatePythonPath } from '../../../common/experimentGroups'; import { IDisposableRegistry, IExperimentsManager, Resource } from '../../../common/types'; import { Common, Diagnostics } from '../../../common/utils/localize'; -import { learnMoreOnInterpreterSecurityURI } from '../../../interpreter/autoSelection/constants'; import { IServiceContainer } from '../../../ioc/types'; import { BaseDiagnostic, BaseDiagnosticsService } from '../base'; import { IDiagnosticsCommandFactory } from '../commands/types'; @@ -97,13 +96,6 @@ export class PythonPathDeprecatedDiagnosticService extends BaseDiagnosticsServic { prompt: Common.noIWillDoItLater() }, - { - prompt: Common.moreInfo(), - command: commandFactory.createCommand(diagnostic, { - type: 'launch', - options: learnMoreOnInterpreterSecurityURI - }) - }, { prompt: Common.doNotShowAgain(), command: commandFactory.createCommand(diagnostic, { type: 'ignore', options: DiagnosticScope.Global }) diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 526cd0df6035..946eb7d09adc 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -32,15 +32,15 @@ export namespace Diagnostics { ); export const removePythonPathSettingsJson = localize( 'diagnostics.removePythonPathSettingsJson', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your settings.json.' + 'The setting "python.pythonPath" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? [Learn more](https://aka.ms/AA7jfor).' ); export const removePythonPathCodeWorkspace = localize( 'diagnostics.removePythonPathCodeWorkspace', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your .code-workspace file.' + 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? [Learn more](https://aka.ms/AA7jfor).' ); export const removePythonPathCodeWorkspaceAndSettingsJson = localize( 'diagnostics.removePythonPathCodeWorkspaceAndSettingsJson', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your settings.json and .code-workspace file.' + 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? [Learn more](https://aka.ms/AA7jfor).' ); export const invalidPythonPathInDebuggerSettings = localize( 'diagnostics.invalidPythonPathInDebuggerSettings', diff --git a/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts b/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts index 05bfaa95d6a7..014bf92eff1e 100644 --- a/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts @@ -142,7 +142,6 @@ suite('Application Diagnostics - Python Path Deprecated', () => { }); test('Python Path Deprecated Diagnostic is handled as expected', async () => { const diagnostic = new PythonPathDeprecatedDiagnostic('message', resource); - const launchCmd = ({ cmd: 'launchCmd' } as any) as IDiagnosticCommand; const ignoreCmd = ({ cmd: 'ignoreCmd' } as any) as IDiagnosticCommand; filterService .setup((f) => @@ -155,15 +154,6 @@ suite('Application Diagnostics - Python Path Deprecated', () => { .callback((_d, p: MessageCommandPrompt) => (messagePrompt = p)) .returns(() => Promise.resolve()) .verifiable(typemoq.Times.once()); - commandFactory - .setup((f) => - f.createCommand( - typemoq.It.isAny(), - typemoq.It.isObjectWith>({ type: 'launch' }) - ) - ) - .returns(() => launchCmd) - .verifiable(typemoq.Times.once()); commandFactory .setup((f) => @@ -180,16 +170,12 @@ suite('Application Diagnostics - Python Path Deprecated', () => { messageHandler.verifyAll(); commandFactory.verifyAll(); expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); - expect(messagePrompt!.commandPrompts.length).to.equal(4, 'Incorrect length'); + expect(messagePrompt!.commandPrompts.length).to.equal(3, 'Incorrect length'); expect(messagePrompt!.commandPrompts[0].command).not.be.equal(undefined, 'Command not set'); expect(messagePrompt!.commandPrompts[0].command!.diagnostic).to.be.deep.equal(diagnostic); expect(messagePrompt!.commandPrompts[0].prompt).to.be.deep.equal(Common.yesPlease()); expect(messagePrompt!.commandPrompts[1]).to.be.deep.equal({ prompt: Common.noIWillDoItLater() }); expect(messagePrompt!.commandPrompts[2]).to.be.deep.equal({ - prompt: Common.moreInfo(), - command: launchCmd - }); - expect(messagePrompt!.commandPrompts[3]).to.be.deep.equal({ prompt: Common.doNotShowAgain(), command: ignoreCmd }); From 2739b34698de841811eb54f25481350bfcba328d Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Date: Fri, 8 May 2020 07:38:11 -0700 Subject: [PATCH 2/2] Leave pipenv in a corner until the user decides to select an interpreter (#11654) * add onSuggestion option * Swap onActivation with onSuggestion * Update unit tests * Remove registration of IPipenvService * Move didTriggerInterpreterSuggestions logic inside pipenv locator * Fix existing unit tests * Add new unit tests * Replace typemoq any param with object * Shorten the tests * Fix warning * Duplicate teardown --- src/client/activation/activationManager.ts | 2 +- .../checks/macPythonInterpreter.ts | 2 +- .../pipEnvActivationProvider.ts | 14 +- .../interpreter/autoSelection/rules/system.ts | 2 +- .../interpreterSelector.ts | 2 +- src/client/interpreter/contracts.ts | 5 +- src/client/interpreter/locators/index.ts | 21 +- .../services/cacheableLocatorService.ts | 11 + .../locators/services/pipEnvService.ts | 17 +- src/client/interpreter/serviceRegistry.ts | 2 - src/client/interpreter/virtualEnvs/index.ts | 8 +- src/client/startupTelemetry.ts | 4 +- .../activation/activationManager.unit.test.ts | 8 +- .../checks/macPythonInterpreter.unit.test.ts | 8 +- .../interpreterSelector.unit.test.ts | 6 +- .../datascience/dataScienceIocContainer.ts | 2 - .../rules/currentPath.unit.test.ts | 2 +- .../autoSelection/rules/system.unit.test.ts | 15 +- .../interpreters/locators/index.unit.test.ts | 31 +- .../interpreters/pipEnvService.unit.test.ts | 329 ++++++++++-------- .../interpreters/serviceRegistry.unit.test.ts | 2 - .../virtualEnvManager.unit.test.ts | 12 +- .../virtualEnvs/index.unit.test.ts | 11 +- src/test/serviceRegistry.ts | 2 - 24 files changed, 308 insertions(+), 210 deletions(-) diff --git a/src/client/activation/activationManager.ts b/src/client/activation/activationManager.ts index 4a5333c6111b..f570eb05aa3a 100644 --- a/src/client/activation/activationManager.ts +++ b/src/client/activation/activationManager.ts @@ -73,7 +73,7 @@ export class ExtensionActivationManager implements IExtensionActivationManager { this.experiments.sendTelemetryIfInExperiment(DeprecatePythonPath.control); // Get latest interpreter list in the background. - this.interpreterService.getInterpreters(resource, { onActivation: true }).ignoreErrors(); + this.interpreterService.getInterpreters(resource).ignoreErrors(); await sendActivationTelemetry(this.fileSystem, this.workspaceService, resource); diff --git a/src/client/application/diagnostics/checks/macPythonInterpreter.ts b/src/client/application/diagnostics/checks/macPythonInterpreter.ts index 9515c7b76928..26344f41a607 100644 --- a/src/client/application/diagnostics/checks/macPythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/macPythonInterpreter.ts @@ -101,7 +101,7 @@ export class InvalidMacPythonInterpreterService extends BaseDiagnosticsService { return []; } - const interpreters = await this.interpreterService.getInterpreters(resource, { onActivation: true }); + const interpreters = await this.interpreterService.getInterpreters(resource); if (interpreters.filter((i) => !this.helper.isMacDefaultPythonPath(i.path)).length === 0) { return [ new InvalidMacPythonInterpreterDiagnostic( diff --git a/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts b/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts index a25862ebd007..4b0ff07129fd 100644 --- a/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts +++ b/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts @@ -3,10 +3,16 @@ 'use strict'; -import { inject, injectable } from 'inversify'; +import { inject, injectable, named } from 'inversify'; import { Uri } from 'vscode'; import '../../../common/extensions'; -import { IInterpreterService, InterpreterType, IPipEnvService } from '../../../interpreter/contracts'; +import { + IInterpreterLocatorService, + IInterpreterService, + InterpreterType, + IPipEnvService, + PIPENV_SERVICE +} from '../../../interpreter/contracts'; import { IWorkspaceService } from '../../application/types'; import { IFileSystem } from '../../platform/types'; import { ITerminalActivationCommandProvider, TerminalShellType } from '../types'; @@ -15,7 +21,9 @@ import { ITerminalActivationCommandProvider, TerminalShellType } from '../types' export class PipEnvActivationCommandProvider implements ITerminalActivationCommandProvider { constructor( @inject(IInterpreterService) private readonly interpreterService: IInterpreterService, - @inject(IPipEnvService) private readonly pipenvService: IPipEnvService, + @inject(IInterpreterLocatorService) + @named(PIPENV_SERVICE) + private readonly pipenvService: IPipEnvService, @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IFileSystem) private readonly fs: IFileSystem ) {} diff --git a/src/client/interpreter/autoSelection/rules/system.ts b/src/client/interpreter/autoSelection/rules/system.ts index 6785d83a9fa8..9feb4925b9a4 100644 --- a/src/client/interpreter/autoSelection/rules/system.ts +++ b/src/client/interpreter/autoSelection/rules/system.ts @@ -25,7 +25,7 @@ export class SystemWideInterpretersAutoSelectionRule extends BaseRuleService { resource: Resource, manager?: IInterpreterAutoSelectionService ): Promise { - const interpreters = await this.interpreterService.getInterpreters(resource, { onActivation: true }); + const interpreters = await this.interpreterService.getInterpreters(resource); // Exclude non-local interpreters. const filteredInterpreters = interpreters.filter( (int) => diff --git a/src/client/interpreter/configuration/interpreterSelector/interpreterSelector.ts b/src/client/interpreter/configuration/interpreterSelector/interpreterSelector.ts index c328b1a58ad0..f7c95fb7e0e1 100644 --- a/src/client/interpreter/configuration/interpreterSelector/interpreterSelector.ts +++ b/src/client/interpreter/configuration/interpreterSelector/interpreterSelector.ts @@ -27,7 +27,7 @@ export class InterpreterSelector implements IInterpreterSelector { } public async getSuggestions(resource: Resource) { - let interpreters = await this.interpreterManager.getInterpreters(resource); + let interpreters = await this.interpreterManager.getInterpreters(resource, { onSuggestion: true }); if (this.experimentsManager.inExperiment(DeprecatePythonPath.experiment)) { interpreters = interpreters.filter((item) => this.interpreterSecurityService.isSafe(item) !== false); } diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index cfea89200a91..6013ff504bd2 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -28,7 +28,7 @@ export interface IVirtualEnvironmentsSearchPathProvider { } export type GetInterpreterOptions = { - onActivation?: boolean; + onSuggestion?: boolean; }; export type GetInterpreterLocatorOptions = GetInterpreterOptions & { ignoreCache?: boolean }; @@ -38,6 +38,7 @@ export const IInterpreterLocatorService = Symbol('IInterpreterLocatorService'); export interface IInterpreterLocatorService extends Disposable { readonly onLocating: Event>; readonly hasInterpreters: Promise; + didTriggerInterpreterSuggestions?: boolean; getInterpreters(resource?: Uri, options?: GetInterpreterLocatorOptions): Promise; } @@ -126,7 +127,7 @@ export interface IInterpreterHelper { } export const IPipEnvService = Symbol('IPipEnvService'); -export interface IPipEnvService { +export interface IPipEnvService extends IInterpreterLocatorService { executable: string; isRelatedPipEnvironment(dir: string, pythonPath: string): Promise; } diff --git a/src/client/interpreter/locators/index.ts b/src/client/interpreter/locators/index.ts index 76b3443c4a80..dc65eead948f 100644 --- a/src/client/interpreter/locators/index.ts +++ b/src/client/interpreter/locators/index.ts @@ -30,10 +30,13 @@ const flatten = require('lodash/flatten') as typeof import('lodash/flatten'); */ @injectable() export class PythonInterpreterLocatorService implements IInterpreterLocatorService { + public didTriggerInterpreterSuggestions: boolean; + private readonly disposables: Disposable[] = []; private readonly platform: IPlatformService; private readonly interpreterLocatorHelper: IInterpreterLocatorHelper; private readonly _hasInterpreters: Deferred; + constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer, @inject(InterpreterFilter) private readonly interpreterFilter: IInterpreterFilter @@ -42,6 +45,7 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi serviceContainer.get(IDisposableRegistry).push(this); this.platform = serviceContainer.get(IPlatformService); this.interpreterLocatorHelper = serviceContainer.get(IInterpreterLocatorHelper); + this.didTriggerInterpreterSuggestions = false; } /** * This class should never emit events when we're locating. @@ -106,18 +110,27 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi // The order is important because the data sources at the bottom of the list do not contain all, // the information about the interpreters (e.g. type, environment name, etc). // This way, the items returned from the top of the list will win, when we combine the items returned. - const keys: [string | undefined, OSType | undefined][] = [ + const keys: [string, OSType | undefined][] = [ [WINDOWS_REGISTRY_SERVICE, OSType.Windows], [CONDA_ENV_SERVICE, undefined], [CONDA_ENV_FILE_SERVICE, undefined], - options?.onActivation ? [undefined, undefined] : [PIPENV_SERVICE, undefined], + [PIPENV_SERVICE, undefined], [GLOBAL_VIRTUAL_ENV_SERVICE, undefined], [WORKSPACE_VIRTUAL_ENV_SERVICE, undefined], [KNOWN_PATH_SERVICE, undefined], [CURRENT_PATH_SERVICE, undefined] ]; - return keys - .filter((item) => item[0] !== undefined && (item[1] === undefined || item[1] === this.platform.osType)) + + const locators = keys + .filter((item) => item[1] === undefined || item[1] === this.platform.osType) .map((item) => this.serviceContainer.get(IInterpreterLocatorService, item[0])); + + // Set it to true the first time the user selects an interpreter + if (!this.didTriggerInterpreterSuggestions && options?.onSuggestion === true) { + this.didTriggerInterpreterSuggestions = true; + locators.forEach((locator) => (locator.didTriggerInterpreterSuggestions = true)); + } + + return locators; } } diff --git a/src/client/interpreter/locators/services/cacheableLocatorService.ts b/src/client/interpreter/locators/services/cacheableLocatorService.ts index 320b4c9a21b9..440204780fca 100644 --- a/src/client/interpreter/locators/services/cacheableLocatorService.ts +++ b/src/client/interpreter/locators/services/cacheableLocatorService.ts @@ -70,6 +70,8 @@ export abstract class CacheableLocatorService implements IInterpreterLocatorServ private readonly handlersAddedToResource = new Set(); private readonly cacheKeyPrefix: string; private readonly locating = new EventEmitter>(); + private _didTriggerInterpreterSuggestions: boolean; + constructor( @unmanaged() private readonly name: string, @unmanaged() protected readonly serviceContainer: IServiceContainer, @@ -77,6 +79,15 @@ export abstract class CacheableLocatorService implements IInterpreterLocatorServ ) { this._hasInterpreters = createDeferred(); this.cacheKeyPrefix = `INTERPRETERS_CACHE_v3_${name}`; + this._didTriggerInterpreterSuggestions = false; + } + + public get didTriggerInterpreterSuggestions(): boolean { + return this._didTriggerInterpreterSuggestions; + } + + public set didTriggerInterpreterSuggestions(value: boolean) { + this._didTriggerInterpreterSuggestions = value; } public get onLocating(): Event> { diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index 55ae9e09738a..f0459c51806b 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -43,9 +43,15 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer this.configService = this.serviceContainer.get(IConfigurationService); this.pipEnvServiceHelper = this.serviceContainer.get(IPipEnvServiceHelper); } + // tslint:disable-next-line:no-empty public dispose() {} + public async isRelatedPipEnvironment(dir: string, pythonPath: string): Promise { + if (!this.didTriggerInterpreterSuggestions) { + return false; + } + // In PipEnv, the name of the cwd is used as a prefix in the virtual env. if (pythonPath.indexOf(`${path.sep}${path.basename(dir)}-`) === -1) { return false; @@ -55,10 +61,14 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer } public get executable(): string { - return this.configService.getSettings().pipenvPath; + return this.didTriggerInterpreterSuggestions ? this.configService.getSettings().pipenvPath : ''; } public async getInterpreters(resource?: Uri, options?: GetInterpreterLocatorOptions): Promise { + if (!this.didTriggerInterpreterSuggestions) { + return []; + } + const stopwatch = new StopWatch(); const startDiscoveryTime = stopwatch.elapsedTime; @@ -71,6 +81,10 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer } protected getInterpretersImplementation(resource?: Uri): Promise { + if (!this.didTriggerInterpreterSuggestions) { + return Promise.resolve([]); + } + const pipenvCwd = this.getPipenvWorkingDirectory(resource); if (!pipenvCwd) { return Promise.resolve([]); @@ -146,6 +160,7 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer } } } + private async checkIfPipFileExists(cwd: string): Promise { const currentProcess = this.serviceContainer.get(ICurrentProcess); const pipFileName = currentProcess.env[pipEnvFileNameVariable]; diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 7c01b14eb03a..d4602eb61363 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -60,7 +60,6 @@ import { IInterpreterWatcherBuilder, IKnownSearchPathsForInterpreters, INTERPRETER_LOCATOR_SERVICE, - IPipEnvService, IShebangCodeLensProvider, IVirtualEnvironmentsSearchPathProvider, KNOWN_PATH_SERVICE, @@ -200,7 +199,6 @@ export function registerInterpreterTypes(serviceManager: IServiceManager) { WORKSPACE_VIRTUAL_ENV_SERVICE ); serviceManager.addSingleton(IInterpreterLocatorService, PipEnvService, PIPENV_SERVICE); - serviceManager.addSingleton(IPipEnvService, PipEnvService); serviceManager.addSingleton( IInterpreterLocatorService, diff --git a/src/client/interpreter/virtualEnvs/index.ts b/src/client/interpreter/virtualEnvs/index.ts index fb3f7940ef7b..34778820d6f2 100644 --- a/src/client/interpreter/virtualEnvs/index.ts +++ b/src/client/interpreter/virtualEnvs/index.ts @@ -12,7 +12,7 @@ import { ICurrentProcess, IPathUtils } from '../../common/types'; import { getNamesAndValues } from '../../common/utils/enum'; import { noop } from '../../common/utils/misc'; import { IServiceContainer } from '../../ioc/types'; -import { InterpreterType, IPipEnvService } from '../contracts'; +import { IInterpreterLocatorService, InterpreterType, IPipEnvService, PIPENV_SERVICE } from '../contracts'; import { IVirtualEnvironmentManager } from './types'; const PYENVFILES = ['pyvenv.cfg', path.join('..', 'pyvenv.cfg')]; @@ -27,7 +27,10 @@ export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { constructor(@inject(IServiceContainer) private readonly serviceContainer: IServiceContainer) { this.processServiceFactory = serviceContainer.get(IProcessServiceFactory); this.fs = serviceContainer.get(IFileSystem); - this.pipEnvService = serviceContainer.get(IPipEnvService); + this.pipEnvService = serviceContainer.get( + IInterpreterLocatorService, + PIPENV_SERVICE + ) as IPipEnvService; this.workspaceService = serviceContainer.get(IWorkspaceService); } public async getEnvironmentName(pythonPath: string, resource?: Uri): Promise { @@ -37,6 +40,7 @@ export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { const workspaceFolder = resource ? this.workspaceService.getWorkspaceFolder(resource) : undefined; const workspaceUri = workspaceFolder ? workspaceFolder.uri : defaultWorkspaceUri; const grandParentDirName = path.basename(path.dirname(path.dirname(pythonPath))); + if (workspaceUri && (await this.pipEnvService.isRelatedPipEnvironment(workspaceUri.fsPath, pythonPath))) { // In pipenv, return the folder name of the workspace. return path.basename(workspaceUri.fsPath); diff --git a/src/client/startupTelemetry.ts b/src/client/startupTelemetry.ts index e5cc8b03c57a..c84e78c10837 100644 --- a/src/client/startupTelemetry.ts +++ b/src/client/startupTelemetry.ts @@ -129,9 +129,7 @@ async function getActivationTelemetryProps(serviceContainer: IServiceContainer): .then((ver) => (ver ? ver.raw : '')) .catch(() => ''), interpreterService.getActiveInterpreter().catch(() => undefined), - interpreterService - .getInterpreters(mainWorkspaceUri, { onActivation: true }) - .catch(() => []) + interpreterService.getInterpreters(mainWorkspaceUri).catch(() => []) ]); const workspaceFolderCount = workspaceService.hasWorkspaceFolders ? workspaceService.workspaceFolders!.length : 0; const pythonVersion = interpreter && interpreter.version ? interpreter.version.raw : undefined; diff --git a/src/test/activation/activationManager.unit.test.ts b/src/test/activation/activationManager.unit.test.ts index 0dff26f9e6ed..43a14e4978c0 100644 --- a/src/test/activation/activationManager.unit.test.ts +++ b/src/test/activation/activationManager.unit.test.ts @@ -198,7 +198,7 @@ suite('Activation Manager', () => { when(workspaceService.getWorkspaceFolder(resource)).thenReturn(folder2); when(activationService1.activate(resource)).thenResolve(); when(activationService2.activate(resource)).thenResolve(); - when(interpreterService.getInterpreters(anything(), anything())).thenResolve(); + when(interpreterService.getInterpreters(anything())).thenResolve(); autoSelection .setup((a) => a.autoSelectInterpreter(resource)) .returns(() => Promise.resolve()) @@ -232,7 +232,7 @@ suite('Activation Manager', () => { const resource = Uri.parse('two'); when(activationService1.activate(resource)).thenResolve(); when(activationService2.activate(resource)).thenResolve(); - when(interpreterService.getInterpreters(anything(), anything())).thenResolve(); + when(interpreterService.getInterpreters(anything())).thenResolve(); autoSelection .setup((a) => a.autoSelectInterpreter(resource)) .returns(() => Promise.resolve()) @@ -252,7 +252,7 @@ suite('Activation Manager', () => { const resource = Uri.parse('two'); when(activationService1.activate(resource)).thenResolve(); when(activationService2.activate(resource)).thenResolve(); - when(interpreterService.getInterpreters(anything(), anything())).thenResolve(); + when(interpreterService.getInterpreters(anything())).thenResolve(); when(experiments.inExperiment(DeprecatePythonPath.experiment)).thenReturn(true); interpreterPathService .setup((i) => i.copyOldInterpreterStorageValuesToNew(resource)) @@ -278,7 +278,7 @@ suite('Activation Manager', () => { const resource = Uri.parse('two'); when(activationService1.activate(resource)).thenResolve(); when(activationService2.activate(resource)).thenResolve(); - when(interpreterService.getInterpreters(anything(), anything())).thenResolve(); + when(interpreterService.getInterpreters(anything())).thenResolve(); autoSelection .setup((a) => a.autoSelectInterpreter(resource)) .returns(() => Promise.resolve()) diff --git a/src/test/application/diagnostics/checks/macPythonInterpreter.unit.test.ts b/src/test/application/diagnostics/checks/macPythonInterpreter.unit.test.ts index dfaad331374f..eaa9315341d3 100644 --- a/src/test/application/diagnostics/checks/macPythonInterpreter.unit.test.ts +++ b/src/test/application/diagnostics/checks/macPythonInterpreter.unit.test.ts @@ -174,7 +174,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => { .returns(() => Promise.resolve(true)) .verifiable(typemoq.Times.once()); interpreterService - .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true })) + .setup((i) => i.getInterpreters(typemoq.It.isAny())) .returns(() => Promise.resolve([{} as any])) .verifiable(typemoq.Times.never()); interpreterService @@ -204,7 +204,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => { .returns(() => Promise.resolve(true)) .verifiable(typemoq.Times.once()); interpreterService - .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true })) + .setup((i) => i.getInterpreters(typemoq.It.isAny())) .returns(() => Promise.resolve([{} as any])) .verifiable(typemoq.Times.never()); interpreterService @@ -235,7 +235,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => { .returns(() => false) .verifiable(typemoq.Times.once()); interpreterService - .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true })) + .setup((i) => i.getInterpreters(typemoq.It.isAny())) .returns(() => Promise.resolve([{ path: pythonPath } as any, { path: pythonPath } as any])) .verifiable(typemoq.Times.once()); interpreterService @@ -275,7 +275,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => { .returns(() => false) .verifiable(typemoq.Times.once()); interpreterService - .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true })) + .setup((i) => i.getInterpreters(typemoq.It.isAny())) .returns(() => Promise.resolve([ { path: pythonPath } as any, diff --git a/src/test/configuration/interpreterSelector/interpreterSelector.unit.test.ts b/src/test/configuration/interpreterSelector/interpreterSelector.unit.test.ts index 006634c5b2f8..0410a29dac78 100644 --- a/src/test/configuration/interpreterSelector/interpreterSelector.unit.test.ts +++ b/src/test/configuration/interpreterSelector/interpreterSelector.unit.test.ts @@ -106,7 +106,7 @@ suite('Interpreters - selector', () => { return { ...info, ...item }; }); interpreterService - .setup((x) => x.getInterpreters(TypeMoq.It.isAny())) + .setup((x) => x.getInterpreters(TypeMoq.It.isAny(), { onSuggestion: true })) .returns(() => new Promise((resolve) => resolve(initial))); const actual = await selector.getSuggestions(undefined); @@ -139,7 +139,9 @@ suite('Interpreters - selector', () => { test('When in Deprecate PythonPath experiment, remove unsafe interpreters from the suggested interpreters list', async () => { // tslint:disable-next-line: no-any const interpreterList = ['interpreter1', 'interpreter2', 'interpreter3'] as any; - interpreterService.setup((i) => i.getInterpreters(folder1.uri)).returns(() => interpreterList); + interpreterService + .setup((i) => i.getInterpreters(folder1.uri, { onSuggestion: true })) + .returns(() => interpreterList); // tslint:disable-next-line: no-any interpreterSecurityService.setup((i) => i.isSafe('interpreter1' as any)).returns(() => true); // tslint:disable-next-line: no-any diff --git a/src/test/datascience/dataScienceIocContainer.ts b/src/test/datascience/dataScienceIocContainer.ts index 68e2bc003473..5ba7cc96afc7 100644 --- a/src/test/datascience/dataScienceIocContainer.ts +++ b/src/test/datascience/dataScienceIocContainer.ts @@ -327,7 +327,6 @@ import { IKnownSearchPathsForInterpreters, INTERPRETER_LOCATOR_SERVICE, InterpreterType, - IPipEnvService, IShebangCodeLensProvider, IVirtualEnvironmentsSearchPathProvider, KNOWN_PATH_SERVICE, @@ -1065,7 +1064,6 @@ export class DataScienceIocContainer extends UnitTestIocContainer { PipEnvService, PIPENV_SERVICE ); - this.serviceManager.addSingleton(IPipEnvService, PipEnvService); this.serviceManager.addSingleton( IInterpreterLocatorService, WindowsRegistryService, diff --git a/src/test/interpreters/autoSelection/rules/currentPath.unit.test.ts b/src/test/interpreters/autoSelection/rules/currentPath.unit.test.ts index ca3d3b599d06..1215ca88af70 100644 --- a/src/test/interpreters/autoSelection/rules/currentPath.unit.test.ts +++ b/src/test/interpreters/autoSelection/rules/currentPath.unit.test.ts @@ -69,7 +69,7 @@ suite('Interpreters - Auto Selection - Current Path Rule', () => { const manager = mock(InterpreterAutoSelectionService); const resource = Uri.file('x'); - when(locator.getInterpreters(resource, anything())).thenResolve([]); + when(locator.getInterpreters(resource)).thenResolve([]); const nextAction = await rule.onAutoSelectInterpreter(resource, manager); diff --git a/src/test/interpreters/autoSelection/rules/system.unit.test.ts b/src/test/interpreters/autoSelection/rules/system.unit.test.ts index a18baf5d82cf..d5ccb4173cd4 100644 --- a/src/test/interpreters/autoSelection/rules/system.unit.test.ts +++ b/src/test/interpreters/autoSelection/rules/system.unit.test.ts @@ -64,9 +64,8 @@ suite('Interpreters - Auto Selection - System Interpreters Rule', () => { test('Invoke next rule if there are no interpreters in the current path', async () => { const manager = mock(InterpreterAutoSelectionService); const resource = Uri.file('x'); - const options = { onActivation: true }; let setGlobalInterpreterInvoked = false; - when(interpreterService.getInterpreters(resource, deepEqual(options))).thenResolve([]); + when(interpreterService.getInterpreters(resource)).thenResolve([]); when(helper.getBestInterpreter(deepEqual([]))).thenReturn(undefined); rule.setGlobalInterpreter = async (res: any) => { setGlobalInterpreterInvoked = true; @@ -76,17 +75,16 @@ suite('Interpreters - Auto Selection - System Interpreters Rule', () => { const nextAction = await rule.onAutoSelectInterpreter(resource, manager); - verify(interpreterService.getInterpreters(resource, deepEqual(options))).once(); + verify(interpreterService.getInterpreters(resource)).once(); expect(nextAction).to.be.equal(NextAction.runNextRule); expect(setGlobalInterpreterInvoked).to.be.equal(true, 'setGlobalInterpreter not invoked'); }); test('Invoke next rule if there interpreters in the current path but update fails', async () => { const manager = mock(InterpreterAutoSelectionService); const resource = Uri.file('x'); - const options = { onActivation: true }; let setGlobalInterpreterInvoked = false; const interpreterInfo = { path: '1', version: new SemVer('1.0.0') } as any; - when(interpreterService.getInterpreters(resource, deepEqual(options))).thenResolve([interpreterInfo]); + when(interpreterService.getInterpreters(resource)).thenResolve([interpreterInfo]); when(helper.getBestInterpreter(deepEqual([interpreterInfo]))).thenReturn(interpreterInfo); rule.setGlobalInterpreter = async (res: any) => { setGlobalInterpreterInvoked = true; @@ -96,17 +94,16 @@ suite('Interpreters - Auto Selection - System Interpreters Rule', () => { const nextAction = await rule.onAutoSelectInterpreter(resource, manager); - verify(interpreterService.getInterpreters(resource, deepEqual(options))).once(); + verify(interpreterService.getInterpreters(resource)).once(); expect(nextAction).to.be.equal(NextAction.runNextRule); expect(setGlobalInterpreterInvoked).to.be.equal(true, 'setGlobalInterpreter not invoked'); }); test('Do not Invoke next rule if there interpreters in the current path and update does not fail', async () => { const manager = mock(InterpreterAutoSelectionService); const resource = Uri.file('x'); - const options = { onActivation: true }; let setGlobalInterpreterInvoked = false; const interpreterInfo = { path: '1', version: new SemVer('1.0.0') } as any; - when(interpreterService.getInterpreters(resource, deepEqual(options))).thenResolve([interpreterInfo]); + when(interpreterService.getInterpreters(resource)).thenResolve([interpreterInfo]); when(helper.getBestInterpreter(deepEqual([interpreterInfo]))).thenReturn(interpreterInfo); rule.setGlobalInterpreter = async (res: any) => { setGlobalInterpreterInvoked = true; @@ -116,7 +113,7 @@ suite('Interpreters - Auto Selection - System Interpreters Rule', () => { const nextAction = await rule.onAutoSelectInterpreter(resource, manager); - verify(interpreterService.getInterpreters(resource, deepEqual(options))).once(); + verify(interpreterService.getInterpreters(resource)).once(); expect(nextAction).to.be.equal(NextAction.exit); expect(setGlobalInterpreterInvoked).to.be.equal(true, 'setGlobalInterpreter not invoked'); }); diff --git a/src/test/interpreters/locators/index.unit.test.ts b/src/test/interpreters/locators/index.unit.test.ts index 669a2386a205..c4c0fac23ae9 100644 --- a/src/test/interpreters/locators/index.unit.test.ts +++ b/src/test/interpreters/locators/index.unit.test.ts @@ -90,6 +90,7 @@ suite('Interpreters - Locators Index', () => { .setup((l) => l.hasInterpreters) .returns(() => Promise.resolve(true)) .verifiable(TypeMoq.Times.once()); + typeLocator .setup((l) => l.getInterpreters(TypeMoq.It.isValue(resource))) .returns(() => Promise.resolve([interpreter])) @@ -152,6 +153,7 @@ suite('Interpreters - Locators Index', () => { .setup((l) => l.hasInterpreters) .returns(() => Promise.resolve(true)) .verifiable(TypeMoq.Times.once()); + typeLocator .setup((l) => l.getInterpreters(TypeMoq.It.isValue(resource))) .returns(() => Promise.resolve([interpreter])) @@ -171,6 +173,7 @@ suite('Interpreters - Locators Index', () => { }); const expectedInterpreters = locatorsWithInterpreters.map((item) => item.interpreters[0]); + helper .setup((h) => h.mergeInterpreters(TypeMoq.It.isAny())) .returns(() => Promise.resolve(expectedInterpreters)) @@ -180,11 +183,9 @@ suite('Interpreters - Locators Index', () => { locatorsWithInterpreters.forEach((item) => item.locator.verifyAll()); helper.verifyAll(); - expect(interpreters).to.be.lengthOf(locatorsTypes.length); expect(interpreters).to.be.deep.equal(expectedInterpreters); }); - - test(`The pipenv locator service isn't used when the onActivation option is true ${testSuffix}`, async () => { + test(`didTriggerInterpreterSuggestions is set to true in the locators if onSuggestion is true ${testSuffix}`, async () => { const locatorsTypes: string[] = []; if (osType.value === OSType.Windows) { locatorsTypes.push(WINDOWS_REGISTRY_SERVICE); @@ -218,6 +219,7 @@ suite('Interpreters - Locators Index', () => { .setup((l) => l.hasInterpreters) .returns(() => Promise.resolve(true)) .verifiable(TypeMoq.Times.once()); + typeLocator .setup((l) => l.getInterpreters(TypeMoq.It.isValue(resource))) .returns(() => Promise.resolve([interpreter])) @@ -236,24 +238,19 @@ suite('Interpreters - Locators Index', () => { }; }); - const expectedInterpreters: PythonInterpreter[] = []; - locatorsWithInterpreters.forEach((item) => { - if (item.type !== PIPENV_SERVICE) { - expectedInterpreters.push(item.interpreters[0]); - } - }); - - // const expectedInterpreters = locatorsWithInterpreters.map((item) => item.interpreters[0]); helper .setup((h) => h.mergeInterpreters(TypeMoq.It.isAny())) - .returns(() => Promise.resolve(expectedInterpreters)) - .verifiable(TypeMoq.Times.once()); + .returns(() => Promise.resolve(locatorsWithInterpreters.map((item) => item.interpreters[0]))); - const interpreters = await locator.getInterpreters(resource, { onActivation: false }); + await locator.getInterpreters(resource, { onSuggestion: true }); - locatorsWithInterpreters.forEach((item) => item.locator.verifyAll()); - helper.verifyAll(); - expect(interpreters).to.be.deep.equal(expectedInterpreters); + locatorsWithInterpreters.forEach((item) => + item.locator.verify((l) => (l.didTriggerInterpreterSuggestions = true), TypeMoq.Times.once()) + ); + expect(locator.didTriggerInterpreterSuggestions).to.equal( + true, + 'didTriggerInterpreterSuggestions should be set to true.' + ); }); }); }); diff --git a/src/test/interpreters/pipEnvService.unit.test.ts b/src/test/interpreters/pipEnvService.unit.test.ts index bc7b25efe0c6..3d8c2bf995e0 100644 --- a/src/test/interpreters/pipEnvService.unit.test.ts +++ b/src/test/interpreters/pipEnvService.unit.test.ts @@ -60,6 +60,7 @@ suite('Interpreters - PipEnv', () => { let settings: TypeMoq.IMock; let pipenvPathSetting: string; let pipEnvServiceHelper: IPipEnvServiceHelper; + setup(() => { serviceContainer = TypeMoq.Mock.ofType(); const workspaceService = TypeMoq.Mock.ofType(); @@ -138,150 +139,198 @@ suite('Interpreters - PipEnv', () => { pipEnvService = new PipEnvService(serviceContainer.object); }); - test(`Should return an empty list'${testSuffix}`, () => { - const environments = pipEnvService.getInterpreters(resource); - expect(environments).to.be.eventually.deep.equal([]); - }); - 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); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) - .returns(() => Promise.resolve(false)) - .verifiable(TypeMoq.Times.once()); - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.deep.equal([]); - fileSystem.verifyAll(); - }); - test(`Should display warning message if there is a \'PipFile\' but \'pipenv --version\' fails ${testSuffix}`, async () => { - const env = {}; - currentProcess.setup((c) => c.env).returns(() => env); - processService - .setup((p) => - p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny()) - ) - .returns(() => Promise.reject('')); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) - .returns(() => Promise.resolve(true)); - const warningMessage = - "Workspace contains Pipfile but 'pipenv' was not found. Make sure 'pipenv' is on the PATH."; - appShell - .setup((a) => a.showWarningMessage(warningMessage)) - .returns(() => Promise.resolve('')) - .verifiable(TypeMoq.Times.once()); - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.deep.equal([]); - appShell.verifyAll(); - }); - test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' fails with stderr ${testSuffix}`, async () => { - const env = {}; - currentProcess.setup((c) => c.env).returns(() => env); - processService - .setup((p) => - p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny()) - ) - .returns(() => Promise.resolve({ stderr: '', stdout: 'pipenv, version 2018.11.26' })); - processService - .setup((p) => - p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--venv']), TypeMoq.It.isAny()) - ) - .returns(() => Promise.resolve({ stderr: 'Aborted!', stdout: '' })); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) - .returns(() => Promise.resolve(true)); - const warningMessage = - 'Workspace contains Pipfile but the associated virtual environment has not been setup. Setup the virtual environment manually if needed.'; - appShell - .setup((a) => a.showWarningMessage(warningMessage)) - .returns(() => Promise.resolve('')) - .verifiable(TypeMoq.Times.once()); - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.deep.equal([]); - appShell.verifyAll(); - }); - test(`Should return interpreter information${testSuffix}`, async () => { - const env = {}; - 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.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: new SemVer('1.0.0') })); - 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(); - - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.lengthOf(1); - fileSystem.verifyAll(); - }); - test(`Should return interpreter information using PipFile defined in Env variable${testSuffix}`, async () => { - const envPipFile = 'XYZ'; - const env = { - PIPENV_PIPFILE: envPipFile - }; - 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.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: new SemVer('1.0.0') })); - 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()); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(pythonPath))) - .returns(() => Promise.resolve(true)) - .verifiable(); - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.lengthOf(1); - fileSystem.verifyAll(); - }); - test("Must use 'python.pipenvPath' setting", async () => { - pipenvPathSetting = 'spam-spam-pipenv-spam-spam'; - const pipenvExe = pipEnvService.executable; - assert.equal(pipenvExe, 'spam-spam-pipenv-spam-spam', 'Failed to identify pipenv.exe'); + suite('With didTriggerInterpreterSuggestions set to true', () => { + setup(() => { + sinon.stub(pipEnvService, 'didTriggerInterpreterSuggestions').get(() => true); + }); + + teardown(() => { + sinon.restore(); + }); + + test(`Should return an empty list'${testSuffix}`, () => { + const environments = pipEnvService.getInterpreters(resource); + expect(environments).to.be.eventually.deep.equal([]); + }); + 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); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) + .returns(() => Promise.resolve(false)) + .verifiable(TypeMoq.Times.once()); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.deep.equal([]); + fileSystem.verifyAll(); + }); + test(`Should display warning message if there is a \'PipFile\' but \'pipenv --version\' fails ${testSuffix}`, async () => { + const env = {}; + currentProcess.setup((c) => c.env).returns(() => env); + processService + .setup((p) => + p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny()) + ) + .returns(() => Promise.reject('')); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) + .returns(() => Promise.resolve(true)); + const warningMessage = + "Workspace contains Pipfile but 'pipenv' was not found. Make sure 'pipenv' is on the PATH."; + appShell + .setup((a) => a.showWarningMessage(warningMessage)) + .returns(() => Promise.resolve('')) + .verifiable(TypeMoq.Times.once()); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.deep.equal([]); + appShell.verifyAll(); + }); + test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' fails with stderr ${testSuffix}`, async () => { + const env = {}; + currentProcess.setup((c) => c.env).returns(() => env); + processService + .setup((p) => + p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny()) + ) + .returns(() => Promise.resolve({ stderr: '', stdout: 'pipenv, version 2018.11.26' })); + processService + .setup((p) => + p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--venv']), TypeMoq.It.isAny()) + ) + .returns(() => Promise.resolve({ stderr: 'Aborted!', stdout: '' })); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) + .returns(() => Promise.resolve(true)); + const warningMessage = + 'Workspace contains Pipfile but the associated virtual environment has not been setup. Setup the virtual environment manually if needed.'; + appShell + .setup((a) => a.showWarningMessage(warningMessage)) + .returns(() => Promise.resolve('')) + .verifiable(TypeMoq.Times.once()); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.deep.equal([]); + appShell.verifyAll(); + }); + test(`Should return interpreter information${testSuffix}`, async () => { + const env = {}; + 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.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: new SemVer('1.0.0') })); + 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(); + + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.lengthOf(1); + fileSystem.verifyAll(); + }); + test(`Should return interpreter information using PipFile defined in Env variable${testSuffix}`, async () => { + const envPipFile = 'XYZ'; + const env = { + PIPENV_PIPFILE: envPipFile + }; + 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.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: new SemVer('1.0.0') })); + 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()); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(pythonPath))) + .returns(() => Promise.resolve(true)) + .verifiable(); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.lengthOf(1); + fileSystem.verifyAll(); + }); + test("Must use 'python.pipenvPath' setting", async () => { + pipenvPathSetting = 'spam-spam-pipenv-spam-spam'; + const pipenvExe = pipEnvService.executable; + assert.equal(pipenvExe, 'spam-spam-pipenv-spam-spam', 'Failed to identify pipenv.exe'); + }); + + test('Should send telemetry event when calling getInterpreters', async () => { + const sendTelemetryStub = sinon.stub(Telemetry, 'sendTelemetryEvent'); + + await pipEnvService.getInterpreters(resource); + + sinon.assert.calledWith(sendTelemetryStub, EventName.PIPENV_INTERPRETER_DISCOVERY); + sinon.restore(); + }); }); - test('Should send telemetry event when calling getInterpreters', async () => { - const sendTelemetryStub = sinon.stub(Telemetry, 'sendTelemetryEvent'); + suite('With didTriggerInterpreterSuggestions set to false', () => { + setup(() => { + sinon.stub(pipEnvService, 'didTriggerInterpreterSuggestions').get(() => false); + }); + + teardown(() => { + sinon.restore(); + }); + + test('isRelatedPipEnvironment should exit early', async () => { + processService + .setup((p) => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .verifiable(TypeMoq.Times.never()); + + const result = await pipEnvService.isRelatedPipEnvironment('foo', 'some/python/path'); + + expect(result).to.be.equal(false, 'isRelatedPipEnvironment should return false.'); + processService.verifyAll(); + }); + + test('Executable getter should return an empty string', () => { + const executable = pipEnvService.executable; + + expect(executable).to.be.equal('', 'The executable getter should return an empty string.'); + }); + + test('getInterpreters should exit early', async () => { + processService + .setup((p) => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .verifiable(TypeMoq.Times.never()); - await pipEnvService.getInterpreters(resource); + const interpreters = await pipEnvService.getInterpreters(resource); - sinon.assert.calledWith(sendTelemetryStub, EventName.PIPENV_INTERPRETER_DISCOVERY); - sinon.restore(); + expect(interpreters).to.be.lengthOf(0); + processService.verifyAll(); + }); }); }); }); diff --git a/src/test/interpreters/serviceRegistry.unit.test.ts b/src/test/interpreters/serviceRegistry.unit.test.ts index 78a116853b4c..93880dad0ee0 100644 --- a/src/test/interpreters/serviceRegistry.unit.test.ts +++ b/src/test/interpreters/serviceRegistry.unit.test.ts @@ -61,7 +61,6 @@ import { IInterpreterWatcherBuilder, IKnownSearchPathsForInterpreters, INTERPRETER_LOCATOR_SERVICE, - IPipEnvService, IShebangCodeLensProvider, IVirtualEnvironmentsSearchPathProvider, KNOWN_PATH_SERVICE, @@ -148,7 +147,6 @@ suite('Interpreters - Service Registry', () => { [IInterpreterLocatorService, GlobalVirtualEnvService, GLOBAL_VIRTUAL_ENV_SERVICE], [IInterpreterLocatorService, WorkspaceVirtualEnvService, WORKSPACE_VIRTUAL_ENV_SERVICE], [IInterpreterLocatorService, PipEnvService, PIPENV_SERVICE], - [IPipEnvService, PipEnvService], [IInterpreterLocatorService, WindowsRegistryService, WINDOWS_REGISTRY_SERVICE], [IInterpreterLocatorService, KnownPathsService, KNOWN_PATH_SERVICE], diff --git a/src/test/interpreters/virtualEnvManager.unit.test.ts b/src/test/interpreters/virtualEnvManager.unit.test.ts index 96afb7ff872f..d4c3de8c7851 100644 --- a/src/test/interpreters/virtualEnvManager.unit.test.ts +++ b/src/test/interpreters/virtualEnvManager.unit.test.ts @@ -10,7 +10,7 @@ import { Uri, WorkspaceFolder } from 'vscode'; import { IWorkspaceService } from '../../client/common/application/types'; import { IFileSystem } from '../../client/common/platform/types'; import { IProcessServiceFactory } from '../../client/common/process/types'; -import { IPipEnvService } from '../../client/interpreter/contracts'; +import { IInterpreterLocatorService, IPipEnvService, PIPENV_SERVICE } from '../../client/interpreter/contracts'; import { VirtualEnvironmentManager } from '../../client/interpreter/virtualEnvs'; import { IServiceContainer } from '../../client/ioc/types'; @@ -41,7 +41,7 @@ suite('Virtual environment manager', () => { expect(name).to.be.equal(virtualEnvFolderName); }); - test('Use workspacec name as env name', async () => { + test('Use workspace name as env name', async () => { const serviceContainer = TypeMoq.Mock.ofType(); const pipEnvService = TypeMoq.Mock.ofType(); pipEnvService @@ -51,7 +51,9 @@ suite('Virtual environment manager', () => { serviceContainer .setup((s) => s.get(TypeMoq.It.isValue(IProcessServiceFactory))) .returns(() => TypeMoq.Mock.ofType().object); - serviceContainer.setup((s) => s.get(TypeMoq.It.isValue(IPipEnvService))).returns(() => pipEnvService.object); + serviceContainer + .setup((s) => s.get(TypeMoq.It.isValue(IInterpreterLocatorService), TypeMoq.It.isValue(PIPENV_SERVICE))) + .returns(() => pipEnvService.object); serviceContainer .setup((s) => s.get(TypeMoq.It.isValue(IFileSystem))) .returns(() => TypeMoq.Mock.ofType().object); @@ -83,7 +85,9 @@ suite('Virtual environment manager', () => { pipEnvService .setup((w) => w.isRelatedPipEnvironment(TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve(isPipEnvironment)); - serviceContainer.setup((s) => s.get(TypeMoq.It.isValue(IPipEnvService))).returns(() => pipEnvService.object); + serviceContainer + .setup((s) => s.get(TypeMoq.It.isValue(IInterpreterLocatorService), TypeMoq.It.isValue(PIPENV_SERVICE))) + .returns(() => pipEnvService.object); const workspaceService = TypeMoq.Mock.ofType(); workspaceService.setup((w) => w.hasWorkspaceFolders).returns(() => false); if (resource) { diff --git a/src/test/interpreters/virtualEnvs/index.unit.test.ts b/src/test/interpreters/virtualEnvs/index.unit.test.ts index b0ab3c9139c8..7913dbb7edae 100644 --- a/src/test/interpreters/virtualEnvs/index.unit.test.ts +++ b/src/test/interpreters/virtualEnvs/index.unit.test.ts @@ -14,7 +14,12 @@ import { IFileSystem, IPlatformService } from '../../../client/common/platform/t import { IProcessService, IProcessServiceFactory } from '../../../client/common/process/types'; import { ITerminalActivationCommandProvider } from '../../../client/common/terminal/types'; import { ICurrentProcess, IPathUtils } from '../../../client/common/types'; -import { InterpreterType, IPipEnvService } from '../../../client/interpreter/contracts'; +import { + IInterpreterLocatorService, + InterpreterType, + IPipEnvService, + PIPENV_SERVICE +} from '../../../client/interpreter/contracts'; import { VirtualEnvironmentManager } from '../../../client/interpreter/virtualEnvs'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -51,7 +56,9 @@ suite('Virtual Environment Manager', () => { serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IPathUtils))).returns(() => pathUtils.object); serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fs.object); serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IWorkspaceService))).returns(() => workspace.object); - serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IPipEnvService))).returns(() => pipEnvService.object); + serviceContainer + .setup((c) => c.get(TypeMoq.It.isValue(IInterpreterLocatorService), TypeMoq.It.isValue(PIPENV_SERVICE))) + .returns(() => pipEnvService.object); serviceContainer .setup((c) => c.get(TypeMoq.It.isValue(ITerminalActivationCommandProvider), TypeMoq.It.isAny())) .returns(() => terminalActivation.object); diff --git a/src/test/serviceRegistry.ts b/src/test/serviceRegistry.ts index 57a133000870..5ee6414daa70 100644 --- a/src/test/serviceRegistry.ts +++ b/src/test/serviceRegistry.ts @@ -55,7 +55,6 @@ import { IInterpreterLocatorService, IInterpreterService, INTERPRETER_LOCATOR_SERVICE, - IPipEnvService, KNOWN_PATH_SERVICE, PIPENV_SERVICE, WINDOWS_REGISTRY_SERVICE, @@ -379,7 +378,6 @@ export class IocContainer { KnownPathsService, KNOWN_PATH_SERVICE ); - this.serviceManager.addSingleton(IPipEnvService, PipEnvService); this.serviceManager.addSingleton( IInterpreterLocatorHelper,