testing: smarter change event

src/vs/vscode.proposed.d.ts

@@ -2200,7 +2200,7 @@ declare module 'vscode' {
 		 * null if a top-level test was added or removed. When fired, the consumer
 		 * should check the test item and all its children for changes.
 		 */
-		readonly onDidChangeTest: Event<TestItem | null>;
+		readonly onDidChangeTest: Event<TestChangeEvent>;
 
 		/**
 		 * An event the fires when all test providers have signalled that the tests
@@ -2219,6 +2219,29 @@ declare module 'vscode' {
 		dispose(): void;
 	}
 
+	export interface TestChangeEvent {
+		/**
+		 * List of all tests that are newly added.
+		 */
+		readonly added: ReadonlyArray<TestItem>;
+
+		/**
+		 * List of existing tests that have updated.
+		 */
+		readonly updated: ReadonlyArray<TestItem>;
+
+		/**
+		 * List of existing tests that have been removed.
+		 */
+		readonly removed: ReadonlyArray<TestItem>;
+
+		/**
+		 * Highest node in the test tree under which changes were made. This can
+		 * be easily plugged into events like the TreeDataProvider update event.
+		 */
+		readonly commonChangeAncestor: TestItem | null;
+	}
+
 	/**
 	 * Tree of tests returned from the provide methods in the {@link TestProvider}.
 	 */

src/vs/workbench/api/common/extHostTesting.ts

@@ -19,7 +19,7 @@ import { IExtHostRpcService } from 'vs/workbench/api/common/extHostRpcService';
 import { TestItem } from 'vs/workbench/api/common/extHostTypeConverters';
 import { Disposable } from 'vs/workbench/api/common/extHostTypes';
 import { IExtHostWorkspace } from 'vs/workbench/api/common/extHostWorkspace';
-import { AbstractIncrementalTestCollection, EMPTY_TEST_RESULT, IncrementalTestCollectionItem, InternalTestItem, RunTestForProviderRequest, RunTestsResult, TestDiffOpType, TestsDiff } from 'vs/workbench/contrib/testing/common/testCollection';
+import { AbstractIncrementalTestCollection, EMPTY_TEST_RESULT, IncrementalChangeCollector, IncrementalTestCollectionItem, InternalTestItem, RunTestForProviderRequest, RunTestsResult, TestDiffOpType, TestsDiff } from 'vs/workbench/contrib/testing/common/testCollection';
 import type * as vscode from 'vscode';
 
 const getTestSubscriptionKey = (resource: ExtHostTestingResource, uri: URI) => `${resource}:${uri.toString()}`;
@@ -378,27 +378,160 @@ export class SingleUseTestCollection implements IDisposable {
  */
 interface MirroredCollectionTestItem extends IncrementalTestCollectionItem {
 	revived: vscode.TestItem;
+	depth: number;
 	wrapped?: vscode.TestItem;
 }
 
+class MirroredChangeCollector extends IncrementalChangeCollector<MirroredCollectionTestItem> {
+	private readonly added = new Set<MirroredCollectionTestItem>();
+	private readonly updated = new Set<MirroredCollectionTestItem>();
+	private readonly removed = new Set<MirroredCollectionTestItem>();
+
+	private readonly alreadyRemoved = new Set<string>();
+
+	public get isEmpty() {
+		return this.added.size === 0 && this.removed.size === 0 && this.updated.size === 0;
+	}
+
+	constructor(private readonly collection: MirroredTestCollection, private readonly emitter: Emitter<vscode.TestChangeEvent>) {
+		super();
+	}
+
+	/**
+	 * @override
+	 */
+	public add(node: MirroredCollectionTestItem): void {
+		this.added.add(node);
+	}
+
+	/**
+	 * @override
+	 */
+	public update(node: MirroredCollectionTestItem): void {
+		Object.assign(node.revived, TestItem.to(node.item));
+		if (!this.added.has(node)) {
+			this.updated.add(node);
+		}
+	}
+
+	/**
+	 * @override
+	 */
+	public remove(node: MirroredCollectionTestItem): void {
+		if (this.added.has(node)) {
+			this.added.delete(node);
+			return;
+		}
+
+		this.updated.delete(node);
+
+		if (node.parent && this.alreadyRemoved.has(node.parent)) {
+			this.alreadyRemoved.add(node.id);
+			return;
+		}
+
+		this.removed.add(node);
+	}
+
+	/**
+	 * @override
+	 */
+	public getChangeEvent(): vscode.TestChangeEvent {
+		const { collection, added, updated, removed } = this;
+		return {
+			get added() { return [...added].map(collection.getPublicTestItem, collection); },
+			get updated() { return [...updated].map(collection.getPublicTestItem, collection); },
+			get removed() { return [...removed].map(collection.getPublicTestItem, collection); },
+			get commonChangeAncestor() {
+				let ancestorPath: MirroredCollectionTestItem[] | undefined;
+				const buildAncestorPath = (node: MirroredCollectionTestItem | undefined) => {
+					if (!node) {
+						return undefined;
+					}
+
+					// add the node and all its parents to the list of ancestors. If
+					// the node is detached, do not return a path (its parent will
+					// also have been passed to remove() and be present)
+					const path: MirroredCollectionTestItem[] = new Array(node.depth + 1);
+					for (let i = node.depth; i >= 0; i--) {
+						if (!node) {
+							return undefined; // detached child
+						}
+
+						path[node.depth] = node;
+						node = node.parent ? collection.getMirroredTestDataById(node.parent) : undefined;
+					}
+
+					return path;
+				};
+
+				const addAncestorPath = (node: MirroredCollectionTestItem) => {
+					// fast path: if the common ancestor is already the root, no more work to do
+					if (ancestorPath && ancestorPath.length === 0) {
+						return;
+					}
+
+					const thisPath = buildAncestorPath(node);
+					if (!thisPath) {
+						return;
+					}
+
+					if (!ancestorPath) {
+						ancestorPath = thisPath;
+						return;
+					}
+
+					// removes node from the path to the ancestor that don't match
+					// the corresponding node in *this* path.
+					for (let i = ancestorPath.length - 1; i >= 0; i--) {
+						if (ancestorPath[i] !== thisPath[i]) {
+							ancestorPath.pop();
+						}
+					}
+				};
+
+				const addParentAncestor = (node: MirroredCollectionTestItem) => {
+					if (ancestorPath && ancestorPath.length === 0) {
+						// no-op
+					} else if (node.parent === null) {
+						ancestorPath = [];
+					} else {
+						const parent = collection.getMirroredTestDataById(node.parent);
+						if (parent) {
+							addAncestorPath(parent);
+						}
+					}
+				};
+
+				for (const node of added) { addParentAncestor(node); }
+				for (const node of updated) { addAncestorPath(node); }
+				for (const node of removed) { addParentAncestor(node); }
+
+				const ancestor = ancestorPath && ancestorPath[ancestorPath.length - 1];
+				return ancestor ? collection.getPublicTestItem(ancestor) : null;
+			},
+		};
+	}
+
+	public complete() {
+		if (!this.isEmpty) {
+			this.emitter.fire(this.getChangeEvent());
+		}
+	}
+}
+
 /**
  * Maintains tests in this extension host sent from the main thread.
  * @private
  */
 export class MirroredTestCollection extends AbstractIncrementalTestCollection<MirroredCollectionTestItem> {
-	private changeEmitter = new Emitter<vscode.TestItem | null>();
+	private changeEmitter = new Emitter<vscode.TestChangeEvent>();
 
 	/**
 	 * Change emitter that fires with the same sematics as `TestObserver.onDidChangeTests`.
 	 */
 	public readonly onDidChangeTests = this.changeEmitter.event;
 
-	/**
-	 * Mapping of mirrored test items to their underlying ID. Given here to avoid
-	 * exposing them to extensions.
-	 */
-	protected readonly mirroredTestIds = new WeakMap<vscode.TestItem, string>();
-
 	/**
 	 * Gets a list of root test items.
 	 */
@@ -412,49 +545,67 @@ export class MirroredTestCollection extends AbstractIncrementalTestCollection<Mi
 	public getAllAsTestItem(itemIds: ReadonlyArray<string>): vscode.TestItem[] {
 		return itemIds.map(itemId => {
 			const item = this.items.get(itemId);
-			return item && this.createCollectionItemWrapper(item);
+			return item && this.getPublicTestItem(item);
 		}).filter(isDefined);
 	}
 
+	/**
+	 *
+	 * If the test ID exists, returns its underlying ID.
+	 */
+	public getMirroredTestDataById(itemId: string) {
+		return this.items.get(itemId);
+	}
+
 	/**
 	 * If the test item is a mirrored test item, returns its underlying ID.
 	 */
 	public getMirroredTestDataByReference(item: vscode.TestItem) {
-		const itemId = this.mirroredTestIds.get(item);
-		return itemId ? this.items.get(itemId) : undefined;
+		const id = getMirroredItemId(item);
+		return id ? this.items.get(id) : undefined;
 	}
 
 	/**
 	 * @override
 	 */
-	protected createItem(item: InternalTestItem): MirroredCollectionTestItem {
-		return { ...item, revived: TestItem.to(item.item), children: new Set() };
+	protected createItem(item: InternalTestItem, parent?: MirroredCollectionTestItem): MirroredCollectionTestItem {
+		return { ...item, revived: TestItem.to(item.item), depth: parent ? parent.depth + 1 : 0, children: new Set() };
 	}
 
 	/**
 	 * @override
 	 */
-	protected onChange(item: MirroredCollectionTestItem | null) {
-		if (item) {
-			Object.assign(item.revived, TestItem.to(item.item));
-		}
-
-		this.changeEmitter.fire(item ? this.createCollectionItemWrapper(item) : null);
+	protected createChangeCollector() {
+		return new MirroredChangeCollector(this, this.changeEmitter);
 	}
 
-	private createCollectionItemWrapper(item: MirroredCollectionTestItem): vscode.TestItem {
+	/**
+	 * Gets the public test item instance for the given mirrored record.
+	 */
+	public getPublicTestItem(item: MirroredCollectionTestItem): vscode.TestItem {
 		if (!item.wrapped) {
 			item.wrapped = createMirroredTestItem(item, this);
-			this.mirroredTestIds.set(item.wrapped, item.id);
 		}
 
 		return item.wrapped;
 	}
 }
 
+const getMirroredItemId = (item: vscode.TestItem) => {
+	return (item as any)[MirroredItemId] as string | undefined;
+};
+
+const MirroredItemId = Symbol('MirroredItemId');
+
 const createMirroredTestItem = (internal: MirroredCollectionTestItem, collection: MirroredTestCollection): vscode.TestItem => {
 	const obj = {};
 
+	Object.defineProperty(obj, MirroredItemId, {
+		enumerable: false,
+		configurable: false,
+		value: internal.id,
+	});
+
 	Object.defineProperty(obj, 'children', {
 		enumerable: true,
 		configurable: false,

src/vs/workbench/contrib/testing/common/testCollection.ts

@@ -108,6 +108,33 @@ export interface IncrementalTestCollectionItem extends InternalTestItem {
 	children: Set<string>;
 }
 
+/**
+ * The IncrementalChangeCollector is used in the IncrementalTestCollection
+ * and called with diff changes as they're applied. This is used in the
+ * ext host to create a cohesive change event from a diff.
+ */
+export class IncrementalChangeCollector<T> {
+	/**
+	 * A node was added.
+	 */
+	public add(node: T): void { }
+
+	/**
+	 * A node in the collection was updated.
+	 */
+	public update(node: T): void { }
+
+	/**
+	 * A node was removed.
+	 */
+	public remove(node: T): void { }
+
+	/**
+	 * Called when the diff has been applied.
+	 */
+	public complete(): void { }
+}
+
 /**
  * Maintains tests in this extension host sent from the main thread.
  */
@@ -126,19 +153,23 @@ export abstract class AbstractIncrementalTestCollection<T extends IncrementalTes
 	 * Applies the diff to the collection.
 	 */
 	public apply(diff: TestsDiff) {
+		const changes = this.createChangeCollector();
+
 		for (const op of diff) {
 			switch (op[0]) {
 				case TestDiffOpType.Add: {
 					const item = op[1];
 					if (!item.parent) {
 						this.roots.add(item.id);
-						this.items.set(item.id, this.createItem(item));
-						this.onChange(null);
+						const created = this.createItem(item);
+						this.items.set(item.id, created);
+						changes.add(created);
 					} else if (this.items.has(item.parent)) {
 						const parent = this.items.get(item.parent)!;
 						parent.children.add(item.id);
-						this.items.set(item.id, this.createItem(item));
-						this.onChange(parent);
+						const created = this.createItem(item, parent);
+						this.items.set(item.id, created);
+						changes.add(created);
 					}
 					break;
 				}
@@ -148,7 +179,7 @@ export abstract class AbstractIncrementalTestCollection<T extends IncrementalTes
 					const existing = this.items.get(item.id);
 					if (existing) {
 						Object.assign(existing.item, item.item);
-						this.onChange(existing);
+						changes.update(existing);
 					}
 					break;
 				}
@@ -160,7 +191,8 @@ export abstract class AbstractIncrementalTestCollection<T extends IncrementalTes
 					}
 
 					if (toRemove.parent) {
-						this.items.get(toRemove.parent)!.children.delete(toRemove.id);
+						const parent = this.items.get(toRemove.parent)!;
+						parent.children.delete(toRemove.id);
 					} else {
 						this.roots.delete(toRemove.id);
 					}
@@ -172,26 +204,26 @@ export abstract class AbstractIncrementalTestCollection<T extends IncrementalTes
 							if (existing) {
 								queue.push(existing.children);
 								this.items.delete(itemId);
+								changes.remove(existing);
 							}
 						}
 					}
-
-					this.onChange(toRemove);
 				}
 			}
 		}
+
+		changes.complete();
 	}
 
 	/**
-	 * Called when an item in the collection changes, with the same semantics
-	 * as `onDidChangeTests` in vscode.d.ts.
+	 * Called before a diff is applied to create a new change collector.
 	 */
-	protected onChange(item: T | null): void {
-		// no-op
+	protected createChangeCollector() {
+		return new IncrementalChangeCollector<T>();
 	}
 
 	/**
 	 * Creates a new item for the collection from the internal test item.
 	 */
-	protected abstract createItem(internal: InternalTestItem): T;
+	protected abstract createItem(internal: InternalTestItem, parent?: T): T;
 }

src/vs/workbench/test/browser/api/extHostTesting.test.ts

@@ -8,7 +8,81 @@ import { MirroredTestCollection, OwnedTestCollection, SingleUseTestCollection }
 import * as convert from 'vs/workbench/api/common/extHostTypeConverters';
 import { TestRunState, TestState } from 'vs/workbench/api/common/extHostTypes';
 import { TestDiffOpType, TestsDiff } from 'vs/workbench/contrib/testing/common/testCollection';
-import { TestItem } from 'vscode';
+import { TestChangeEvent, TestItem } from 'vscode';
+
+const stubTest = (label: string): TestItem => ({
+	label,
+	location: undefined,
+	state: new TestState(TestRunState.Unset),
+	debuggable: true,
+	runnable: true,
+	description: ''
+});
+
+const assertTreesEqual = (a: Readonly<TestItem>, b: Readonly<TestItem>) => {
+	assert.deepStrictEqual({ ...a, children: undefined }, { ...b, children: undefined });
+
+	const aChildren = (a.children ?? []).sort();
+	const bChildren = (b.children ?? []).sort();
+	assert.strictEqual(aChildren.length, bChildren.length, `expected ${a.label}.children.length == ${b.label}.children.length`);
+	aChildren.forEach((_, i) => assertTreesEqual(aChildren[i], bChildren[i]));
+};
+
+const assertTreeListEqual = (a: ReadonlyArray<Readonly<TestItem>>, b: ReadonlyArray<Readonly<TestItem>>) => {
+	assert.strictEqual(a.length, b.length, `expected a.length == n.length`);
+	a.forEach((_, i) => assertTreesEqual(a[i], b[i]));
+};
+
+const stubNestedTests = () => ({
+	...stubTest('root'),
+	children: [
+		{ ...stubTest('a'), children: [stubTest('aa'), stubTest('ab')] },
+		stubTest('b'),
+	]
+});
+
+class TestOwnedTestCollection extends OwnedTestCollection {
+	public get idToInternal() {
+		return this.testIdToInternal;
+	}
+
+	public createForHierarchy(publishDiff: (diff: TestsDiff) => void = () => undefined) {
+		return new TestSingleUseCollection(this.testIdToInternal, publishDiff);
+	}
+}
+
+class TestSingleUseCollection extends SingleUseTestCollection {
+	private idCounter = 0;
+
+	public get itemToInternal() {
+		return this.testItemToInternal;
+	}
+
+	public get currentDiff() {
+		return this.diff;
+	}
+
+	protected getId() {
+		return String(this.idCounter++);
+	}
+
+	public setDiff(diff: TestsDiff) {
+		this.diff = diff;
+	}
+}
+
+class TestMirroredCollection extends MirroredTestCollection {
+	public changeEvent!: TestChangeEvent;
+
+	constructor() {
+		super();
+		this.onDidChangeTests(evt => this.changeEvent = evt);
+	}
+
+	public get length() {
+		return this.items.size;
+	}
+}
 
 suite('ExtHost Testing', () => {
 	let single: TestSingleUseCollection;
@@ -88,8 +162,10 @@ suite('ExtHost Testing', () => {
 	});
 
 	suite('MirroredTestCollection', () => {
+		let m: TestMirroredCollection;
+		setup(() => m = new TestMirroredCollection());
+
 		test('mirrors creation of the root', () => {
-			const m = new TestMirroredCollection();
 			const tests = stubNestedTests();
 			single.addRoot(tests, 'pid');
 			m.apply(single.collectDiff());
@@ -98,7 +174,6 @@ suite('ExtHost Testing', () => {
 		});
 
 		test('mirrors node deletion', () => {
-			const m = new TestMirroredCollection();
 			const tests = stubNestedTests();
 			single.addRoot(tests, 'pid');
 			m.apply(single.collectDiff());
@@ -111,7 +186,6 @@ suite('ExtHost Testing', () => {
 		});
 
 		test('mirrors node addition', () => {
-			const m = new TestMirroredCollection();
 			const tests = stubNestedTests();
 			single.addRoot(tests, 'pid');
 			m.apply(single.collectDiff());
@@ -124,7 +198,6 @@ suite('ExtHost Testing', () => {
 		});
 
 		test('mirrors node update', () => {
-			const m = new TestMirroredCollection();
 			const tests = stubNestedTests();
 			single.addRoot(tests, 'pid');
 			m.apply(single.collectDiff());
@@ -134,67 +207,78 @@ suite('ExtHost Testing', () => {
 
 			assertTreesEqual(m.rootTestItems[0], owned.getTestById('0')!.actual);
 		});
-	});
-});
-
-const stubTest = (label: string): TestItem => ({
-	label,
-	location: undefined,
-	state: new TestState(TestRunState.Unset),
-	debuggable: true,
-	runnable: true,
-	description: ''
-});
 
-const assertTreesEqual = (a: TestItem, b: TestItem) => {
-	assert.deepStrictEqual({ ...a, children: undefined }, { ...b, children: undefined });
-
-	const aChildren = (a.children ?? []).sort();
-	const bChildren = (b.children ?? []).sort();
-	assert.strictEqual(aChildren.length, bChildren.length, `expected ${a.label}.children.length == ${b.label}.children.length`);
-	aChildren.forEach((_, i) => assertTreesEqual(aChildren[i], bChildren[i]));
-};
-
-const stubNestedTests = () => ({
-	...stubTest('root'),
-	children: [
-		{ ...stubTest('a'), children: [stubTest('aa'), stubTest('ab')] },
-		stubTest('b'),
-	]
-});
+		suite('MirroredChangeCollector', () => {
+			let tests = stubNestedTests();
+			setup(() => {
+				tests = stubNestedTests();
+				single.addRoot(tests, 'pid');
+				m.apply(single.collectDiff());
+			});
 
-class TestOwnedTestCollection extends OwnedTestCollection {
-	public get idToInternal() {
-		return this.testIdToInternal;
-	}
+			test('creates change for root', () => {
+				assert.deepStrictEqual(m.changeEvent.commonChangeAncestor, null);
+				assertTreeListEqual(m.changeEvent.added, [
+					tests,
+					tests.children[0],
+					tests.children![0].children![0],
+					tests.children![0].children![1],
+					tests.children[1],
+				]);
+				assertTreeListEqual(m.changeEvent.removed, []);
+				assertTreeListEqual(m.changeEvent.updated, []);
+			});
 
-	public createForHierarchy(publishDiff: (diff: TestsDiff) => void = () => undefined) {
-		return new TestSingleUseCollection(this.testIdToInternal, publishDiff);
-	}
-}
+			test('creates change for delete', () => {
+				const rm = tests.children.shift()!;
+				single.onItemChange(tests, 'pid');
+				m.apply(single.collectDiff());
 
-class TestSingleUseCollection extends SingleUseTestCollection {
-	private idCounter = 0;
+				assertTreesEqual(m.changeEvent.commonChangeAncestor!, tests);
+				assertTreeListEqual(m.changeEvent.added, []);
+				assertTreeListEqual(m.changeEvent.removed, [
+					{ ...rm, children: [] },
+					{ ...rm.children![0], children: [] },
+					{ ...rm.children![1], children: [] },
+				]);
+				assertTreeListEqual(m.changeEvent.updated, []);
+			});
 
-	public get itemToInternal() {
-		return this.testItemToInternal;
-	}
+			test('creates change for update', () => {
+				tests.children[0].label = 'updated!';
+				single.onItemChange(tests, 'pid');
+				m.apply(single.collectDiff());
 
-	public get currentDiff() {
-		return this.diff;
-	}
+				assert.deepStrictEqual(m.changeEvent.commonChangeAncestor?.label, 'updated!');
+				assertTreeListEqual(m.changeEvent.added, []);
+				assertTreeListEqual(m.changeEvent.removed, []);
+				assertTreeListEqual(m.changeEvent.updated, [tests.children[0]]);
+			});
 
-	protected getId() {
-		return String(this.idCounter++);
-	}
+			test('is a no-op if a node is added and removed', () => {
+				const nested = stubNestedTests();
+				tests.children.push(nested);
+				single.onItemChange(tests, 'pid');
+				tests.children.pop();
+				single.onItemChange(tests, 'pid');
+				const previousEvent = m.changeEvent;
+				m.apply(single.collectDiff());
+				assert.strictEqual(m.changeEvent, previousEvent);
+			});
 
-	public setDiff(diff: TestsDiff) {
-		this.diff = diff;
-	}
-}
+			test('is a single-op if a node is added and changed', () => {
+				const child = stubTest('c');
+				tests.children.push(child);
+				single.onItemChange(tests, 'pid');
+				child.label = 'd';
+				single.onItemChange(tests, 'pid');
+				m.apply(single.collectDiff());
 
-class TestMirroredCollection extends MirroredTestCollection {
-	public get length() {
-		return this.items.size;
-	}
-}
+				assert.deepStrictEqual(m.changeEvent.commonChangeAncestor?.label, 'root');
+				assertTreeListEqual(m.changeEvent.added, [child]);
+				assertTreeListEqual(m.changeEvent.removed, []);
+				assertTreeListEqual(m.changeEvent.updated, []);
+			});
+		});
+	});
+});