debt: finish adopting ensureNoDisposablesAreLeakedInTestSuite (#249678)

Closes #200091

Notably makes `raceCancellablePromises` itself be cancellable to allow
for composition, and make `Event.toPromise` cancellable too.
This commit is contained in:
Connor Peet 2025-05-23 17:28:32 -07:00 committed by GitHub
parent 446a4ab5f8
commit 90885486e5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
15 changed files with 107 additions and 80 deletions

View File

@ -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'
]
}
]

View File

@ -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> {

View File

@ -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;
}
/**

View File

@ -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: [] });

View File

@ -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 });

View File

@ -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> {

View File

@ -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));

View File

@ -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';

View File

@ -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, '');

View File

@ -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);

View File

@ -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);
}

View File

@ -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();

View File

@ -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;

View File

@ -3,31 +3,29 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/
import assert from 'assert';
import * as sinon from 'sinon';
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js';
import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js';
import { DomActivityTracker } from '../../browser/domActivityTracker.js';
import { UserActivityService } from '../../common/userActivityService.js';
import * as sinon from 'sinon';
import assert from 'assert';
suite('DomActivityTracker', () => {
let uas: UserActivityService;
let dom: DomActivityTracker;
let insta: TestInstantiationService;
let clock: sinon.SinonFakeTimers;
const maxTimeToBecomeIdle = 3 * 30_000; // (MIN_INTERVALS_WITHOUT_ACTIVITY + 1) * CHECK_INTERVAL;
const store = ensureNoDisposablesAreLeakedInTestSuite();
setup(() => {
clock = sinon.useFakeTimers();
insta = new TestInstantiationService();
uas = new UserActivityService(insta);
dom = new DomActivityTracker(uas);
insta = store.add(new TestInstantiationService());
uas = store.add(new UserActivityService(insta));
store.add(new DomActivityTracker(uas));
});
teardown(() => {
dom.dispose();
uas.dispose();
clock.restore();
insta.dispose();
});

View File

@ -20,10 +20,11 @@ import { PickerEditorState } from '../../browser/quickaccess.js';
import { EditorsOrder } from '../../common/editor.js';
import { Range } from '../../../editor/common/core/range.js';
import { TestInstantiationService } from '../../../platform/instantiation/test/common/instantiationServiceMock.js';
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../base/test/common/utils.js';
suite('QuickAccess', () => {
let disposables: DisposableStore;
const disposables = ensureNoDisposablesAreLeakedInTestSuite();
let instantiationService: TestInstantiationService;
let accessor: TestServiceAccessor;
@ -50,12 +51,14 @@ suite('QuickAccess', () => {
provide(picker: IQuickPick<IQuickPickItem, { useSeparators: true }>, token: CancellationToken): IDisposable {
assert.ok(picker);
providerDefaultCalled = true;
token.onCancellationRequested(() => providerDefaultCanceled = true);
const store = new DisposableStore();
store.add(toDisposable(() => providerDefaultDisposed = true));
store.add(token.onCancellationRequested(() => providerDefaultCanceled = true));
// bring up provider #3
setTimeout(() => this.quickInputService.quickAccess.show(providerDescriptor3.prefix));
return toDisposable(() => providerDefaultDisposed = true);
return store;
}
}
@ -63,9 +66,11 @@ suite('QuickAccess', () => {
provide(picker: IQuickPick<IQuickPickItem, { useSeparators: true }>, token: CancellationToken): IDisposable {
assert.ok(picker);
provider1Called = true;
token.onCancellationRequested(() => provider1Canceled = true);
const store = new DisposableStore();
store.add(token.onCancellationRequested(() => provider1Canceled = true));
return toDisposable(() => provider1Disposed = true);
store.add(toDisposable(() => provider1Disposed = true));
return store;
}
}
@ -73,9 +78,11 @@ suite('QuickAccess', () => {
provide(picker: IQuickPick<IQuickPickItem, { useSeparators: true }>, token: CancellationToken): IDisposable {
assert.ok(picker);
provider2Called = true;
token.onCancellationRequested(() => provider2Canceled = true);
const store = new DisposableStore();
store.add(token.onCancellationRequested(() => provider2Canceled = true));
return toDisposable(() => provider2Disposed = true);
store.add(toDisposable(() => provider2Disposed = true));
return store;
}
}
@ -83,12 +90,14 @@ suite('QuickAccess', () => {
provide(picker: IQuickPick<IQuickPickItem, { useSeparators: true }>, token: CancellationToken): IDisposable {
assert.ok(picker);
provider3Called = true;
token.onCancellationRequested(() => provider3Canceled = true);
const store = new DisposableStore();
store.add(token.onCancellationRequested(() => provider3Canceled = true));
// hide without picking
setTimeout(() => picker.hide());
return toDisposable(() => provider3Disposed = true);
store.add(toDisposable(() => provider3Disposed = true));
return store;
}
}
@ -98,15 +107,10 @@ suite('QuickAccess', () => {
const providerDescriptor3 = { ctor: TestProvider3, prefix: 'changed', helpEntries: [] };
setup(() => {
disposables = new DisposableStore();
instantiationService = workbenchInstantiationService(undefined, disposables);
accessor = instantiationService.createInstance(TestServiceAccessor);
});
teardown(() => {
disposables.dispose();
});
test('registry', () => {
const registry = (Registry.as<IQuickAccessRegistry>(Extensions.Quickaccess));
const restore = (registry as QuickAccessRegistry).clear();
@ -344,7 +348,7 @@ suite('QuickAccess', () => {
test('PickerEditorState can properly restore editors', async () => {
const part = await createEditorPart(instantiationService, disposables);
const part = await createEditorPart(instantiationService, disposables.add(new DisposableStore()));
instantiationService.stub(IEditorGroupsService, part);
const editorService = disposables.add(instantiationService.createInstance(EditorService, undefined));