From c497bf83fb616ae38c5e824172621513f0bf2d42 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, logs each failure with its manager id, and throws AggregateEnvironmentError only when every manager fails (an empty manager list resolves with []). - Open the picker immediately with Browse/Create and load managers concurrently after it is shown, through a small optional onDidShow controller seam on showQuickPickWithButtons that reuses the existing accept/back/cancel/button wiring. A failed manager is logged and skipped without blocking the others, and late updates no-op once the picker is closed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../errors/AggregateEnvironmentError.ts | 11 + src/common/pickers/environments.ts | 72 ++-- src/common/window.apis.ts | 33 +- src/features/pythonApi.ts | 51 ++- src/test/common/fakeQuickPick.ts | 100 ++++++ src/test/common/pickEnvironment.unit.test.ts | 176 ++++++++++ .../showQuickPickWithButtons.unit.test.ts | 148 ++++++++ .../pythonApi.failureIsolation.unit.test.ts | 331 ++++++++++++++++++ 8 files changed, 894 insertions(+), 28 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/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..bc16461ac 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, @@ -120,16 +121,20 @@ async function createEnvironment( } } +type EnvironmentPickItem = QuickPickItem | (QuickPickItem & { result: PythonEnvironment }); + 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 +157,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 +193,49 @@ export async function pickEnvironment( ); } - for (const manager of managers) { - items.push({ - label: manager.displayName, - kind: QuickPickItemKind.Separator, + // Load every manager's environments concurrently after the picker is shown so opening never waits + // on the slowest manager, and a single manager that rejects can't hide the others' environments. + const onDidShow = (controller: QuickPickController) => { + controller.setBusy(true); + void Promise.allSettled(managers.map((manager) => manager.getEnvironments('all'))).then((results) => { + const withEnvironments: EnvironmentPickItem[] = [...items]; + results.forEach((outcome, index) => { + const manager = managers[index]; + if (outcome.status === 'rejected') { + traceError( + `[pickEnvironment] Failed to load environments for manager "${manager.id}"; section skipped.`, + outcome.reason, + ); + return; + } + withEnvironments.push({ + label: manager.displayName, + kind: QuickPickItemKind.Separator, + }); + withEnvironments.push( + ...outcome.value.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), + }; + }), + ); + }); + controller.setItems(withEnvironments); + 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 { diff --git a/src/common/window.apis.ts b/src/common/window.apis.ts index 89326a823..4e8b428cf 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,27 @@ 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; + } + quickPick.items = newItems; + }, + }; + 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..9ad8c0e46 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,45 @@ 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 []; + } + + // Wrap each call so a manager that throws synchronously is isolated as a rejected outcome rather + // than escaping Promise.allSettled and hiding the other managers' results. + const settled = await Promise.allSettled(managers.map(async (manager) => operation(manager))); + + const results: T[] = []; + const errors: unknown[] = []; + settled.forEach((outcome, index) => { + if (outcome.status === 'fulfilled') { + results.push(outcome.value); + } else { + errors.push(outcome.reason); + traceError( + `[${context}] Environment manager "${managers[index].id}" failed and was skipped.`, + 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 +249,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 +266,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..437922b84 --- /dev/null +++ b/src/test/common/pickEnvironment.unit.test.ts @@ -0,0 +1,176 @@ +// 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'; + +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), + }; +} + +suite('pickEnvironment - opens promptly and isolates manager failures', () => { + let fake: FakeQuickPick; + let traceErrorStub: sinon.SinonStub; + + const STATIC_LABELS = [Interpreter.browsePath, '', Interpreter.createVirtualEnvironment]; + const labels = (): string[] => fake.items.map((i) => i.label); + + 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: [] }); + + m1.reject(new Error('boom')); + m2.resolve([makeEnv('e2', '/p/e2')]); + await flush(); + + assert.deepStrictEqual(labels(), [...STATIC_LABELS, 'Two', 'e2'], 'the surviving manager still shows'); + assert.ok( + traceErrorStub.getCalls().some((c) => String(c.args[0]).includes('"m1"')), + 'the failed manager id should be logged', + ); + + 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: [] }); + + m1.reject(new Error('boom-1')); + m2.reject(new Error('boom-2')); + 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.getCalls().filter((c) => String(c.args[0]).includes('Failed to load')).length, + 2, + 'each failed manager is logged', + ); + + 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'); + }); +}); diff --git a/src/test/common/showQuickPickWithButtons.unit.test.ts b/src/test/common/showQuickPickWithButtons.unit.test.ts new file mode 100644 index 000000000..1ffb78d1a --- /dev/null +++ b/src/test/common/showQuickPickWithButtons.unit.test.ts @@ -0,0 +1,148 @@ +// 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.ok(Array.isArray(e.item)); + 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.strictEqual(fake.items.length, 3, '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.map((i) => i.label), + ['a'], + '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..192366aa6 --- /dev/null +++ b/src/test/features/pythonApi.failureIsolation.unit.test.ts @@ -0,0 +1,331 @@ +// 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'; + +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; +} + +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(); + }); + + function errorLoggedFor(managerId: string): boolean { + return traceErrorStub + .getCalls() + .some((c) => typeof c.args[0] === 'string' && (c.args[0] as string).includes(managerId)); + } + + suite('getEnvironments(all)', () => { + test('partial success: one manager failing does not hide the others', async () => { + const e1 = makeEnv('one'); + const e3 = makeEnv('three'); + currentManagers = [ + makeManager('m1', { envs: [e1] }), + makeManager('m2', { getError: new Error('m2 boom') }), + 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', + ); + assert.ok(errorLoggedFor('m2'), 'failing manager id should be logged'); + assert.ok(!errorLoggedFor('m1'), 'successful managers should not be logged as failures'); + }); + + test('a manager throwing synchronously is isolated like an async rejection', async () => { + const e1 = makeEnv('one'); + const e3 = makeEnv('three'); + const syncThrower = { + id: 'm2', + displayName: 'm2', + getEnvironments: () => { + throw new Error('sync boom'); + }, + 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', + ); + assert.ok(errorLoggedFor('m2'), 'the synchronously failing manager should be logged'); + }); + + 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.ok(errorLoggedFor('m1') && errorLoggedFor('m2'), 'both failures should be 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; + currentManagers = [scopeAware, makeManager('m2', { getError: new Error('m2 boom') })]; + + 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', + ); + assert.ok(errorLoggedFor('m2'), 'failing manager id should be logged for the global scope'); + }); + + 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', + ); + assert.ok(errorLoggedFor('m1'), 'the failing manager id should still be logged'); + }); + + 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.ok(errorLoggedFor('m1') && errorLoggedFor('m2'), 'both failures should be logged'); + }); + }); + + suite('refreshEnvironments(undefined)', () => { + test('partial success: completes even though one manager fails', async () => { + let m3Refreshed = false; + currentManagers = [ + makeManager('m1', {}), + makeManager('m2', { refreshError: new Error('refresh boom') }), + { + 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'); + assert.ok(errorLoggedFor('m2'), 'failing manager id should be logged'); + }); + + 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('empty manager list completes without throwing', async () => { + currentManagers = []; + await api.refreshEnvironments(undefined); + assert.ok(traceErrorStub.notCalled); + }); + }); +});