From b7a4321569cfce8515478816eda4221f577e95fd Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 18 May 2022 11:52:38 -0500 Subject: [PATCH] Only trigger auto environment discovery if a user attempts to choose a different interpreter, or when a particular scope is opened for the first time (microsoft/vscode-python#19162) * Add progress API * Remove old API in favor of this * Revert "Remove old API in favor of this" This reverts commit 9543501576cd063e6aad1ef3ddd7a39cb3354b22. * Revert "Revert "Remove old API in favor of this"" This reverts commit 5bbea78879f2447c6783d7ad98610d4af36abf29. * Fix * Remove old API impl * Add to proposed API * Add get refresh promise options * Change getter to function * Translate progress events to progress promises * Fix combining iterators * Fix tests * Add test for resolver * Add test for reducer * Fixes * Add tests for environment collection service * News entry * Add another test if a query is provided * Add comments and clarify * Fix bug * Use extensionUrl instead of extensionPath (microsoft/vscode-python#19122) * Only trigger auto environment discovery once in the first session for a particular scope * Oops * Ensure old Jupyter APIs still have the old behavior * Trigger auto-discovery if a user is attempting to choose a different interpreter * Fix tests * Temp * Add an option to trigger refresh for the session * Fix bugs * Fix tests * Remove trigger from telemetry * Add tests for options Co-authored-by: Karthik Nadig --- .../news/1 Enhancements/19102.md | 1 + .../client/interpreter/autoSelection/index.ts | 3 +- .../commands/setInterpreter.ts | 7 +- .../client/interpreter/configuration/types.ts | 3 + .../src/client/interpreter/contracts.ts | 13 +- .../client/interpreter/interpreterService.ts | 15 +- .../positron-python/src/client/proposedApi.ts | 2 +- .../src/client/pythonEnvironments/api.ts | 21 +- .../client/pythonEnvironments/base/locator.ts | 13 +- .../composite/envsCollectionService.ts | 46 +++- .../src/client/pythonEnvironments/index.ts | 49 +++- .../client/pythonEnvironments/legacyIOC.ts | 6 +- .../src/client/telemetry/index.ts | 6 - .../commands/setInterpreter.unit.test.ts | 5 +- .../autoSelection/index.unit.test.ts | 211 ++++++++---------- .../src/test/proposedApi.unit.test.ts | 2 +- .../envsCollectionService.unit.test.ts | 123 ++++++---- 17 files changed, 310 insertions(+), 216 deletions(-) create mode 100644 extensions/positron-python/news/1 Enhancements/19102.md diff --git a/extensions/positron-python/news/1 Enhancements/19102.md b/extensions/positron-python/news/1 Enhancements/19102.md new file mode 100644 index 00000000000..2e6822fb2bf --- /dev/null +++ b/extensions/positron-python/news/1 Enhancements/19102.md @@ -0,0 +1 @@ +Only trigger auto environment discovery if a user attempts to choose a different interpreter, or when a particular scope (a workspace folder or globally) is opened for the first time. diff --git a/extensions/positron-python/src/client/interpreter/autoSelection/index.ts b/extensions/positron-python/src/client/interpreter/autoSelection/index.ts index 3117ad1826f..a57577c8c91 100644 --- a/extensions/positron-python/src/client/interpreter/autoSelection/index.ts +++ b/extensions/positron-python/src/client/interpreter/autoSelection/index.ts @@ -200,7 +200,8 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio }); } - const interpreters = await this.interpreterService.getAllInterpreters(resource); + await this.interpreterService.refreshPromise; + const interpreters = this.interpreterService.getInterpreters(resource); const workspaceUri = this.interpreterHelper.getActiveWorkspaceUri(resource); const recommendedInterpreter = this.envTypeComparer.getRecommended(interpreters, workspaceUri?.folderUri); diff --git a/extensions/positron-python/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts b/extensions/positron-python/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts index 6effa8153ce..d55d66581b5 100644 --- a/extensions/positron-python/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts +++ b/extensions/positron-python/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts @@ -111,6 +111,10 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { // times so that the visible items do not change. const preserveOrderWhenFiltering = !!this.interpreterService.refreshPromise; const suggestions = this.getItems(state.workspace); + // Discovery is no longer guranteed to be auto-triggered on extension load, so trigger it when + // user interacts with the interpreter picker but only once per session. Users can rely on the + // refresh button if they want to trigger it more than once. + this.interpreterService.triggerRefresh(undefined, { ifNotTriggerredAlready: true }).ignoreErrors(); state.path = undefined; const currentInterpreterPathDisplay = this.pathUtils.getDisplayName( this.configurationService.getSettings(state.workspace).pythonPath, @@ -134,7 +138,7 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { iconPath: getIcon(REFRESH_BUTTON_ICON), tooltip: InterpreterQuickPickList.refreshInterpreterList, }, - callback: () => this.interpreterService.triggerRefresh(undefined, 'ui').ignoreErrors(), + callback: () => this.interpreterService.triggerRefresh().ignoreErrors(), }, onChangeItem: { event: this.interpreterService.onDidChangeInterpreters, @@ -375,7 +379,6 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { if (!targetConfig) { return; } - const { configTarget } = targetConfig[0]; const wkspace = targetConfig[0].folderUri; const interpreterState: InterpreterStateArgs = { path: undefined, workspace: wkspace }; diff --git a/extensions/positron-python/src/client/interpreter/configuration/types.ts b/extensions/positron-python/src/client/interpreter/configuration/types.ts index 72c5856361f..2621a297def 100644 --- a/extensions/positron-python/src/client/interpreter/configuration/types.ts +++ b/extensions/positron-python/src/client/interpreter/configuration/types.ts @@ -29,6 +29,9 @@ export interface IInterpreterSelector extends Disposable { suggestions: IInterpreterQuickPickItem[], resource: Resource, ): IInterpreterQuickPickItem | undefined; + /** + * @deprecated Only exists for old Jupyter integration. + */ getAllSuggestions(resource: Resource): Promise; getSuggestions(resource: Resource, useFullDisplayName?: boolean): IInterpreterQuickPickItem[]; suggestionToQuickPickItem( diff --git a/extensions/positron-python/src/client/interpreter/contracts.ts b/extensions/positron-python/src/client/interpreter/contracts.ts index ade0bd8fbaf..7c85cb7d2bc 100644 --- a/extensions/positron-python/src/client/interpreter/contracts.ts +++ b/extensions/positron-python/src/client/interpreter/contracts.ts @@ -3,7 +3,11 @@ import { CodeLensProvider, ConfigurationTarget, Disposable, Event, TextDocument, import { FileChangeType } from '../common/platform/fileSystemWatcher'; import { Resource } from '../common/types'; import { PythonEnvSource } from '../pythonEnvironments/base/info'; -import { ProgressNotificationEvent, PythonLocatorQuery } from '../pythonEnvironments/base/locator'; +import { + ProgressNotificationEvent, + PythonLocatorQuery, + TriggerRefreshOptions, +} from '../pythonEnvironments/base/locator'; import { CondaEnvironmentInfo } from '../pythonEnvironments/common/environmentManagers/conda'; import { EnvironmentType, PythonEnvironment } from '../pythonEnvironments/info'; @@ -17,7 +21,7 @@ export type PythonEnvironmentsChangedEvent = { export const IComponentAdapter = Symbol('IComponentAdapter'); export interface IComponentAdapter { readonly onProgress: Event; - triggerRefresh(query?: PythonLocatorQuery & { clearCache?: boolean }, trigger?: 'auto' | 'ui'): Promise; + triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise; getRefreshPromise(): Promise | undefined; readonly onChanged: Event; // VirtualEnvPrompt @@ -63,7 +67,7 @@ export interface ICondaService { export const IInterpreterService = Symbol('IInterpreterService'); export interface IInterpreterService { - triggerRefresh(query?: PythonLocatorQuery & { clearCache?: boolean }, trigger?: 'auto' | 'ui'): Promise; + triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise; readonly refreshPromise: Promise | undefined; readonly onDidChangeInterpreters: Event; onDidChangeInterpreterConfiguration: Event; @@ -71,6 +75,9 @@ export interface IInterpreterService { onDidChangeInterpreterInformation: Event; hasInterpreters(filter?: (e: PythonEnvironment) => Promise): Promise; getInterpreters(resource?: Uri): PythonEnvironment[]; + /** + * @deprecated Only exists for old Jupyter integration. + */ getAllInterpreters(resource?: Uri): Promise; getActiveInterpreter(resource?: Uri): Promise; getInterpreterDetails(pythonPath: string, resoure?: Uri): Promise; diff --git a/extensions/positron-python/src/client/interpreter/interpreterService.ts b/extensions/positron-python/src/client/interpreter/interpreterService.ts index ea4235382ba..653a6081e31 100644 --- a/extensions/positron-python/src/client/interpreter/interpreterService.ts +++ b/extensions/positron-python/src/client/interpreter/interpreterService.ts @@ -20,7 +20,6 @@ import { IInterpreterStatusbarVisibilityFilter, PythonEnvironmentsChangedEvent, } from './contracts'; -import { PythonLocatorQuery } from '../pythonEnvironments/base/locator'; import { traceError, traceLog } from '../logging'; import { Commands, PYTHON_LANGUAGE } from '../common/constants'; import { reportActiveInterpreterChanged } from '../proposedApi'; @@ -29,6 +28,7 @@ import { Interpreters } from '../common/utils/localize'; import { sendTelemetryEvent } from '../telemetry'; import { EventName } from '../telemetry/constants'; import { cache } from '../common/utils/decorators'; +import { PythonLocatorQuery, TriggerRefreshOptions } from '../pythonEnvironments/base/locator'; type StoredPythonEnvironment = PythonEnvironment & { store?: boolean }; @@ -40,11 +40,8 @@ export class InterpreterService implements Disposable, IInterpreterService { return this.pyenvs.hasInterpreters(filter); } - public triggerRefresh( - query?: PythonLocatorQuery & { clearCache?: boolean }, - trigger?: 'auto' | 'ui', - ): Promise { - return this.pyenvs.triggerRefresh(query, trigger); + public triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise { + return this.pyenvs.triggerRefresh(query, options); } public get refreshPromise(): Promise | undefined { @@ -140,6 +137,10 @@ export class InterpreterService implements Disposable, IInterpreterService { } public async getAllInterpreters(resource?: Uri): Promise { + // For backwards compatibility with old Jupyter APIs, ensure a + // fresh refresh is always triggered when using the API. As it is + // no longer auto-triggered by the extension. + this.triggerRefresh(undefined, { ifNotTriggerredAlready: true }).ignoreErrors(); await this.refreshPromise; return this.getInterpreters(resource); } @@ -211,7 +212,7 @@ export class InterpreterService implements Disposable, IInterpreterService { traceLog('Conda envs without Python are known to not work well; fixing conda environment...'); const promise = installer.install(Product.python, await this.getInterpreterDetails(pythonPath)); shell.withProgress(progressOptions, () => promise); - promise.then(() => this.triggerRefresh({ clearCache: true }).ignoreErrors()); + promise.then(() => this.triggerRefresh(undefined, { clearCache: true }).ignoreErrors()); } } } diff --git a/extensions/positron-python/src/client/proposedApi.ts b/extensions/positron-python/src/client/proposedApi.ts index 16c0e3d603a..e4ac6fd83ca 100644 --- a/extensions/positron-python/src/client/proposedApi.ts +++ b/extensions/positron-python/src/client/proposedApi.ts @@ -102,7 +102,7 @@ export function buildProposedApi( return interpreterPathService.update(resource, ConfigurationTarget.WorkspaceFolder, path); }, async refreshEnvironment(options?: RefreshEnvironmentsOptions) { - await discoveryApi.triggerRefresh(options ? { clearCache: options.clearCache } : undefined); + await discoveryApi.triggerRefresh(undefined, options ? { clearCache: options.clearCache } : undefined); const paths = discoveryApi.getEnvs().map((e) => getEnvPath(e.executable.filename, e.location)); return Promise.resolve(paths); }, diff --git a/extensions/positron-python/src/client/pythonEnvironments/api.ts b/extensions/positron-python/src/client/pythonEnvironments/api.ts index 2f2e01e2226..b9c3152a0b6 100644 --- a/extensions/positron-python/src/client/pythonEnvironments/api.ts +++ b/extensions/positron-python/src/client/pythonEnvironments/api.ts @@ -2,15 +2,12 @@ // Licensed under the MIT License. import { Event } from 'vscode'; -import { StopWatch } from '../common/utils/stopWatch'; -import { sendTelemetryEvent } from '../telemetry'; -import { EventName } from '../telemetry/constants'; -import { getEnvPath } from './base/info/env'; import { GetRefreshEnvironmentsOptions, IDiscoveryAPI, ProgressNotificationEvent, PythonLocatorQuery, + TriggerRefreshOptions, } from './base/locator'; export type GetLocatorFunc = () => Promise; @@ -52,20 +49,8 @@ class PythonEnvironments implements IDiscoveryAPI { return this.locator.resolveEnv(env); } - public async triggerRefresh(query?: PythonLocatorQuery, trigger?: 'auto' | 'ui') { - const stopWatch = new StopWatch(); - await this.locator.triggerRefresh(query); - if (!query) { - // Intent is to capture time taken for all of discovery to complete, so make sure - // all interpreters are queried for. - sendTelemetryEvent(EventName.PYTHON_INTERPRETER_DISCOVERY, stopWatch.elapsedTime, { - interpreters: this.getEnvs().length, - environmentsWithoutPython: this.getEnvs().filter( - (e) => getEnvPath(e.executable.filename, e.location).pathType === 'envFolderPath', - ).length, - trigger: trigger ?? 'auto', - }); - } + public async triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions) { + return this.locator.triggerRefresh(query, options); } } diff --git a/extensions/positron-python/src/client/pythonEnvironments/base/locator.ts b/extensions/positron-python/src/client/pythonEnvironments/base/locator.ts index d2585470225..609010501d6 100644 --- a/extensions/positron-python/src/client/pythonEnvironments/base/locator.ts +++ b/extensions/positron-python/src/client/pythonEnvironments/base/locator.ts @@ -193,6 +193,17 @@ export interface GetRefreshEnvironmentsOptions { stage?: ProgressReportStage; } +export type TriggerRefreshOptions = { + /** + * Trigger a fresh refresh. + */ + clearCache?: boolean; + /** + * Only trigger a refresh if it hasn't already been triggered for this session. + */ + ifNotTriggerredAlready?: boolean; +}; + export interface IDiscoveryAPI { /** * Tracks discovery progress for current list of known environments, i.e when it starts, finishes or any other relevant @@ -212,7 +223,7 @@ export interface IDiscoveryAPI { /** * Triggers a new refresh for query if there isn't any already running. */ - triggerRefresh(query?: PythonLocatorQuery & { clearCache?: boolean }, trigger?: 'auto' | 'ui'): Promise; + triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise; /** * Get current list of known environments. */ diff --git a/extensions/positron-python/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts b/extensions/positron-python/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts index 91fc0df3d90..3352441cbe7 100644 --- a/extensions/positron-python/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts +++ b/extensions/positron-python/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts @@ -4,9 +4,13 @@ import { Event, EventEmitter } from 'vscode'; import '../../../../common/extensions'; import { createDeferred, Deferred } from '../../../../common/utils/async'; +import { StopWatch } from '../../../../common/utils/stopWatch'; import { traceError } from '../../../../logging'; +import { sendTelemetryEvent } from '../../../../telemetry'; +import { EventName } from '../../../../telemetry/constants'; import { normalizePath } from '../../../common/externalDependencies'; import { PythonEnvInfo } from '../../info'; +import { getEnvPath } from '../../info/env'; import { GetRefreshEnvironmentsOptions, IDiscoveryAPI, @@ -15,6 +19,7 @@ import { ProgressNotificationEvent, ProgressReportStage, PythonLocatorQuery, + TriggerRefreshOptions, } from '../../locator'; import { getQueryFilter } from '../../locatorUtils'; import { PythonEnvCollectionChangedEvent, PythonEnvsWatcher } from '../../watcher'; @@ -33,6 +38,9 @@ export class EnvsCollectionService extends PythonEnvsWatcher>(); + /** Keeps track of whether a refresh has been triggered for various queries. */ + private wasRefreshTriggeredForQuery = new Map(); + private readonly progress = new EventEmitter(); public get onProgress(): Event { @@ -89,24 +97,25 @@ export class EnvsCollectionService extends PythonEnvsWatcher { + public triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise { + const stopWatch = new StopWatch(); + if (options?.ifNotTriggerredAlready) { + if (this.wasRefreshTriggered(query)) { + return Promise.resolve(); // Refresh was already triggered, return. + } + } let refreshPromise = this.getRefreshPromiseForQuery(query); if (!refreshPromise) { - refreshPromise = this.startRefresh(query); + refreshPromise = this.startRefresh(query, options); } - return refreshPromise; + return refreshPromise.then(() => this.sendTelemetry(query, stopWatch)); } - private startRefresh(query: (PythonLocatorQuery & { clearCache?: boolean }) | undefined): Promise { - if (query?.clearCache) { + private startRefresh(query: PythonLocatorQuery | undefined, options?: TriggerRefreshOptions): Promise { + if (options?.clearCache) { this.cache.clearCache(); } this.createProgressStates(query); @@ -182,6 +191,10 @@ export class EnvsCollectionService extends PythonEnvsWatcher()); + this.wasRefreshTriggeredForQuery.set(query, true); Object.values(ProgressReportStage).forEach((stage) => { this.progressPromises.set(stage, createDeferred()); }); @@ -230,4 +244,16 @@ export class EnvsCollectionService extends PythonEnvsWatcher getEnvPath(e.executable.filename, e.location).pathType === 'envFolderPath').length, + }); + } + } } diff --git a/extensions/positron-python/src/client/pythonEnvironments/index.ts b/extensions/positron-python/src/client/pythonEnvironments/index.ts index 691710504f9..3f2b1748d09 100644 --- a/extensions/positron-python/src/client/pythonEnvironments/index.ts +++ b/extensions/positron-python/src/client/pythonEnvironments/index.ts @@ -12,7 +12,10 @@ import { PythonEnvsReducer } from './base/locators/composite/envsReducer'; import { PythonEnvsResolver } from './base/locators/composite/envsResolver'; import { WindowsPathEnvVarLocator } from './base/locators/lowLevel/windowsKnownPathsLocator'; import { WorkspaceVirtualEnvironmentLocator } from './base/locators/lowLevel/workspaceVirtualEnvLocator'; -import { initializeExternalDependencies as initializeLegacyExternalDependencies } from './common/externalDependencies'; +import { + initializeExternalDependencies as initializeLegacyExternalDependencies, + normCasePath, +} from './common/externalDependencies'; import { ExtensionLocators, WatchRootsArgs, WorkspaceLocators } from './base/locators/wrappers'; import { CustomVirtualEnvironmentLocator } from './base/locators/lowLevel/customVirtualEnvLocator'; import { CondaEnvironmentLocator } from './base/locators/lowLevel/condaLocator'; @@ -52,20 +55,44 @@ export async function initialize(ext: ExtensionState): Promise { /** * Make use of the component (e.g. register with VS Code). */ -export async function activate(api: IDiscoveryAPI, _ext: ExtensionState): Promise { +export async function activate(api: IDiscoveryAPI, ext: ExtensionState): Promise { /** * Force an initial background refresh of the environments. * - * Note API is ready to be queried only after a refresh has been triggered, and extension activation is blocked on API. So, - * * If discovery was never triggered, we need to block extension activation on the refresh trigger. - * * If discovery was already triggered, it maybe the case that this is a new workspace for which it hasn't been triggered yet. - * So always trigger discovery as part of extension activation for now. - * - * TODO: https://github.com/microsoft/vscode-python/issues/17498 - * Once `onInterpretersChanged` event is exposed via API, we can probably expect extensions to rely on that and - * discovery can be triggered after activation, especially in the second case. + * Note API is ready to be queried only after a refresh has been triggered, and extension activation is + * blocked on API being ready. So if discovery was never triggered for a scope, we need to block + * extension activation on the "refresh trigger". */ - api.triggerRefresh().ignoreErrors(); + const folders = vscode.workspace.workspaceFolders; + const wasTriggered = getGlobalStorage(ext.context, 'PYTHON_WAS_DISCOVERY_TRIGGERED', false); + if (!wasTriggered.get()) { + api.triggerRefresh().ignoreErrors(); + wasTriggered.set(true).then(() => { + folders?.forEach(async (folder) => { + const wasTriggeredForFolder = getGlobalStorage( + ext.context, + `PYTHON_WAS_DISCOVERY_TRIGGERED_${normCasePath(folder.uri.fsPath)}`, + false, + ); + await wasTriggeredForFolder.set(true); + }); + }); + } else { + // Figure out which workspace folders need to be activated. + folders?.forEach(async (folder) => { + const wasTriggeredForFolder = getGlobalStorage( + ext.context, + `PYTHON_WAS_DISCOVERY_TRIGGERED_${normCasePath(folder.uri.fsPath)}`, + false, + ); + if (!wasTriggeredForFolder.get()) { + api.triggerRefresh({ + searchLocations: { roots: [folder.uri], doNotIncludeNonRooted: true }, + }).ignoreErrors(); + await wasTriggeredForFolder.set(true); + } + }); + } return { fullyReady: Promise.resolve(), diff --git a/extensions/positron-python/src/client/pythonEnvironments/legacyIOC.ts b/extensions/positron-python/src/client/pythonEnvironments/legacyIOC.ts index 44ac0e5e946..ee01f018eec 100644 --- a/extensions/positron-python/src/client/pythonEnvironments/legacyIOC.ts +++ b/extensions/positron-python/src/client/pythonEnvironments/legacyIOC.ts @@ -9,7 +9,7 @@ import { Resource } from '../common/types'; import { IComponentAdapter, ICondaService, PythonEnvironmentsChangedEvent } from '../interpreter/contracts'; import { IServiceManager } from '../ioc/types'; import { PythonEnvInfo, PythonEnvKind, PythonEnvSource } from './base/info'; -import { IDiscoveryAPI, PythonLocatorQuery } from './base/locator'; +import { IDiscoveryAPI, PythonLocatorQuery, TriggerRefreshOptions } from './base/locator'; import { isMacDefaultPythonPath } from './base/locators/lowLevel/macDefaultLocator'; import { isParentPath } from './common/externalDependencies'; import { EnvironmentType, PythonEnvironment } from './info'; @@ -103,8 +103,8 @@ class ComponentAdapter implements IComponentAdapter { }); } - public triggerRefresh(query?: PythonLocatorQuery, trigger?: 'auto' | 'ui'): Promise { - return this.api.triggerRefresh(query, trigger); + public triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise { + return this.api.triggerRefresh(query, options); } public getRefreshPromise() { diff --git a/extensions/positron-python/src/client/telemetry/index.ts b/extensions/positron-python/src/client/telemetry/index.ts index 70127c68472..317d7f2287b 100644 --- a/extensions/positron-python/src/client/telemetry/index.ts +++ b/extensions/positron-python/src/client/telemetry/index.ts @@ -1270,7 +1270,6 @@ export interface IEventNamePropertyMapping { "duration" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true, "owner": "karrtikr" }, "interpreters" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true , "owner": "karrtikr"}, "environmentsWithoutPython" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true, "owner": "karrtikr" } - "trigger" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "karrtikr" } } */ [EventName.PYTHON_INTERPRETER_DISCOVERY]: { @@ -1282,11 +1281,6 @@ export interface IEventNamePropertyMapping { * The number of environments discovered not containing an interpreter */ environmentsWithoutPython?: number; - /* - * auto : Triggered automatically by the extension or via an API - * ui : Triggered by clicking a button, particularly the refresh button on the interpreter quickpick - */ - trigger: 'auto' | 'ui'; }; /** * Telemetry event sent when pipenv interpreter discovery is executed. diff --git a/extensions/positron-python/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts b/extensions/positron-python/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts index 4473eb8f163..bd08255324f 100644 --- a/extensions/positron-python/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts +++ b/extensions/positron-python/src/test/configuration/interpreterSelector/commands/setInterpreter.unit.test.ts @@ -80,6 +80,7 @@ suite('Set Interpreter Command', () => { workspace = TypeMoq.Mock.ofType(); interpreterService = mock(); when(interpreterService.refreshPromise).thenReturn(undefined); + when(interpreterService.triggerRefresh(anything(), anything())).thenResolve(); workspace.setup((w) => w.rootPath).returns(() => 'rootPath'); configurationService.setup((x) => x.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); @@ -472,9 +473,9 @@ suite('Set Interpreter Command', () => { const refreshButtonCallback = actualParameters!.customButtonSetup?.callback; expect(refreshButtonCallback).to.not.equal(undefined, 'Callback not set'); - when(interpreterService.triggerRefresh(anything(), anything())).thenResolve(); + when(interpreterService.triggerRefresh()).thenResolve(); await refreshButtonCallback!({} as QuickPick); // Invoke callback, meaning that the refresh button is clicked. - verify(interpreterService.triggerRefresh(anything(), anything())).once(); + verify(interpreterService.triggerRefresh()).once(); }); test('Events to update quickpick updates the quickpick accordingly', async () => { diff --git a/extensions/positron-python/src/test/interpreters/autoSelection/index.unit.test.ts b/extensions/positron-python/src/test/interpreters/autoSelection/index.unit.test.ts index da6e7d1dc62..f9c57f86708 100644 --- a/extensions/positron-python/src/test/interpreters/autoSelection/index.unit.test.ts +++ b/extensions/positron-python/src/test/interpreters/autoSelection/index.unit.test.ts @@ -76,25 +76,24 @@ suite('Interpreters - Auto Selection', () => { instance(helper), ); - when(interpreterService.getAllInterpreters(anything())).thenCall((_) => - Promise.resolve([ - { - envType: EnvironmentType.Conda, - envPath: path.join('some', 'conda', 'env'), - version: { major: 3, minor: 7, patch: 2 }, - } as PythonEnvironment, - { - envType: EnvironmentType.Pipenv, - envPath: path.join('some', 'pipenv', 'env'), - version: { major: 3, minor: 10, patch: 0 }, - } as PythonEnvironment, - { - envType: EnvironmentType.Pyenv, - envPath: path.join('some', 'pipenv', 'env'), - version: { major: 3, minor: 5, patch: 0 }, - } as PythonEnvironment, - ]), - ); + when(interpreterService.refreshPromise).thenReturn(undefined); + when(interpreterService.getInterpreters(anything())).thenCall((_) => [ + { + envType: EnvironmentType.Conda, + envPath: path.join('some', 'conda', 'env'), + version: { major: 3, minor: 7, patch: 2 }, + } as PythonEnvironment, + { + envType: EnvironmentType.Pipenv, + envPath: path.join('some', 'pipenv', 'env'), + version: { major: 3, minor: 10, patch: 0 }, + } as PythonEnvironment, + { + envType: EnvironmentType.Pyenv, + envPath: path.join('some', 'pipenv', 'env'), + version: { major: 3, minor: 5, patch: 0 }, + } as PythonEnvironment, + ]); sendTelemetryEventStub = sinon.stub(Telemetry, 'sendTelemetryEvent').callsFake((( eventName: string, @@ -155,26 +154,24 @@ suite('Interpreters - Auto Selection', () => { version: { major: 3, minor: 10, patch: 0 }, } as PythonEnvironment; - when(interpreterService.getAllInterpreters(resource)).thenCall((_) => - Promise.resolve([ - { - envType: EnvironmentType.Conda, - envPath: path.join('some', 'conda', 'env'), - version: { major: 3, minor: 7, patch: 2 }, - } as PythonEnvironment, - { - envType: EnvironmentType.System, - envPath: path.join('/', 'usr', 'bin'), - version: { major: 3, minor: 9, patch: 1 }, - } as PythonEnvironment, - localEnv, - ]), - ); + when(interpreterService.getInterpreters(resource)).thenCall((_) => [ + { + envType: EnvironmentType.Conda, + envPath: path.join('some', 'conda', 'env'), + version: { major: 3, minor: 7, patch: 2 }, + } as PythonEnvironment, + { + envType: EnvironmentType.System, + envPath: path.join('/', 'usr', 'bin'), + version: { major: 3, minor: 9, patch: 1 }, + } as PythonEnvironment, + localEnv, + ]); await autoSelectionService.autoSelectInterpreter(resource); expect(eventFired).to.deep.equal(true, 'event not fired'); - verify(interpreterService.getAllInterpreters(resource)).once(); + verify(interpreterService.getInterpreters(resource)).once(); verify(state.updateValue(localEnv)).once(); }); @@ -185,30 +182,28 @@ suite('Interpreters - Auto Selection', () => { version: { major: 3, minor: 9, patch: 1 }, } as PythonEnvironment; - when(interpreterService.getAllInterpreters(resource)).thenCall((_) => - Promise.resolve([ - { - envType: EnvironmentType.Conda, - envPath: path.join('some', 'conda', 'env'), - version: { major: 3, minor: 7, patch: 2 }, - } as PythonEnvironment, - systemEnv, - { - envType: EnvironmentType.Pipenv, - envPath: path.join('some', 'pipenv', 'env'), - version: { major: 3, minor: 10, patch: 0 }, - } as PythonEnvironment, - ]), - ); + when(interpreterService.getInterpreters(resource)).thenCall((_) => [ + { + envType: EnvironmentType.Conda, + envPath: path.join('some', 'conda', 'env'), + version: { major: 3, minor: 7, patch: 2 }, + } as PythonEnvironment, + systemEnv, + { + envType: EnvironmentType.Pipenv, + envPath: path.join('some', 'pipenv', 'env'), + version: { major: 3, minor: 10, patch: 0 }, + } as PythonEnvironment, + ]); await autoSelectionService.autoSelectInterpreter(resource); expect(eventFired).to.deep.equal(true, 'event not fired'); - verify(interpreterService.getAllInterpreters(resource)).once(); + verify(interpreterService.getInterpreters(resource)).once(); verify(state.updateValue(systemEnv)).once(); }); - test('getAllInterpreters is called with ignoreCache at true if there is no value set in the workspace persistent state', async () => { + test('getInterpreters is called with ignoreCache at true if there is no value set in the workspace persistent state', async () => { const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); const queryState = mock(PersistentState) as PersistentState; @@ -217,20 +212,18 @@ suite('Interpreters - Auto Selection', () => { instance(queryState), ); when(interpreterService.triggerRefresh(anything())).thenResolve(); - when(interpreterService.getAllInterpreters(resource)).thenCall((_) => - Promise.resolve([ - { - envType: EnvironmentType.Conda, - envPath: path.join('some', 'conda', 'env'), - version: { major: 3, minor: 7, patch: 2 }, - } as PythonEnvironment, - { - envType: EnvironmentType.Pipenv, - envPath: path.join('some', 'pipenv', 'env'), - version: { major: 3, minor: 10, patch: 0 }, - } as PythonEnvironment, - ]), - ); + when(interpreterService.getInterpreters(resource)).thenCall((_) => [ + { + envType: EnvironmentType.Conda, + envPath: path.join('some', 'conda', 'env'), + version: { major: 3, minor: 7, patch: 2 }, + } as PythonEnvironment, + { + envType: EnvironmentType.Pipenv, + envPath: path.join('some', 'pipenv', 'env'), + version: { major: 3, minor: 10, patch: 0 }, + } as PythonEnvironment, + ]); autoSelectionService = new InterpreterAutoSelectionServiceTest( instance(workspaceService), @@ -249,7 +242,7 @@ suite('Interpreters - Auto Selection', () => { verify(interpreterService.triggerRefresh(anything())).once(); }); - test('getAllInterpreters is called with ignoreCache at false if there is a value set in the workspace persistent state', async () => { + test('getInterpreters is called with ignoreCache at false if there is a value set in the workspace persistent state', async () => { const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); const queryState = mock(PersistentState) as PersistentState; @@ -258,20 +251,18 @@ suite('Interpreters - Auto Selection', () => { instance(queryState), ); when(interpreterService.triggerRefresh(anything())).thenResolve(); - when(interpreterService.getAllInterpreters(resource)).thenCall((_) => - Promise.resolve([ - { - envType: EnvironmentType.Conda, - envPath: path.join('some', 'conda', 'env'), - version: { major: 3, minor: 7, patch: 2 }, - } as PythonEnvironment, - { - envType: EnvironmentType.Pipenv, - envPath: path.join('some', 'pipenv', 'env'), - version: { major: 3, minor: 10, patch: 0 }, - } as PythonEnvironment, - ]), - ); + when(interpreterService.getInterpreters(resource)).thenCall((_) => [ + { + envType: EnvironmentType.Conda, + envPath: path.join('some', 'conda', 'env'), + version: { major: 3, minor: 7, patch: 2 }, + } as PythonEnvironment, + { + envType: EnvironmentType.Pipenv, + envPath: path.join('some', 'pipenv', 'env'), + version: { major: 3, minor: 10, patch: 0 }, + } as PythonEnvironment, + ]); autoSelectionService = new InterpreterAutoSelectionServiceTest( instance(workspaceService), @@ -287,27 +278,25 @@ suite('Interpreters - Auto Selection', () => { await autoSelectionService.autoSelectInterpreter(resource); - verify(interpreterService.getAllInterpreters(resource)).once(); + verify(interpreterService.getInterpreters(resource)).once(); verify(interpreterService.triggerRefresh(anything())).never(); }); test('Telemetry event is sent with useCachedInterpreter set to false if auto-selection has not been run before', async () => { const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); - when(interpreterService.getAllInterpreters(resource)).thenCall(() => - Promise.resolve([ - { - envType: EnvironmentType.Conda, - envPath: path.join('some', 'conda', 'env'), - version: { major: 3, minor: 7, patch: 2 }, - } as PythonEnvironment, - { - envType: EnvironmentType.Pipenv, - envPath: path.join('some', 'pipenv', 'env'), - version: { major: 3, minor: 10, patch: 0 }, - } as PythonEnvironment, - ]), - ); + when(interpreterService.getInterpreters(resource)).thenCall(() => [ + { + envType: EnvironmentType.Conda, + envPath: path.join('some', 'conda', 'env'), + version: { major: 3, minor: 7, patch: 2 }, + } as PythonEnvironment, + { + envType: EnvironmentType.Pipenv, + envPath: path.join('some', 'pipenv', 'env'), + version: { major: 3, minor: 10, patch: 0 }, + } as PythonEnvironment, + ]); autoSelectionService = new InterpreterAutoSelectionServiceTest( instance(workspaceService), @@ -323,7 +312,7 @@ suite('Interpreters - Auto Selection', () => { await autoSelectionService.autoSelectInterpreter(resource); - verify(interpreterService.getAllInterpreters(resource)).once(); + verify(interpreterService.getInterpreters(resource)).once(); sinon.assert.calledOnce(sendTelemetryEventStub); expect(telemetryEvents).to.deep.equal( [ @@ -339,20 +328,18 @@ suite('Interpreters - Auto Selection', () => { test('Telemetry event is sent with useCachedInterpreter set to true if auto-selection has been run before', async () => { const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); - when(interpreterService.getAllInterpreters(resource)).thenCall(() => - Promise.resolve([ - { - envType: EnvironmentType.Conda, - envPath: path.join('some', 'conda', 'env'), - version: { major: 3, minor: 7, patch: 2 }, - } as PythonEnvironment, - { - envType: EnvironmentType.Pipenv, - envPath: path.join('some', 'pipenv', 'env'), - version: { major: 3, minor: 10, patch: 0 }, - } as PythonEnvironment, - ]), - ); + when(interpreterService.getInterpreters(resource)).thenCall(() => [ + { + envType: EnvironmentType.Conda, + envPath: path.join('some', 'conda', 'env'), + version: { major: 3, minor: 7, patch: 2 }, + } as PythonEnvironment, + { + envType: EnvironmentType.Pipenv, + envPath: path.join('some', 'pipenv', 'env'), + version: { major: 3, minor: 10, patch: 0 }, + } as PythonEnvironment, + ]); autoSelectionService = new InterpreterAutoSelectionServiceTest( instance(workspaceService), @@ -370,7 +357,7 @@ suite('Interpreters - Auto Selection', () => { await autoSelectionService.autoSelectInterpreter(resource); - verify(interpreterService.getAllInterpreters(resource)).once(); + verify(interpreterService.getInterpreters(resource)).once(); sinon.assert.calledTwice(sendTelemetryEventStub); expect(telemetryEvents).to.deep.equal( [ diff --git a/extensions/positron-python/src/test/proposedApi.unit.test.ts b/extensions/positron-python/src/test/proposedApi.unit.test.ts index 1bcef0717db..ad4cdc904a2 100644 --- a/extensions/positron-python/src/test/proposedApi.unit.test.ts +++ b/extensions/positron-python/src/test/proposedApi.unit.test.ts @@ -333,7 +333,7 @@ suite('Proposed Extension API', () => { test('refreshInterpreters: common scenario', async () => { discoverAPI - .setup((d) => d.triggerRefresh(undefined)) + .setup((d) => d.triggerRefresh(undefined, undefined)) .returns(() => Promise.resolve()) .verifiable(typemoq.Times.once()); discoverAPI diff --git a/extensions/positron-python/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts b/extensions/positron-python/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts index c1f523bfbff..5ba356840f7 100644 --- a/extensions/positron-python/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts +++ b/extensions/positron-python/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts @@ -85,7 +85,7 @@ suite('Python envs locator - Environments Collection', async () => { return [...getValidCachedEnvs(), envCached3]; } - function getExpectedEnvs() { + function getExpectedEnvs(doNotIncludeCached?: boolean) { const fakeLocalAppDataPath = path.join(TEST_LAYOUT_ROOT, 'storeApps'); const envCached1 = createEnv(path.join(fakeLocalAppDataPath, 'Microsoft', 'WindowsApps', 'python.exe')); const env1 = createEnv(path.join(TEST_LAYOUT_ROOT, 'conda1', 'python.exe'), undefined, updatedName); @@ -99,6 +99,12 @@ suite('Python envs locator - Environments Collection', async () => { undefined, updatedName, ); + if (doNotIncludeCached) { + return [env1, env2, env3].map((e: PythonEnvCompleteInfo) => { + e.hasCompleteInfo = true; + return e; + }); + } return [envCached1, env1, env2, env3].map((e: PythonEnvCompleteInfo) => { e.hasCompleteInfo = true; return e; @@ -136,43 +142,6 @@ suite('Python envs locator - Environments Collection', async () => { ); }); - test('getEnvs() triggers a refresh in background if cache is empty and no refresh is going on', async () => { - const parentLocator = new SimpleLocator(getLocatorEnvs()); - const cache = await createCollectionCache({ - load: async () => [], - store: async (envs) => { - storage = envs; - }, - }); - collectionService = new EnvsCollectionService(cache, parentLocator); - - let envs = collectionService.getEnvs(); - - assertEnvsEqual(envs, []); - expect(collectionService.getRefreshPromise()).to.not.equal(undefined); - - await collectionService.getRefreshPromise(); - envs = collectionService.getEnvs(); - - assertEnvsEqual( - envs, - getLocatorEnvs().map((e: PythonEnvCompleteInfo) => { - e.hasCompleteInfo = true; - return e; - }), - ); - - envs.forEach((e) => { - sinon.assert.calledWithExactly(reportInterpretersChangedStub, [ - { - path: e.executable.filename, - type: 'add', - }, - ]); - }); - sinon.assert.callCount(reportInterpretersChangedStub, envs.length); - }); - test('triggerRefresh() refreshes the collection and storage with any new environments', async () => { const onUpdated = new EventEmitter(); const locatedEnvs = getLocatorEnvs(); @@ -266,6 +235,41 @@ suite('Python envs locator - Environments Collection', async () => { sinon.assert.callCount(reportInterpretersChangedStub, eventData.length); }); + test('If `ifNotTriggerredAlready` option is set and a refresh for query is already triggered, triggerRefresh() does not trigger a refresh', async () => { + const onUpdated = new EventEmitter(); + const locatedEnvs = getLocatorEnvs(); + let refreshTriggerCount = 0; + const parentLocator = new SimpleLocator(locatedEnvs, { + onUpdated: onUpdated.event, + after: async () => { + refreshTriggerCount += 1; + locatedEnvs.forEach((env, index) => { + const update = cloneDeep(env); + update.name = updatedName; + onUpdated.fire({ index, update }); + }); + onUpdated.fire({ index: locatedEnvs.length - 1, update: undefined }); + // It turns out the last env is invalid, ensure it does not appear in the final result. + onUpdated.fire({ stage: ProgressReportStage.discoveryFinished }); + }, + }); + const cache = await createCollectionCache({ + load: async () => getCachedEnvs(), + store: async (e) => { + storage = e; + }, + }); + collectionService = new EnvsCollectionService(cache, parentLocator); + + await collectionService.triggerRefresh(undefined); + await collectionService.triggerRefresh(undefined, { ifNotTriggerredAlready: true }); + expect(refreshTriggerCount).to.equal(1, 'Refresh should not be triggered in case 1'); + await collectionService.triggerRefresh({ searchLocations: { roots: [] } }, { ifNotTriggerredAlready: true }); + expect(refreshTriggerCount).to.equal(1, 'Refresh should not be triggered in case 2'); + await collectionService.triggerRefresh(undefined); + expect(refreshTriggerCount).to.equal(2, 'Refresh should be triggered in case 3'); + }); + test('Ensure correct events are fired when collection changes on refresh', async () => { const onUpdated = new EventEmitter(); const locatedEnvs = getLocatorEnvs(); @@ -368,6 +372,49 @@ suite('Python envs locator - Environments Collection', async () => { sinon.assert.callCount(reportInterpretersChangedStub, eventData.length); }); + test('If `clearCache` option is set triggerRefresh() clears the cache before refreshing and fires expected events', async () => { + const onUpdated = new EventEmitter(); + const locatedEnvs = getLocatorEnvs(); + const cachedEnvs = getCachedEnvs(); + const parentLocator = new SimpleLocator(locatedEnvs, { + onUpdated: onUpdated.event, + after: async () => { + locatedEnvs.forEach((env, index) => { + const update = cloneDeep(env); + update.name = updatedName; + onUpdated.fire({ index, update }); + }); + onUpdated.fire({ index: locatedEnvs.length - 1, update: undefined }); + // It turns out the last env is invalid, ensure it does not appear in the final result. + onUpdated.fire({ stage: ProgressReportStage.discoveryFinished }); + }, + }); + const cache = await createCollectionCache({ + load: async () => cachedEnvs, + store: async (e) => { + storage = e; + }, + }); + collectionService = new EnvsCollectionService(cache, parentLocator); + + const events: PythonEnvCollectionChangedEvent[] = []; + collectionService.onChanged((e) => { + events.push(e); + }); + + await collectionService.triggerRefresh(undefined, { clearCache: true }); + + let envs = cachedEnvs; + // Ensure when all the events are applied to the original list in sequence, the final list is as expected. + events.forEach((e) => { + envs = applyChangeEventToEnvList(envs, e); + }); + const expected = getExpectedEnvs(true); + assertEnvsEqual(envs, expected); + const queriedEnvs = collectionService.getEnvs(); + assertEnvsEqual(queriedEnvs, expected); + }); + test('Ensure progress stage updates are emitted correctly and refresh promises correct track promise for each stage', async () => { // Arrange const onUpdated = new EventEmitter();