From 423fd2884f47bc6fe619cd15d48e4b2b92100473 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Sat, 22 Aug 2026 19:05:23 -0700 Subject: [PATCH] fix: isolate environment consumers from single-manager failures The Python API's getEnvironments('all'|'global') and refreshEnvironments(undefined) used Promise.all, so a single manager's rejection hid every other manager's completed results. The environment picker also awaited every manager sequentially before showing, so latency was additive and any rejection stopped the picker from opening at all. - Add a private, type-safe collectFromManagers() helper that runs managers concurrently via Promise.allSettled, returns successful results in original manager order, and throws AggregateEnvironmentError only when every manager fails (an empty manager list resolves with []). Each failure is logged inside its own async boundary as the manager settles, so a synchronous throw or a slow/never-settling manager cannot hide the others or defer the reporting of a failure. Because getEnvironments and refresh are uncancellable and can mutate manager state, each operation is awaited to completion rather than timed out, so a slow-but-valid manager is never falsely reported as failed and its results or refresh are never dropped or detached. The public API contract documents the all-failed aggregate shape. - Open the picker immediately with Browse/Create and load each manager behind an async boundary after it is shown, through a small optional onDidShow controller seam on showQuickPickWithButtons that reuses the existing accept/back/cancel/button wiring. Each manager's results publish independently in manager order, so a synchronous throw, a rejection, or a permanently pending manager can't block the others; the seam preserves the active and selected item by reference across publications, and late updates no-op once the picker is closed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- api/CHANGELOG.md | 6 + api/package.json | 2 +- src/api.ts | 7 + .../errors/AggregateEnvironmentError.ts | 11 + src/common/pickers/environments.ts | 94 +++- src/common/window.apis.ts | 41 +- src/features/pythonApi.ts | 56 ++- src/test/common/fakeQuickPick.ts | 100 ++++ src/test/common/pickEnvironment.unit.test.ts | 258 ++++++++++ .../showQuickPickWithButtons.unit.test.ts | 144 ++++++ .../pythonApi.failureIsolation.unit.test.ts | 451 ++++++++++++++++++ 11 files changed, 1141 insertions(+), 29 deletions(-) create mode 100644 src/common/errors/AggregateEnvironmentError.ts create mode 100644 src/test/common/fakeQuickPick.ts create mode 100644 src/test/common/pickEnvironment.unit.test.ts create mode 100644 src/test/common/showQuickPickWithButtons.unit.test.ts create mode 100644 src/test/features/pythonApi.failureIsolation.unit.test.ts diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 616edca22..71ee02bc9 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to the `@vscode/python-environments` API package are documen The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.2.1] + +### Changed + +- Documented that `getEnvironments('all' | 'global')` and `refreshEnvironments(undefined)` isolate per-manager failures: they resolve with the successful managers' results and only reject when every manager fails, in which case the promise rejects with an aggregate error whose `errors` array holds each manager's failure (mirroring the standard `AggregateError` shape). + ## [1.2.0] ### Added diff --git a/api/package.json b/api/package.json index 6b1c6e70e..44a18f4b7 100644 --- a/api/package.json +++ b/api/package.json @@ -1,7 +1,7 @@ { "name": "@vscode/python-environments", "description": "An API facade for the Python Environments extension in VS Code", - "version": "1.2.0", + "version": "1.2.1", "author": { "name": "Microsoft Corporation" }, diff --git a/src/api.ts b/src/api.ts index 2779d27b0..a404fcd80 100644 --- a/src/api.ts +++ b/src/api.ts @@ -1049,6 +1049,9 @@ export interface PythonEnvironmentsApi { * Initiates a refresh of Python environments within the specified scope. * @param scope - The scope within which to search for environments. * @returns A promise that resolves when the search is complete. + * @throws When the scope spans all managers and every manager fails, the promise rejects with an + * aggregate error whose `errors` array holds each manager's failure (mirroring the standard + * `AggregateError` shape). If at least one manager succeeds, the refresh resolves. */ refreshEnvironments(scope: RefreshEnvironmentsScope): Promise; @@ -1056,6 +1059,10 @@ export interface PythonEnvironmentsApi { * Retrieves a list of Python environments within the specified scope. * @param scope - The scope within which to retrieve environments. * @returns A promise that resolves to an array of Python environments. + * @throws When the scope spans all managers and every manager fails, the promise rejects with an + * aggregate error whose `errors` array holds each manager's failure (mirroring the standard + * `AggregateError` shape). If at least one manager succeeds, only the successful managers' + * environments are returned. */ getEnvironments(scope: GetEnvironmentsScope): Promise; diff --git a/src/common/errors/AggregateEnvironmentError.ts b/src/common/errors/AggregateEnvironmentError.ts new file mode 100644 index 000000000..ee4e8b6f1 --- /dev/null +++ b/src/common/errors/AggregateEnvironmentError.ts @@ -0,0 +1,11 @@ +// Minimal stand-in for `AggregateError` (absent from the ES2020 lib the extension targets); carries +// the aggregated `errors` without requiring a tsconfig lib bump. +export class AggregateEnvironmentError extends Error { + public readonly errors: unknown[]; + + constructor(message: string, errors: unknown[]) { + super(message); + this.name = 'AggregateEnvironmentError'; + this.errors = [...errors]; + } +} diff --git a/src/common/pickers/environments.ts b/src/common/pickers/environments.ts index adf9610ef..42f6d290d 100644 --- a/src/common/pickers/environments.ts +++ b/src/common/pickers/environments.ts @@ -8,6 +8,7 @@ import { sendTelemetryEvent } from '../telemetry/sender'; import { isWindows } from '../utils/platformUtils'; import { handlePythonPath } from '../utils/pythonPath'; import { + QuickPickController, showErrorMessage, showOpenDialog, showQuickPick, @@ -121,15 +122,17 @@ async function createEnvironment( } async function pickEnvironmentImpl( - items: (QuickPickItem | (QuickPickItem & { result: PythonEnvironment }))[], + items: EnvironmentPickItem[], managers: InternalEnvironmentManager[], projectEnvManagers: InternalEnvironmentManager[], options: EnvironmentPickOptions, + onDidShow?: (controller: QuickPickController) => void, ): Promise { const selected = await showQuickPickWithButtons(items, { placeHolder: Pickers.Environments.selectEnvironment, ignoreFocusOut: true, showBackButton: options?.showBackButton, + onDidShow, }); if (selected && !Array.isArray(selected)) { @@ -152,7 +155,7 @@ export async function pickEnvironment( projectEnvManagers: InternalEnvironmentManager[], options: EnvironmentPickOptions, ): Promise { - const items: (QuickPickItem | (QuickPickItem & { result: PythonEnvironment }))[] = [ + const items: EnvironmentPickItem[] = [ { label: Interpreter.browsePath, iconPath: new ThemeIcon('folder'), @@ -188,30 +191,71 @@ export async function pickEnvironment( ); } - for (const manager of managers) { - items.push({ - label: manager.displayName, - kind: QuickPickItemKind.Separator, + const onDidShow = (controller: QuickPickController) => { + controller.setBusy(true); + if (managers.length === 0) { + controller.setBusy(false); + return; + } + + const sections: (EnvironmentPickItem[] | undefined)[] = managers.map(() => undefined); + let remaining = managers.length; + + const publish = () => { + const withEnvironments: EnvironmentPickItem[] = [...items]; + for (const section of sections) { + if (section) { + withEnvironments.push(...section); + } + } + controller.setItems(withEnvironments); + }; + + managers.forEach((manager, index) => { + void (async () => { + try { + const environments = await manager.getEnvironments('all'); + const section: EnvironmentPickItem[] = [ + { + label: manager.displayName, + kind: QuickPickItemKind.Separator, + }, + ]; + section.push( + ...environments.map((e) => { + const pathDescription = e.displayPath; + const description = + e.description && e.description.trim() + ? `${e.description} (${pathDescription})` + : pathDescription; + + return { + label: e.displayName ?? e.name, + description: description, + result: e, + manager: manager, + iconPath: getIconPath(e.iconPath), + }; + }), + ); + sections[index] = section; + publish(); + } catch (reason) { + traceError( + `[pickEnvironment] Failed to load environments for manager "${manager.id}"; section skipped.`, + reason, + ); + } finally { + remaining -= 1; + if (remaining === 0) { + controller.setBusy(false); + } + } + })(); }); - const envs = await manager.getEnvironments('all'); - items.push( - ...envs.map((e) => { - const pathDescription = e.displayPath; - const description = - e.description && e.description.trim() ? `${e.description} (${pathDescription})` : pathDescription; - - return { - label: e.displayName ?? e.name, - description: description, - result: e, - manager: manager, - iconPath: getIconPath(e.iconPath), - }; - }), - ); - } + }; - return pickEnvironmentImpl(items, managers, projectEnvManagers, options); + return pickEnvironmentImpl(items, managers, projectEnvManagers, options, onDidShow); } export async function pickEnvironmentFrom(environments: PythonEnvironment[]): Promise { @@ -233,3 +277,5 @@ export async function pickEnvironmentFrom(environments: PythonEnvironment[]): Pr }); return (selected as { e: PythonEnvironment })?.e; } + +type EnvironmentPickItem = QuickPickItem | (QuickPickItem & { result: PythonEnvironment }); diff --git a/src/common/window.apis.ts b/src/common/window.apis.ts index 89326a823..0fc4dad80 100644 --- a/src/common/window.apis.ts +++ b/src/common/window.apis.ts @@ -144,6 +144,12 @@ export interface QuickPickButtonEvent { readonly button: QuickInputButton; } +/** Populates items and toggles the busy indicator on a shown quick pick; no-ops once it settles. */ +export interface QuickPickController { + setItems(items: readonly T[]): void; + setBusy(busy: boolean): void; +} + export function showQuickPick( items: readonly T[] | Thenable, options?: QuickPickOptions, @@ -167,13 +173,19 @@ export function withProgress( export async function showQuickPickWithButtons( items: readonly T[], - options?: QuickPickOptions & { showBackButton?: boolean; buttons?: QuickInputButton[]; selected?: T[] }, + options?: QuickPickOptions & { + showBackButton?: boolean; + buttons?: QuickInputButton[]; + selected?: T[]; + onDidShow?: (controller: QuickPickController) => void; + }, token?: CancellationToken, itemButtonHandler?: (e: QuickPickItemButtonEvent) => void, ): Promise { const quickPick: QuickPick = window.createQuickPick(); const disposables: Disposable[] = [quickPick]; const deferred = createDeferred(); + let disposed = false; quickPick.items = items; quickPick.canSelectMany = options?.canPickMany ?? false; @@ -234,8 +246,35 @@ export async function showQuickPickWithButtons( quickPick.show(); try { + if (options?.onDidShow) { + const controller: QuickPickController = { + setBusy(busy: boolean) { + if (deferred.completed || disposed) { + return; + } + quickPick.busy = busy; + }, + setItems(newItems: readonly T[]) { + if (deferred.completed || disposed) { + return; + } + const activeItems = quickPick.activeItems.filter((item) => newItems.includes(item)); + const selectedItems = quickPick.selectedItems.filter((item) => newItems.includes(item)); + quickPick.items = newItems; + if (activeItems.length > 0) { + quickPick.activeItems = activeItems; + } + if (selectedItems.length > 0) { + quickPick.selectedItems = selectedItems; + } + }, + }; + options.onDidShow(controller); + } + return await deferred.promise; } finally { + disposed = true; disposables.forEach((d) => d.dispose()); } } diff --git a/src/features/pythonApi.ts b/src/features/pythonApi.ts index e93ed0cdb..2cdcee057 100644 --- a/src/features/pythonApi.ts +++ b/src/features/pythonApi.ts @@ -33,6 +33,7 @@ import { ResolveEnvironmentContext, SetEnvironmentScope, } from '../api'; +import { AggregateEnvironmentError } from '../common/errors/AggregateEnvironmentError'; import { traceError, traceInfo } from '../common/logging'; import { pickEnvironmentManager } from '../common/pickers/managers'; import { timeout } from '../common/utils/asyncUtils'; @@ -60,6 +61,50 @@ import { TerminalManager } from './terminal/terminalManager'; const GET_ENVIRONMENT_TIMEOUT_MS = 1000; const GET_ENVIRONMENT_TIMED_OUT = Symbol('getEnvironmentTimedOut'); +// Runs `operation` on every manager concurrently, returns the successful results in manager order, +// logs each failure, and throws AggregateEnvironmentError only when all fail (empty list -> []). +async function collectFromManagers( + managers: readonly InternalEnvironmentManager[], + context: string, + operation: (manager: InternalEnvironmentManager) => Promise, +): Promise { + if (managers.length === 0) { + return []; + } + + // Log each failure inside its own async boundary as the manager settles, so a synchronous throw + // or one slow/never-settling manager cannot hide the others or defer reporting of a failure. + const settled = await Promise.allSettled( + managers.map(async (manager) => { + try { + return await operation(manager); + } catch (err) { + traceError(`[${context}] Environment manager "${manager.id}" failed and was skipped.`, err); + throw err; + } + }), + ); + + const results: T[] = []; + const errors: unknown[] = []; + settled.forEach((outcome) => { + if (outcome.status === 'fulfilled') { + results.push(outcome.value); + } else { + errors.push(outcome.reason); + } + }); + + if (errors.length === managers.length) { + throw new AggregateEnvironmentError( + `[${context}] All ${managers.length} environment manager(s) failed.`, + errors, + ); + } + + return results; +} + export class PythonEnvironmentApiImpl implements PythonEnvironmentApi { private readonly _onDidChangeEnvironments = new EventEmitter(); private readonly _onDidChangeEnvironment = new EventEmitter(); @@ -209,7 +254,9 @@ export class PythonEnvironmentApiImpl implements PythonEnvironmentApi { if (currentScope === undefined) { await waitForAllEnvManagers(); - await Promise.all(this.envManagers.managers.map((manager) => manager.refresh(currentScope))); + await collectFromManagers(this.envManagers.managers, 'refreshEnvironments(all)', (manager) => + manager.refresh(currentScope), + ); return Promise.resolve(); } @@ -224,8 +271,11 @@ export class PythonEnvironmentApiImpl implements PythonEnvironmentApi { const currentScope = checkUri(scope) as GetEnvironmentsScope; if (currentScope === 'all' || currentScope === 'global') { await waitForAllEnvManagers(); - const promises = this.envManagers.managers.map((manager) => manager.getEnvironments(currentScope)); - const items = await Promise.all(promises); + const items = await collectFromManagers( + this.envManagers.managers, + `getEnvironments(${currentScope})`, + (manager) => manager.getEnvironments(currentScope), + ); return items.flat(); } diff --git a/src/test/common/fakeQuickPick.ts b/src/test/common/fakeQuickPick.ts new file mode 100644 index 000000000..fcbf5582a --- /dev/null +++ b/src/test/common/fakeQuickPick.ts @@ -0,0 +1,100 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { when } from 'ts-mockito'; +import { + EventEmitter, + QuickInputButton, + QuickPick, + QuickPickItem, + QuickPickItemButtonEvent, +} from 'vscode'; +import { mockedVSCodeNamespaces } from '../unittests'; + +export class FakeQuickPick { + private _items: readonly T[] = []; + // Models VS Code: assigning `items` moves focus to the first item and clears the selection. + public get items(): readonly T[] { + return this._items; + } + public set items(value: readonly T[]) { + this._items = value; + this.activeItems = value.length > 0 ? [value[0]] : []; + this.selectedItems = []; + } + public activeItems: readonly T[] = []; + public selectedItems: readonly T[] = []; + public value = ''; + public placeholder: string | undefined; + public title: string | undefined; + public busy = false; + public enabled = true; + public canSelectMany = false; + public ignoreFocusOut = false; + public matchOnDescription = false; + public matchOnDetail = false; + public keepScrollPosition = false; + public buttons: readonly QuickInputButton[] = []; + public step: number | undefined; + public totalSteps: number | undefined; + + public shown = false; + public disposed = false; + + private readonly _onDidAccept = new EventEmitter(); + private readonly _onDidHide = new EventEmitter(); + private readonly _onDidChangeValue = new EventEmitter(); + private readonly _onDidChangeActive = new EventEmitter(); + private readonly _onDidChangeSelection = new EventEmitter(); + private readonly _onDidTriggerButton = new EventEmitter(); + private readonly _onDidTriggerItemButton = new EventEmitter>(); + + public readonly onDidAccept = this._onDidAccept.event; + public readonly onDidHide = this._onDidHide.event; + public readonly onDidChangeValue = this._onDidChangeValue.event; + public readonly onDidChangeActive = this._onDidChangeActive.event; + public readonly onDidChangeSelection = this._onDidChangeSelection.event; + public readonly onDidTriggerButton = this._onDidTriggerButton.event; + public readonly onDidTriggerItemButton = this._onDidTriggerItemButton.event; + + public show(): void { + this.shown = true; + } + + public hide(): void { + this._onDidHide.fire(); + } + + public dispose(): void { + this.disposed = true; + } + + public accept(item?: T): void { + if (item) { + this.selectedItems = [item]; + } + this._onDidAccept.fire(); + } + + public triggerButton(button: QuickInputButton): void { + this._onDidTriggerButton.fire(button); + } + + public cancel(): void { + this.hide(); + } + + public asQuickPick(): QuickPick { + return this as unknown as QuickPick; + } +} + +export function useFakeQuickPick(): FakeQuickPick { + const fake = new FakeQuickPick(); + when(mockedVSCodeNamespaces.window!.createQuickPick()).thenReturn(fake.asQuickPick()); + return fake; +} + +export function flush(): Promise { + return new Promise((resolve) => setImmediate(resolve)); +} diff --git a/src/test/common/pickEnvironment.unit.test.ts b/src/test/common/pickEnvironment.unit.test.ts new file mode 100644 index 000000000..3f764c33a --- /dev/null +++ b/src/test/common/pickEnvironment.unit.test.ts @@ -0,0 +1,258 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as sinon from 'sinon'; +import { QuickPickItem } from 'vscode'; +import { PythonEnvironment } from '../../api'; +import * as logging from '../../common/logging'; +import { Common, Interpreter } from '../../common/localize'; +import { pickEnvironment } from '../../common/pickers/environments'; +import { createDeferred } from '../../common/utils/deferred'; +import { InternalEnvironmentManager } from '../../internal.api'; +import { FakeQuickPick, flush, useFakeQuickPick } from './fakeQuickPick'; + +suite('pickEnvironment - opens promptly and isolates manager failures', () => { + let fake: FakeQuickPick; + let traceErrorStub: sinon.SinonStub; + + const STATIC_LABELS = [Interpreter.browsePath, '', Interpreter.createVirtualEnvironment]; + + setup(() => { + fake = useFakeQuickPick(); + traceErrorStub = sinon.stub(logging, 'traceError'); + sinon.stub(logging, 'traceInfo'); + sinon.stub(logging, 'traceVerbose'); + sinon.stub(logging, 'traceWarn'); + sinon.stub(logging, 'traceLog'); + }); + + teardown(() => { + sinon.restore(); + }); + + test('opens immediately with browse/create before any manager resolves', async () => { + const m1 = controllableManager('m1', 'Manager One'); + const pick = pickEnvironment([m1.manager], [], { projects: [] }); + + assert.ok(fake.shown, 'picker should be shown before slow managers resolve'); + assert.strictEqual(fake.busy, true, 'picker should be busy while managers load'); + assert.deepStrictEqual(labels(), STATIC_LABELS); + + m1.resolve([makeEnv('env-1', '/p/env-1')]); + await flush(); + + assert.deepStrictEqual(labels(), [...STATIC_LABELS, 'Manager One', 'env-1']); + assert.strictEqual(fake.busy, false, 'busy should clear once all loads settle'); + + fake.accept(fake.items.find((i) => i.label === 'env-1')!); + const result = await pick; + assert.strictEqual(result?.envId.id, 'env-1'); + }); + + test('keeps fixed manager order regardless of completion order', async () => { + const m1 = controllableManager('m1', 'One'); + const m2 = controllableManager('m2', 'Two'); + const pick = pickEnvironment([m1.manager, m2.manager], [], { projects: [] }); + + m2.resolve([makeEnv('e2', '/p/e2')]); + m1.resolve([makeEnv('e1', '/p/e1')]); + await flush(); + + assert.deepStrictEqual(labels(), [...STATIC_LABELS, 'One', 'e1', 'Two', 'e2']); + + fake.cancel(); + await pick; + }); + + test('one manager failing does not hide the others and is logged', async () => { + const m1 = controllableManager('m1', 'One'); + const m2 = controllableManager('m2', 'Two'); + const pick = pickEnvironment([m1.manager, m2.manager], [], { projects: [] }); + + const m1Error = new Error('boom'); + m1.reject(m1Error); + m2.resolve([makeEnv('e2', '/p/e2')]); + await flush(); + + assert.deepStrictEqual(labels(), [...STATIC_LABELS, 'Two', 'e2'], 'the surviving manager still shows'); + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[pickEnvironment] Failed to load environments for manager "m1"; section skipped.', + m1Error, + ); + + fake.accept(fake.items.find((i) => i.label === 'e2')!); + const result = await pick; + assert.strictEqual(result?.envId.id, 'e2'); + }); + + test('every manager failing still leaves a usable browse/create picker', async () => { + const m1 = controllableManager('m1', 'One'); + const m2 = controllableManager('m2', 'Two'); + const pick = pickEnvironment([m1.manager, m2.manager], [], { projects: [] }); + + const m1Error = new Error('boom-1'); + const m2Error = new Error('boom-2'); + m1.reject(m1Error); + m2.reject(m2Error); + await flush(); + + assert.deepStrictEqual(labels(), STATIC_LABELS, 'only browse/create remain when all managers fail'); + assert.strictEqual(fake.busy, false, 'busy clears even when every manager fails'); + assert.strictEqual(traceErrorStub.callCount, 2, 'each failed manager is logged exactly once'); + sinon.assert.calledWithExactly( + traceErrorStub, + '[pickEnvironment] Failed to load environments for manager "m1"; section skipped.', + m1Error, + ); + sinon.assert.calledWithExactly( + traceErrorStub, + '[pickEnvironment] Failed to load environments for manager "m2"; section skipped.', + m2Error, + ); + + fake.cancel(); + assert.strictEqual(await pick, undefined, 'the picker can still be dismissed'); + }); + + test('shows a synchronous recommended environment immediately', async () => { + const m1 = controllableManager('m1', 'One'); + const recommended = makeEnv('rec', '/p/rec', 'Recommended Env'); + const pick = pickEnvironment([m1.manager], [], { projects: [], recommended }); + + assert.deepStrictEqual(labels(), [...STATIC_LABELS, Common.recommended, 'Recommended Env']); + + m1.resolve([]); + await flush(); + fake.cancel(); + await pick; + }); + + test('an empty manager list opens and settles with only browse/create', async () => { + const pick = pickEnvironment([], [], { projects: [] }); + + assert.deepStrictEqual(labels(), STATIC_LABELS); + await flush(); + assert.strictEqual(fake.busy, false); + + fake.cancel(); + assert.strictEqual(await pick, undefined); + }); + + test('late manager results after the picker closes are ignored', async () => { + const m1 = controllableManager('m1', 'One'); + const pick = pickEnvironment([m1.manager], [], { projects: [] }); + + fake.cancel(); + assert.strictEqual(await pick, undefined); + + m1.resolve([makeEnv('late', '/p/late')]); + await flush(); + assert.deepStrictEqual(labels(), STATIC_LABELS, 'no items should be added after the picker closed'); + }); + + test('a manager throwing synchronously is isolated and does not reject the picker', async () => { + const throwError = new Error('sync boom'); + const throwing = { + id: 'boom', + displayName: 'Boom', + getEnvironments: () => { + throw throwError; + }, + refresh: async () => {}, + } as unknown as InternalEnvironmentManager; + const m2 = controllableManager('m2', 'Two'); + const pick = pickEnvironment([throwing, m2.manager], [], { projects: [] }); + + assert.ok(fake.shown, 'picker opens despite a synchronous manager throw'); + + m2.resolve([makeEnv('e2', '/p/e2')]); + await flush(); + + assert.deepStrictEqual(labels(), [...STATIC_LABELS, 'Two', 'e2'], 'the surviving manager still shows'); + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[pickEnvironment] Failed to load environments for manager "boom"; section skipped.', + throwError, + ); + + fake.accept(fake.items.find((i) => i.label === 'e2')!); + assert.strictEqual((await pick)?.envId.id, 'e2'); + }); + + test('a permanently pending manager does not block other managers from showing', async () => { + const pending = controllableManager('pending', 'Pending'); + const done = controllableManager('done', 'Done'); + const pick = pickEnvironment([pending.manager, done.manager], [], { projects: [] }); + + done.resolve([makeEnv('e-done', '/p/e-done')]); + await flush(); + + assert.deepStrictEqual( + labels(), + [...STATIC_LABELS, 'Done', 'e-done'], + 'a completed manager appears even though another manager never settles', + ); + assert.strictEqual(fake.busy, true, 'busy stays while a manager is still pending'); + + fake.accept(fake.items.find((i) => i.label === 'e-done')!); + assert.strictEqual((await pick)?.envId.id, 'e-done'); + }); + + test('preserves the active and selected item across manager publications', async () => { + const m1 = controllableManager('m1', 'One'); + const pick = pickEnvironment([m1.manager], [], { projects: [] }); + + const createItem = fake.items.find((i) => i.label === Interpreter.createVirtualEnvironment)!; + fake.activeItems = [createItem]; + fake.selectedItems = [createItem]; + + m1.resolve([makeEnv('e1', '/p/e1')]); + await flush(); + + assert.deepStrictEqual(labels(), [...STATIC_LABELS, 'One', 'e1']); + assert.strictEqual( + fake.activeItems[0], + createItem, + 'the item the user navigated to stays active after a manager publishes', + ); + assert.strictEqual(fake.selectedItems[0], createItem, 'the selected item is preserved after publication'); + + fake.cancel(); + await pick; + }); + + const labels = (): string[] => fake.items.map((i) => i.label); +}); + +function makeEnv(id: string, execPath: string, displayName = id, managerId = 'test-manager'): PythonEnvironment { + return { + envId: { id, managerId }, + name: id, + displayName, + displayPath: execPath, + execInfo: { run: { executable: execPath } }, + } as unknown as PythonEnvironment; +} + +interface ControllableManager { + manager: InternalEnvironmentManager; + resolve: (envs: PythonEnvironment[]) => void; + reject: (err: unknown) => void; +} + +function controllableManager(id: string, displayName = id): ControllableManager { + const deferred = createDeferred(); + const manager = { + id, + displayName, + getEnvironments: () => deferred.promise, + refresh: async () => {}, + } as unknown as InternalEnvironmentManager; + return { + manager, + resolve: (envs) => deferred.resolve(envs), + reject: (err) => deferred.reject(err), + }; +} diff --git a/src/test/common/showQuickPickWithButtons.unit.test.ts b/src/test/common/showQuickPickWithButtons.unit.test.ts new file mode 100644 index 000000000..51f63c783 --- /dev/null +++ b/src/test/common/showQuickPickWithButtons.unit.test.ts @@ -0,0 +1,144 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import { CancellationTokenSource, QuickInputButton, QuickInputButtons, QuickPickItem } from 'vscode'; +import { QuickPickController, showQuickPickWithButtons } from '../../common/window.apis'; +import { flush, useFakeQuickPick } from './fakeQuickPick'; + +suite('showQuickPickWithButtons - onDidShow controller seam', () => { + test('static callers (no onDidShow): accept resolves with the selected item', async () => { + const fake = useFakeQuickPick(); + const items: QuickPickItem[] = [{ label: 'x' }, { label: 'y' }]; + + const promise = showQuickPickWithButtons(items); + await flush(); + + assert.ok(fake.shown, 'quick pick should be shown'); + assert.deepStrictEqual(fake.items, items, 'items should be assigned up front'); + + fake.accept(items[1]); + assert.strictEqual(await promise, items[1]); + assert.ok(fake.disposed, 'quick pick should be disposed after settling'); + }); + + test('static callers: hide resolves with undefined', async () => { + const fake = useFakeQuickPick(); + const promise = showQuickPickWithButtons([{ label: 'x' }]); + await flush(); + + fake.cancel(); + assert.strictEqual(await promise, undefined); + }); + + test('static callers: Back button rejects with QuickInputButtons.Back', async () => { + const fake = useFakeQuickPick(); + const promise = showQuickPickWithButtons([{ label: 'x' }], { showBackButton: true }); + await flush(); + + assert.deepStrictEqual(fake.buttons, [QuickInputButtons.Back], 'back button should be wired'); + + fake.triggerButton(QuickInputButtons.Back); + await assert.rejects( + () => promise, + (err: unknown) => err === QuickInputButtons.Back, + ); + }); + + test('static callers: custom button rejects with { item, button }', async () => { + const fake = useFakeQuickPick(); + const button = { iconPath: undefined } as unknown as QuickInputButton; + const items: QuickPickItem[] = [{ label: 'x' }]; + + const promise = showQuickPickWithButtons(items, { buttons: [button] }); + await flush(); + + fake.selectedItems = [items[0]]; + fake.triggerButton(button); + + await assert.rejects( + () => promise, + (err: unknown) => { + const e = err as { item: QuickPickItem[]; button: QuickInputButton }; + assert.strictEqual(e.button, button); + assert.deepStrictEqual(e.item, [items[0]]); + return true; + }, + ); + }); + + test('static callers: token cancellation hides and resolves undefined', async () => { + useFakeQuickPick(); + const cts = new CancellationTokenSource(); + + const promise = showQuickPickWithButtons([{ label: 'x' }], {}, cts.token); + await flush(); + + cts.cancel(); + assert.strictEqual(await promise, undefined); + }); + + test('onDidShow receives a controller that can populate items and toggle busy', async () => { + const fake = useFakeQuickPick(); + const items: QuickPickItem[] = [{ label: 'a' }, { label: 'b' }]; + let controller: QuickPickController | undefined; + + const promise = showQuickPickWithButtons(items, { + onDidShow: (c) => { + controller = c; + }, + }); + await flush(); + + assert.ok(controller, 'controller should be delivered after show'); + + controller!.setBusy(true); + assert.strictEqual(fake.busy, true, 'setBusy should update the quick pick'); + + const extended: QuickPickItem[] = [...items, { label: 'c' }]; + controller!.setItems(extended); + assert.deepStrictEqual(fake.items, extended, 'setItems should replace the item list'); + + fake.accept(items[0]); + assert.strictEqual(await promise, items[0]); + }); + + test('controller mutations are ignored after the picker settles', async () => { + const fake = useFakeQuickPick(); + const items: QuickPickItem[] = [{ label: 'a' }]; + let controller: QuickPickController | undefined; + + const promise = showQuickPickWithButtons(items, { + onDidShow: (ctl) => { + controller = ctl; + }, + }); + await flush(); + + controller!.setBusy(true); + fake.cancel(); + assert.strictEqual(await promise, undefined); + + assert.doesNotThrow(() => controller!.setItems([{ label: 'late' }])); + assert.doesNotThrow(() => controller!.setBusy(false)); + assert.strictEqual(fake.busy, true, 'busy state must not change after settle'); + assert.deepStrictEqual(fake.items, items, 'items must not change after settle'); + }); + + test('a synchronous throw from onDidShow still disposes the quick pick', async () => { + const fake = useFakeQuickPick(); + const boom = new Error('onDidShow boom'); + + const promise = showQuickPickWithButtons([{ label: 'a' }], { + onDidShow: () => { + throw boom; + }, + }); + + await assert.rejects( + () => promise, + (err: unknown) => err === boom, + ); + assert.ok(fake.disposed, 'the quick pick must be disposed even when onDidShow throws'); + }); +}); diff --git a/src/test/features/pythonApi.failureIsolation.unit.test.ts b/src/test/features/pythonApi.failureIsolation.unit.test.ts new file mode 100644 index 000000000..8e2a03417 --- /dev/null +++ b/src/test/features/pythonApi.failureIsolation.unit.test.ts @@ -0,0 +1,451 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as sinon from 'sinon'; +import { Disposable, EventEmitter } from 'vscode'; +import { PythonEnvironment } from '../../api'; +import { AggregateEnvironmentError } from '../../common/errors/AggregateEnvironmentError'; +import * as extensionApis from '../../common/extension.apis'; +import * as logging from '../../common/logging'; +import * as telemetrySender from '../../common/telemetry/sender'; +import { PythonEnvironmentApiImpl } from '../../features/pythonApi'; +import { _resetManagerReadyForTesting, createManagerReady } from '../../features/common/managerReady'; +import * as settingHelpers from '../../features/settings/settingHelpers'; +import { + DidChangeEnvironmentManagerEventArgs, + DidChangePackageManagerEventArgs, + EnvironmentManagers, + InternalEnvironmentManager, +} from '../../internal.api'; + +const DEFAULT_MANAGER_ID = 'ms-python.python:venv'; + +suite('PythonEnvironmentApiImpl - manager failure isolation', () => { + let envManagerEmitter: EventEmitter; + let pkgManagerEmitter: EventEmitter; + let disposables: Disposable[]; + let traceErrorStub: sinon.SinonStub; + let currentManagers: InternalEnvironmentManager[]; + let api: PythonEnvironmentApiImpl; + + setup(() => { + disposables = []; + currentManagers = []; + _resetManagerReadyForTesting(); + + envManagerEmitter = new EventEmitter(); + pkgManagerEmitter = new EventEmitter(); + + traceErrorStub = sinon.stub(logging, 'traceError'); + sinon.stub(logging, 'traceInfo'); + sinon.stub(logging, 'traceWarn'); + sinon.stub(telemetrySender, 'sendTelemetryEvent'); + sinon.stub(extensionApis, 'getExtension').returns({ + id: 'ms-python.python', + isActive: true, + } as unknown as ReturnType); + sinon.stub(settingHelpers, 'getDefaultEnvManagerSetting').returns(DEFAULT_MANAGER_ID); + sinon.stub(settingHelpers, 'getDefaultPkgManagerSetting').returns('ms-python.python:pip'); + + const mockEm = { + get managers() { + return currentManagers; + }, + onDidChangeActiveEnvironment: new EventEmitter().event, + onDidChangeEnvironmentManager: envManagerEmitter.event, + onDidChangePackageManager: pkgManagerEmitter.event, + } as unknown as EnvironmentManagers; + + const mockPm = { + getProjects: () => [], + onDidChangeProjects: new EventEmitter().event, + } as unknown as ConstructorParameters[1]; + + const mockEvm = { + onDidChangeEnvironmentVariables: new EventEmitter().event, + } as unknown as ConstructorParameters[4]; + + createManagerReady(mockEm, mockPm, disposables); + envManagerEmitter.fire({ + kind: 'registered', + manager: { id: DEFAULT_MANAGER_ID } as unknown as InternalEnvironmentManager, + }); + + api = new PythonEnvironmentApiImpl( + mockEm, + mockPm, + {} as unknown as ConstructorParameters[2], + {} as unknown as ConstructorParameters[3], + mockEvm, + disposables, + ); + }); + + teardown(() => { + disposables.forEach((d) => d.dispose()); + envManagerEmitter.dispose(); + pkgManagerEmitter.dispose(); + sinon.restore(); + _resetManagerReadyForTesting(); + }); + + suite('getEnvironments(all)', () => { + test('partial success: one manager failing does not hide the others', async () => { + const e1 = makeEnv('one'); + const e3 = makeEnv('three'); + const m2Error = new Error('m2 boom'); + currentManagers = [ + makeManager('m1', { envs: [e1] }), + makeManager('m2', { getError: m2Error }), + makeManager('m3', { envs: [e3] }), + ]; + + const result = await api.getEnvironments('all'); + + assert.deepStrictEqual( + result.map((e) => e.envId.id), + ['one', 'three'], + 'should return successful managers only, in original order', + ); + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[getEnvironments(all)] Environment manager "m2" failed and was skipped.', + m2Error, + ); + }); + + test('a manager throwing synchronously is isolated like an async rejection', async () => { + const e1 = makeEnv('one'); + const e3 = makeEnv('three'); + const syncError = new Error('sync boom'); + const syncThrower = { + id: 'm2', + displayName: 'm2', + getEnvironments: () => { + throw syncError; + }, + refresh: async () => {}, + } as unknown as InternalEnvironmentManager; + currentManagers = [makeManager('m1', { envs: [e1] }), syncThrower, makeManager('m3', { envs: [e3] })]; + + const result = await api.getEnvironments('all'); + + assert.deepStrictEqual( + result.map((e) => e.envId.id), + ['one', 'three'], + 'a synchronous throw must not hide the other managers results', + ); + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[getEnvironments(all)] Environment manager "m2" failed and was skipped.', + syncError, + ); + }); + + test('results stay in original manager order even when a later manager resolves first', async () => { + const e1 = makeEnv('one'); + const e2 = makeEnv('two'); + const e3 = makeEnv('three'); + currentManagers = [ + makeManager('m1', { envs: [e1], delayMs: 25 }), + makeManager('m2', { envs: [e2], delayMs: 10 }), + makeManager('m3', { envs: [e3], delayMs: 0 }), + ]; + + const result = await api.getEnvironments('all'); + + assert.deepStrictEqual( + result.map((e) => e.envId.id), + ['one', 'two', 'three'], + 'flattened result must follow manager order, not completion order', + ); + }); + + test('total failure: throws AggregateEnvironmentError with all reasons in order', async () => { + const err1 = new Error('first'); + const err2 = new Error('second'); + currentManagers = [ + makeManager('m1', { getError: err1 }), + makeManager('m2', { getError: err2 }), + ]; + + await assert.rejects( + () => api.getEnvironments('all'), + (err: unknown) => { + assert.ok(err instanceof AggregateEnvironmentError, 'should throw AggregateEnvironmentError'); + assert.deepStrictEqual(err.errors, [err1, err2], 'should carry all reasons in manager order'); + return true; + }, + ); + assert.strictEqual(traceErrorStub.callCount, 2, 'both failures should be logged exactly once'); + sinon.assert.calledWithExactly( + traceErrorStub, + '[getEnvironments(all)] Environment manager "m1" failed and was skipped.', + err1, + ); + sinon.assert.calledWithExactly( + traceErrorStub, + '[getEnvironments(all)] Environment manager "m2" failed and was skipped.', + err2, + ); + }); + + test('a slow successful manager is awaited, not timed out, so a failing peer never rejects the caller or drops the slow result', async () => { + const clock = sinon.useFakeTimers(); + const failing = new Error('boom'); + const slowEnv = makeEnv('slow-env'); + currentManagers = [ + makeManager('failing', { getError: failing }), + makeManager('slow', { envs: [slowEnv], delayMs: 300_000 }), + ]; + + const resultPromise = api.getEnvironments('all'); + await clock.tickAsync(300_000); + const result = await resultPromise; + + assert.deepStrictEqual(result, [slowEnv], 'the slow manager still contributes once its operation settles'); + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[getEnvironments(all)] Environment manager "failing" failed and was skipped.', + failing, + ); + }); + + test('a manager rejection is logged as soon as it settles even while another manager never settles', async () => { + const clock = sinon.useFakeTimers(); + const failing = new Error('boom'); + const neverSettles = { + id: 'pending', + displayName: 'pending', + getEnvironments: () => new Promise(() => {}), + refresh: async () => {}, + } as unknown as InternalEnvironmentManager; + currentManagers = [makeManager('failing', { getError: failing }), neverSettles]; + + let settled = false; + const resultPromise = api.getEnvironments('all'); + void resultPromise.then( + () => { + settled = true; + }, + () => { + settled = true; + }, + ); + await clock.tickAsync(0); + + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[getEnvironments(all)] Environment manager "failing" failed and was skipped.', + failing, + ); + assert.strictEqual( + settled, + false, + 'the aggregate stays pending while one manager never settles, yet the failure is already logged', + ); + }); + + test('empty manager list resolves with an empty array', async () => { + currentManagers = []; + const result = await api.getEnvironments('all'); + assert.deepStrictEqual(result, []); + assert.ok(traceErrorStub.notCalled, 'no failures should be logged for an empty manager list'); + }); + }); + + suite('getEnvironments(global)', () => { + test('partial success: one manager failing does not hide the others, and the scope is forwarded', async () => { + const globalEnv = makeEnv('global-one'); + const scopeAware = { + id: 'm1', + displayName: 'm1', + getEnvironments: async (scope: unknown) => (scope === 'global' ? [globalEnv] : []), + refresh: async () => {}, + } as unknown as InternalEnvironmentManager; + const m2Error = new Error('m2 boom'); + currentManagers = [scopeAware, makeManager('m2', { getError: m2Error })]; + + const result = await api.getEnvironments('global'); + + assert.deepStrictEqual( + result.map((e) => e.envId.id), + ['global-one'], + 'global scope should return successful managers only and forward the scope', + ); + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[getEnvironments(global)] Environment manager "m2" failed and was skipped.', + m2Error, + ); + }); + + test('partial success where the only surviving manager has no environments resolves with [] (logged, not thrown)', async () => { + const err = new Error('global-owner boom'); + currentManagers = [makeManager('m1', { getError: err }), makeManager('m2', { envs: [] })]; + + const result = await api.getEnvironments('global'); + + assert.deepStrictEqual( + result, + [], + 'a surviving manager with no environments yields an empty (not thrown) result', + ); + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[getEnvironments(global)] Environment manager "m1" failed and was skipped.', + err, + ); + }); + + test('total failure: throws AggregateEnvironmentError with all reasons', async () => { + const err1 = new Error('global-first'); + const err2 = new Error('global-second'); + currentManagers = [makeManager('m1', { getError: err1 }), makeManager('m2', { getError: err2 })]; + + await assert.rejects( + () => api.getEnvironments('global'), + (err: unknown) => { + assert.ok(err instanceof AggregateEnvironmentError, 'should throw AggregateEnvironmentError'); + assert.deepStrictEqual(err.errors, [err1, err2], 'should carry all reasons in manager order'); + return true; + }, + ); + assert.strictEqual(traceErrorStub.callCount, 2, 'both failures should be logged exactly once'); + sinon.assert.calledWithExactly( + traceErrorStub, + '[getEnvironments(global)] Environment manager "m1" failed and was skipped.', + err1, + ); + sinon.assert.calledWithExactly( + traceErrorStub, + '[getEnvironments(global)] Environment manager "m2" failed and was skipped.', + err2, + ); + }); + }); + + suite('refreshEnvironments(undefined)', () => { + test('partial success: completes even though one manager fails', async () => { + let m3Refreshed = false; + const m2Error = new Error('refresh boom'); + currentManagers = [ + makeManager('m1', {}), + makeManager('m2', { refreshError: m2Error }), + { + id: 'm3', + displayName: 'm3', + getEnvironments: async () => [], + refresh: async () => { + m3Refreshed = true; + }, + } as unknown as InternalEnvironmentManager, + ]; + + await api.refreshEnvironments(undefined); + + assert.ok(m3Refreshed, 'later managers still refresh despite an earlier failure'); + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[refreshEnvironments(all)] Environment manager "m2" failed and was skipped.', + m2Error, + ); + }); + + test('total failure: throws AggregateEnvironmentError with all reasons', async () => { + const err1 = new Error('r1'); + const err2 = new Error('r2'); + currentManagers = [ + makeManager('m1', { refreshError: err1 }), + makeManager('m2', { refreshError: err2 }), + ]; + + await assert.rejects( + () => api.refreshEnvironments(undefined), + (err: unknown) => { + assert.ok(err instanceof AggregateEnvironmentError); + assert.deepStrictEqual(err.errors, [err1, err2]); + return true; + }, + ); + }); + + test('a slow refresh is awaited to completion so its state mutation is never detached by a timeout', async () => { + const clock = sinon.useFakeTimers(); + const failing = new Error('boom'); + let slowRefreshed = false; + const slow = { + id: 'slow', + displayName: 'slow', + getEnvironments: async () => [], + refresh: async () => { + await delay(300_000); + slowRefreshed = true; + }, + } as unknown as InternalEnvironmentManager; + currentManagers = [makeManager('failing', { refreshError: failing }), slow]; + + const resultPromise = api.refreshEnvironments(undefined); + await clock.tickAsync(300_000); + await resultPromise; + + assert.ok(slowRefreshed, 'the slow refresh runs to completion instead of being timed out and detached'); + sinon.assert.calledOnceWithExactly( + traceErrorStub, + '[refreshEnvironments(all)] Environment manager "failing" failed and was skipped.', + failing, + ); + }); + + test('empty manager list completes without throwing', async () => { + currentManagers = []; + await api.refreshEnvironments(undefined); + assert.ok(traceErrorStub.notCalled); + }); + }); +}); + +function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +function makeEnv(id: string): PythonEnvironment { + return { + envId: { id, managerId: 'test-manager' }, + name: id, + displayName: id, + displayPath: `/envs/${id}`, + } as unknown as PythonEnvironment; +} + +interface FakeManagerOptions { + envs?: PythonEnvironment[]; + getError?: unknown; + refreshError?: unknown; + delayMs?: number; +} + +function makeManager(id: string, options: FakeManagerOptions = {}): InternalEnvironmentManager { + return { + id, + displayName: id, + getEnvironments: async () => { + if (options.delayMs) { + await delay(options.delayMs); + } + if (options.getError !== undefined) { + throw options.getError; + } + return options.envs ?? []; + }, + refresh: async () => { + if (options.delayMs) { + await delay(options.delayMs); + } + if (options.refreshError !== undefined) { + throw options.refreshError; + } + }, + } as unknown as InternalEnvironmentManager; +}