debt: finish adopting ensureNoDisposablesAreLeakedInTestSuite (microsoft#249678)

connor4312 · web-flow · commit 90885486e5c2 · 2025-05-23T17:28:32.000-07:00
Closes microsoft#200091 Notably makes `raceCancellablePromises` itself be cancellable to allow for composition, and make `Event.toPromise` cancellable too.
diff --git a/eslint.config.js b/eslint.config.js
@@ -223,20 +223,7 @@ export default tseslint.config(
 				{
 					// Files should (only) be removed from the list they adopt the leak detector
 					'exclude': [
-						'src/vs/platform/configuration/test/common/configuration.test.ts',
-						'src/vs/platform/opener/test/common/opener.test.ts',
-						'src/vs/platform/registry/test/common/platform.test.ts',
-						'src/vs/platform/workspace/test/common/workspace.test.ts',
-						'src/vs/platform/workspaces/test/electron-main/workspaces.test.ts',
-						'src/vs/workbench/contrib/bulkEdit/test/browser/bulkCellEdits.test.ts',
-						'src/vs/workbench/contrib/chat/test/common/chatWordCounter.test.ts',
-						'src/vs/workbench/contrib/extensions/test/common/extensionQuery.test.ts',
-						'src/vs/workbench/contrib/notebook/test/browser/notebookExecutionService.test.ts',
-						'src/vs/workbench/contrib/notebook/test/browser/notebookExecutionStateService.test.ts',
-						'src/vs/workbench/contrib/tasks/test/common/problemMatcher.test.ts',
-						'src/vs/workbench/services/commands/test/common/commandService.test.ts',
 						'src/vs/workbench/services/userActivity/test/browser/domActivityTracker.test.ts',
-						'src/vs/workbench/test/browser/quickAccess.test.ts'
 					]
 				}
 			]
diff --git a/src/vs/base/common/async.ts b/src/vs/base/common/async.ts
@@ -119,19 +119,21 @@ export function raceCancellationError<T>(promise: Promise<T>, token: Cancellatio
 /**
  * Returns as soon as one of the promises resolves or rejects and cancels remaining promises
  */
-export async function raceCancellablePromises<T>(cancellablePromises: CancelablePromise<T>[]): Promise<T> {
+export function raceCancellablePromises<T>(cancellablePromises: (CancelablePromise<T> | Promise<T>)[]): CancelablePromise<T> {
 	let resolvedPromiseIndex = -1;
 	const promises = cancellablePromises.map((promise, index) => promise.then(result => { resolvedPromiseIndex = index; return result; }));
-	try {
-		const result = await Promise.race(promises);
-		return result;
-	} finally {
+	const promise = Promise.race(promises) as CancelablePromise<T>;
+	promise.cancel = () => {
 		cancellablePromises.forEach((cancellablePromise, index) => {
-			if (index !== resolvedPromiseIndex) {
-				cancellablePromise.cancel();
+			if (index !== resolvedPromiseIndex && (cancellablePromise as CancelablePromise<T>).cancel) {
+				(cancellablePromise as CancelablePromise<T>).cancel();
 			}
 		});
-	}
+	};
+	promise.finally(() => {
+		promise.cancel();
+	});
+	return promise;
 }
 
 export function raceTimeout<T>(promise: Promise<T>, timeout: number, onTimeout?: () => void): Promise<T | undefined> {
diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts
@@ -3,6 +3,7 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
+import { CancelablePromise } from './async.js';
 import { CancellationToken } from './cancellation.js';
 import { diffSets } from './collections.js';
 import { onUnexpectedError } from './errors.js';
@@ -598,8 +599,16 @@ export namespace Event {
 	/**
 	 * Creates a promise out of an event, using the {@link Event.once} helper.
 	 */
-	export function toPromise<T>(event: Event<T>, disposables?: IDisposable[] | DisposableStore): Promise<T> {
-		return new Promise(resolve => once(event)(resolve, null, disposables));
+	export function toPromise<T>(event: Event<T>, disposables?: IDisposable[] | DisposableStore): CancelablePromise<T> {
+		let cancelRef: () => void;
+		const promise = new Promise((resolve, reject) => {
+			const listener = once(event)(resolve, null, disposables);
+			// not resolved, matching the behavior of a normal disposal
+			cancelRef = () => listener.dispose();
+		}) as CancelablePromise<T>;
+		promise.cancel = cancelRef!;
+
+		return promise;
 	}
 
 	/**
diff --git a/src/vs/platform/configuration/test/common/configuration.test.ts b/src/vs/platform/configuration/test/common/configuration.test.ts
@@ -5,9 +5,12 @@
 import assert from 'assert';
 import { merge, removeFromValueTree } from '../../common/configuration.js';
 import { mergeChanges } from '../../common/configurationModels.js';
+import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js';
 
 suite('Configuration', () => {
 
+	ensureNoDisposablesAreLeakedInTestSuite();
+
 	test('simple merge', () => {
 		let base = { 'a': 1, 'b': 2 };
 		merge(base, { 'a': 3, 'c': 4 }, true);
@@ -121,6 +124,8 @@ suite('Configuration', () => {
 
 suite('Configuration Changes: Merge', () => {
 
+	ensureNoDisposablesAreLeakedInTestSuite();
+
 	test('merge only keys', () => {
 		const actual = mergeChanges({ keys: ['a', 'b'], overrides: [] }, { keys: ['c', 'd'], overrides: [] });
 		assert.deepStrictEqual(actual, { keys: ['a', 'b', 'c', 'd'], overrides: [] });
diff --git a/src/vs/platform/opener/test/common/opener.test.ts b/src/vs/platform/opener/test/common/opener.test.ts
@@ -6,9 +6,12 @@
 import assert from 'assert';
 import { URI } from '../../../../base/common/uri.js';
 import { extractSelection, withSelection } from '../../common/opener.js';
+import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js';
 
 suite('extractSelection', () => {
 
+	ensureNoDisposablesAreLeakedInTestSuite();
+
 	test('extractSelection with only startLineNumber', async () => {
 		const uri = URI.parse('file:///some/file.js#73');
 		assert.deepStrictEqual(extractSelection(uri).selection, { startLineNumber: 73, startColumn: 1, endLineNumber: undefined, endColumn: undefined });
diff --git a/src/vs/platform/quickinput/browser/quickAccess.ts b/src/vs/platform/quickinput/browser/quickAccess.ts
@@ -6,7 +6,7 @@
 import { DeferredPromise } from '../../../base/common/async.js';
 import { CancellationTokenSource } from '../../../base/common/cancellation.js';
 import { Event } from '../../../base/common/event.js';
-import { Disposable, DisposableStore, IDisposable, toDisposable } from '../../../base/common/lifecycle.js';
+import { Disposable, DisposableStore, IDisposable, isDisposable, toDisposable } from '../../../base/common/lifecycle.js';
 import { IInstantiationService } from '../../instantiation/common/instantiation.js';
 import { DefaultQuickAccessFilterValue, Extensions, IQuickAccessController, IQuickAccessOptions, IQuickAccessProvider, IQuickAccessProviderDescriptor, IQuickAccessRegistry } from '../common/quickAccess.js';
 import { IQuickInputService, IQuickPick, IQuickPickItem, ItemActivation } from '../common/quickInput.js';
@@ -30,6 +30,15 @@ export class QuickAccessController extends Disposable implements IQuickAccessCon
 		@IInstantiationService private readonly instantiationService: IInstantiationService
 	) {
 		super();
+		this._register(toDisposable(() => {
+			for (const provider of this.mapProviderToDescriptor.values()) {
+				if (isDisposable(provider)) {
+					provider.dispose();
+				}
+			}
+
+			this.visibleQuickAccess?.picker.dispose();
+		}));
 	}
 
 	pick(value = '', options?: IQuickAccessOptions): Promise<IQuickPickItem[] | undefined> {
diff --git a/src/vs/platform/registry/test/common/platform.test.ts b/src/vs/platform/registry/test/common/platform.test.ts
@@ -6,9 +6,12 @@
 import assert from 'assert';
 import { isFunction } from '../../../../base/common/types.js';
 import { Registry } from '../../common/platform.js';
+import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js';
 
 suite('Platform / Registry', () => {
 
+	ensureNoDisposablesAreLeakedInTestSuite();
+
 	test('registry - api', function () {
 		assert.ok(isFunction(Registry.add));
 		assert.ok(isFunction(Registry.as));
diff --git a/src/vs/platform/workspace/test/common/workspace.test.ts b/src/vs/platform/workspace/test/common/workspace.test.ts
@@ -10,9 +10,12 @@ import { extUriBiasedIgnorePathCase } from '../../../../base/common/resources.js
 import { URI } from '../../../../base/common/uri.js';
 import { IRawFileWorkspaceFolder, Workspace, WorkspaceFolder } from '../../common/workspace.js';
 import { toWorkspaceFolders } from '../../../workspaces/common/workspaces.js';
+import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js';
 
 suite('Workspace', () => {
 
+	ensureNoDisposablesAreLeakedInTestSuite();
+
 	const fileFolder = isWindows ? 'c:\\src' : '/src';
 	const abcFolder = isWindows ? 'c:\\abc' : '/abc';
 
diff --git a/src/vs/workbench/contrib/extensions/test/common/extensionQuery.test.ts b/src/vs/workbench/contrib/extensions/test/common/extensionQuery.test.ts
@@ -5,8 +5,11 @@
 
 import assert from 'assert';
 import { Query } from '../../common/extensionQuery.js';
+import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js';
 
 suite('Extension query', () => {
+	ensureNoDisposablesAreLeakedInTestSuite();
+
 	test('parse', () => {
 		let query = Query.parse('');
 		assert.strictEqual(query.value, '');
diff --git a/src/vs/workbench/contrib/tasks/test/common/problemMatcher.test.ts b/src/vs/workbench/contrib/tasks/test/common/problemMatcher.test.ts
@@ -6,6 +6,7 @@ import * as matchers from '../../common/problemMatcher.js';
 
 import assert from 'assert';
 import { ValidationState, IProblemReporter, ValidationStatus } from '../../../../../base/common/parsers.js';
+import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js';
 
 class ProblemReporter implements IProblemReporter {
 	private _validationStatus: ValidationStatus;
@@ -60,6 +61,8 @@ suite('ProblemPatternParser', () => {
 	let parser: matchers.ProblemPatternParser;
 	const testRegexp = new RegExp('test');
 
+	ensureNoDisposablesAreLeakedInTestSuite();
+
 	setup(() => {
 		reporter = new ProblemReporter();
 		parser = new matchers.ProblemPatternParser(reporter);
diff --git a/src/vs/workbench/services/commands/common/commandService.ts b/src/vs/workbench/services/commands/common/commandService.ts
@@ -3,21 +3,21 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js';
-import { ICommandService, ICommandEvent, CommandsRegistry } from '../../../../platform/commands/common/commands.js';
-import { IExtensionService } from '../../extensions/common/extensions.js';
-import { Event, Emitter } from '../../../../base/common/event.js';
+import { CancelablePromise, raceCancellablePromises, timeout } from '../../../../base/common/async.js';
+import { Emitter, Event } from '../../../../base/common/event.js';
 import { Disposable } from '../../../../base/common/lifecycle.js';
-import { ILogService } from '../../../../platform/log/common/log.js';
+import { CommandsRegistry, ICommandEvent, ICommandService } from '../../../../platform/commands/common/commands.js';
 import { InstantiationType, registerSingleton } from '../../../../platform/instantiation/common/extensions.js';
-import { timeout } from '../../../../base/common/async.js';
+import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js';
+import { ILogService } from '../../../../platform/log/common/log.js';
+import { IExtensionService } from '../../extensions/common/extensions.js';
 
 export class CommandService extends Disposable implements ICommandService {
 
 	declare readonly _serviceBrand: undefined;
 
 	private _extensionHostIsReady: boolean = false;
-	private _starActivation: Promise<void> | null;
+	private _starActivation: CancelablePromise<void> | null;
 
 	private readonly _onWillExecuteCommand: Emitter<ICommandEvent> = this._register(new Emitter<ICommandEvent>());
 	public readonly onWillExecuteCommand: Event<ICommandEvent> = this._onWillExecuteCommand.event;
@@ -35,10 +35,10 @@ export class CommandService extends Disposable implements ICommandService {
 		this._starActivation = null;
 	}
 
-	private _activateStar(): Promise<void> {
+	private _activateStar(): CancelablePromise<void> {
 		if (!this._starActivation) {
 			// wait for * activation, limited to at most 30s
-			this._starActivation = Promise.race<any>([
+			this._starActivation = raceCancellablePromises([
 				this._extensionService.activateByEvent(`*`),
 				timeout(30000)
 			]);
@@ -76,12 +76,13 @@ export class CommandService extends Disposable implements ICommandService {
 		// as well as a * activation event raced against registration and against 30s
 		await Promise.all([
 			this._extensionService.activateByEvent(activationEvent),
-			Promise.race<any>([
+			raceCancellablePromises<unknown>([
 				// race * activation against command registration
 				this._activateStar(),
 				Event.toPromise(Event.filter(CommandsRegistry.onDidRegisterCommand, e => e === id))
 			]),
 		]);
+
 		return this._tryExecuteCommand(id, args);
 	}
 
diff --git a/src/vs/workbench/services/commands/test/common/commandService.test.ts b/src/vs/workbench/services/commands/test/common/commandService.test.ts
@@ -3,35 +3,32 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 import assert from 'assert';
-import { IDisposable, DisposableStore } from '../../../../../base/common/lifecycle.js';
+import { DisposableStore } from '../../../../../base/common/lifecycle.js';
+import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js';
 import { CommandsRegistry } from '../../../../../platform/commands/common/commands.js';
-import { CommandService } from '../../common/commandService.js';
-import { NullExtensionService } from '../../../extensions/common/extensions.js';
 import { InstantiationService } from '../../../../../platform/instantiation/common/instantiationService.js';
 import { NullLogService } from '../../../../../platform/log/common/log.js';
+import { NullExtensionService } from '../../../extensions/common/extensions.js';
+import { CommandService } from '../../common/commandService.js';
 
 suite('CommandService', function () {
 
-	let commandRegistration: IDisposable;
+	const store = ensureNoDisposablesAreLeakedInTestSuite();
 
 	setup(function () {
-		commandRegistration = CommandsRegistry.registerCommand('foo', function () { });
-	});
-
-	teardown(function () {
-		commandRegistration.dispose();
+		store.add(CommandsRegistry.registerCommand('foo', function () { }));
 	});
 
 	test('activateOnCommand', () => {
 
 		let lastEvent: string;
 
-		const service = new CommandService(new InstantiationService(), new class extends NullExtensionService {
+		const service = store.add(new CommandService(new InstantiationService(), new class extends NullExtensionService {
 			override activateByEvent(activationEvent: string): Promise<void> {
 				lastEvent = activationEvent;
 				return super.activateByEvent(activationEvent);
 			}
-		}, new NullLogService());
+		}, new NullLogService()));
 
 		return service.executeCommand('foo').then(() => {
 			assert.ok(lastEvent, 'onCommand:foo');
@@ -51,7 +48,7 @@ suite('CommandService', function () {
 			}
 		};
 
-		const service = new CommandService(new InstantiationService(), extensionService, new NullLogService());
+		const service = store.add(new CommandService(new InstantiationService(), extensionService, new NullLogService()));
 
 		await extensionService.whenInstalledExtensionsRegistered();
 
@@ -65,11 +62,11 @@ suite('CommandService', function () {
 		let callCounter = 0;
 		const reg = CommandsRegistry.registerCommand('bar', () => callCounter += 1);
 
-		const service = new CommandService(new InstantiationService(), new class extends NullExtensionService {
+		const service = store.add(new CommandService(new InstantiationService(), new class extends NullExtensionService {
 			override whenInstalledExtensionsRegistered() {
 				return new Promise<boolean>(_resolve => { /*ignore*/ });
 			}
-		}, new NullLogService());
+		}, new NullLogService()));
 
 		service.executeCommand('bar');
 		assert.strictEqual(callCounter, 1);
@@ -82,11 +79,11 @@ suite('CommandService', function () {
 		let resolveFunc: Function;
 		const whenInstalledExtensionsRegistered = new Promise<boolean>(_resolve => { resolveFunc = _resolve; });
 
-		const service = new CommandService(new InstantiationService(), new class extends NullExtensionService {
+		const service = store.add(new CommandService(new InstantiationService(), new class extends NullExtensionService {
 			override whenInstalledExtensionsRegistered() {
 				return whenInstalledExtensionsRegistered;
 			}
-		}, new NullLogService());
+		}, new NullLogService()));
 
 		const r = service.executeCommand('bar');
 		assert.strictEqual(callCounter, 0);
@@ -105,7 +102,7 @@ suite('CommandService', function () {
 		let callCounter = 0;
 		const disposable = new DisposableStore();
 		const events: string[] = [];
-		const service = new CommandService(new InstantiationService(), new class extends NullExtensionService {
+		const service = store.add(new CommandService(new InstantiationService(), new class extends NullExtensionService {
 
 			override activateByEvent(event: string): Promise<void> {
 				events.push(event);
@@ -126,7 +123,7 @@ suite('CommandService', function () {
 				return Promise.resolve();
 			}
 
-		}, new NullLogService());
+		}, new NullLogService()));
 
 		return service.executeCommand('farboo').then(() => {
 			assert.strictEqual(callCounter, 1);
@@ -140,7 +137,7 @@ suite('CommandService', function () {
 		const expectedOrder: string[] = ['registering command', 'resolving activation event', 'executing command'];
 		const actualOrder: string[] = [];
 		const disposables = new DisposableStore();
-		const service = new CommandService(new InstantiationService(), new class extends NullExtensionService {
+		const service = store.add(new CommandService(new InstantiationService(), new class extends NullExtensionService {
 
 			override activateByEvent(event: string): Promise<void> {
 				if (event === '*') {
@@ -167,7 +164,7 @@ suite('CommandService', function () {
 				return Promise.resolve();
 			}
 
-		}, new NullLogService());
+		}, new NullLogService()));
 
 		return service.executeCommand('farboo2').then(() => {
 			assert.deepStrictEqual(actualOrder, expectedOrder);
@@ -188,7 +185,7 @@ suite('CommandService', function () {
 				return true;
 			}
 		};
-		const service = new CommandService(new InstantiationService(), extensionService, new NullLogService());
+		const service = store.add(new CommandService(new InstantiationService(), extensionService, new NullLogService()));
 
 		await extensionService.whenInstalledExtensionsRegistered();
 
diff --git a/src/vs/workbench/services/history/test/browser/historyService.test.ts b/src/vs/workbench/services/history/test/browser/historyService.test.ts
@@ -838,7 +838,7 @@ suite('HistoryService', function () {
 		const input4 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar4'), TEST_EDITOR_INPUT_ID));
 		const input5 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar5'), TEST_EDITOR_INPUT_ID));
 
-		let editorChangePromise = Event.toPromise(editorService.onDidActiveEditorChange);
+		let editorChangePromise: Promise<void> = Event.toPromise(editorService.onDidActiveEditorChange);
 		await part.activeGroup.openEditor(input1, { pinned: true });
 		assert.strictEqual(part.activeGroup.activeEditor, input1);
 		await editorChangePromise;
diff --git a/src/vs/workbench/services/userActivity/test/browser/domActivityTracker.test.ts b/src/vs/workbench/services/userActivity/test/browser/domActivityTracker.test.ts
diff --git a/src/vs/workbench/test/browser/quickAccess.test.ts b/src/vs/workbench/test/browser/quickAccess.test.ts

Original file line number	Diff line number	Diff line change
`@@ -223,20 +223,7 @@ export default tseslint.config(`
`223`	`223`	`{`
`224`	`224`	`// Files should (only) be removed from the list they adopt the leak detector`
`225`	`225`	`'exclude': [`
`226`		`- 'src/vs/platform/configuration/test/common/configuration.test.ts',`
`227`		`- 'src/vs/platform/opener/test/common/opener.test.ts',`
`228`		`- 'src/vs/platform/registry/test/common/platform.test.ts',`
`229`		`- 'src/vs/platform/workspace/test/common/workspace.test.ts',`
`230`		`- 'src/vs/platform/workspaces/test/electron-main/workspaces.test.ts',`
`231`		`- 'src/vs/workbench/contrib/bulkEdit/test/browser/bulkCellEdits.test.ts',`
`232`		`- 'src/vs/workbench/contrib/chat/test/common/chatWordCounter.test.ts',`
`233`		`- 'src/vs/workbench/contrib/extensions/test/common/extensionQuery.test.ts',`
`234`		`- 'src/vs/workbench/contrib/notebook/test/browser/notebookExecutionService.test.ts',`
`235`		`- 'src/vs/workbench/contrib/notebook/test/browser/notebookExecutionStateService.test.ts',`
`236`		`- 'src/vs/workbench/contrib/tasks/test/common/problemMatcher.test.ts',`
`237`		`- 'src/vs/workbench/services/commands/test/common/commandService.test.ts',`
`238`	`226`	`'src/vs/workbench/services/userActivity/test/browser/domActivityTracker.test.ts',`
`239`		`- 'src/vs/workbench/test/browser/quickAccess.test.ts'`
`240`	`227`	`]`
`241`	`228`	`}`
`242`	`229`	`]`