testing: remove redundant parent in InternalTestItem

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

@@ -751,7 +751,8 @@ class MirroredChangeCollector extends IncrementalChangeCollector<MirroredCollect
 
 		this.updated.delete(node);
 
-		if (node.parent && this.alreadyRemoved.has(node.parent)) {
+		const parentId = TestId.parentId(node.item.extId);
+		if (parentId && this.alreadyRemoved.has(parentId.toString())) {
 			this.alreadyRemoved.add(node.item.extId);
 			return;
 		}

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

@@ -1846,7 +1846,8 @@ export namespace TestResults {
 		const byInternalId = new Map<string, TestResultItem.Serialized>();
 		for (const item of serialized.items) {
 			byInternalId.set(item.item.extId, item);
-			if (serialized.request.targets.some(t => t.controllerId === item.controllerId && t.testIds.includes(item.item.extId))) {
+			const controllerId = TestId.root(item.item.extId);
+			if (serialized.request.targets.some(t => t.controllerId === controllerId && t.testIds.includes(item.item.extId))) {
 				roots.push(item);
 			}
 		}

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

@@ -102,27 +102,27 @@ suite('ExtHost Testing', () => {
 			assert.deepStrictEqual(single.collectDiff(), [
 				{
 					op: TestDiffOpType.Add,
-					item: { controllerId: 'ctrlId', parent: null, expand: TestItemExpandState.BusyExpanding, item: { ...convert.TestItem.from(single.root) } }
+					item: { controllerId: 'ctrlId', expand: TestItemExpandState.BusyExpanding, item: { ...convert.TestItem.from(single.root) } }
 				},
 				{
 					op: TestDiffOpType.Add,
-					item: { controllerId: 'ctrlId', parent: single.root.id, expand: TestItemExpandState.BusyExpanding, item: { ...convert.TestItem.from(a) } }
+					item: { controllerId: 'ctrlId', expand: TestItemExpandState.BusyExpanding, item: { ...convert.TestItem.from(a) } }
 				},
 				{
 					op: TestDiffOpType.Add,
-					item: { controllerId: 'ctrlId', parent: new TestId(['ctrlId', 'id-a']).toString(), expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(a.children.get('id-aa') as TestItemImpl) }
+					item: { controllerId: 'ctrlId', expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(a.children.get('id-aa') as TestItemImpl) }
 				},
 				{
 					op: TestDiffOpType.Add,
-					item: { controllerId: 'ctrlId', parent: new TestId(['ctrlId', 'id-a']).toString(), expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(a.children.get('id-ab') as TestItemImpl) }
+					item: { controllerId: 'ctrlId', expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(a.children.get('id-ab') as TestItemImpl) }
 				},
 				{
 					op: TestDiffOpType.Update,
 					item: { extId: new TestId(['ctrlId', 'id-a']).toString(), expand: TestItemExpandState.Expanded }
 				},
 				{
 					op: TestDiffOpType.Add,
-					item: { controllerId: 'ctrlId', parent: single.root.id, expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(b) }
+					item: { controllerId: 'ctrlId', expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(b) }
 				},
 				{
 					op: TestDiffOpType.Update,
@@ -184,7 +184,6 @@ suite('ExtHost Testing', () => {
 				{
 					op: TestDiffOpType.Add, item: {
 						controllerId: 'ctrlId',
-						parent: new TestId(['ctrlId', 'id-a']).toString(),
 						expand: TestItemExpandState.NotExpandable,
 						item: convert.TestItem.from(child),
 					}
@@ -213,7 +212,6 @@ suite('ExtHost Testing', () => {
 				{
 					op: TestDiffOpType.Add, item: {
 						controllerId: 'ctrlId',
-						parent: new TestId(['ctrlId', 'id-a']).toString(),
 						expand: TestItemExpandState.NotExpandable,
 						item: convert.TestItem.from(child),
 					}
@@ -326,7 +324,7 @@ suite('ExtHost Testing', () => {
 				},
 				{
 					op: TestDiffOpType.Add,
-					item: { controllerId: 'ctrlId', parent: new TestId(['ctrlId', 'id-a']).toString(), expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(b) }
+					item: { controllerId: 'ctrlId', expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(b) }
 				},
 			]);
 

src/vs/workbench/contrib/testing/browser/explorerProjections/hierarchalByLocation.ts

@@ -18,6 +18,7 @@ import { InternalTestItem, TestDiffOpType, TestItemExpandState, TestResultState,
 import { TestResultItemChangeReason } from 'vs/workbench/contrib/testing/common/testResult';
 import { ITestResultService } from 'vs/workbench/contrib/testing/common/testResultService';
 import { ITestService } from 'vs/workbench/contrib/testing/common/testService';
+import { TestId } from 'vs/workbench/contrib/testing/common/testId';
 
 const computedStateAccessor: IComputedStateAndDurationAccessor<IActionableTestTreeElement> = {
 	getOwnState: i => i instanceof TestItemTreeElement ? i.ownState : TestResultState.Unset,
@@ -212,7 +213,8 @@ export class HierarchicalByLocationProjection extends Disposable implements ITes
 	}
 
 	protected createItem(item: InternalTestItem): ByLocationTestItemElement {
-		const parent = item.parent ? this.items.get(item.parent)! : null;
+		const parentId = TestId.parentId(item.item.extId);
+		const parent = parentId ? this.items.get(parentId)! : null;
 		return new ByLocationTestItemElement(item, parent, n => this.changes.addedOrRemoved(n));
 	}
 

src/vs/workbench/contrib/testing/browser/explorerProjections/hierarchalByName.ts

@@ -12,6 +12,7 @@ import { NodeRenderDirective } from 'vs/workbench/contrib/testing/browser/explor
 import { InternalTestItem } from 'vs/workbench/contrib/testing/common/testTypes';
 import { ITestResultService } from 'vs/workbench/contrib/testing/common/testResultService';
 import { ITestService } from 'vs/workbench/contrib/testing/common/testService';
+import { TestId } from 'vs/workbench/contrib/testing/common/testId';
 
 /**
  * Type of test element in the list.
@@ -97,7 +98,8 @@ export class HierarchicalByNameProjection extends HierarchicalByLocationProjecti
 	 * @override
 	 */
 	protected override createItem(item: InternalTestItem): ByLocationTestItemElement {
-		const actualParent = item.parent ? this.items.get(item.parent) as ByNameTestItemElement : undefined;
+		const parentId = TestId.parentId(item.item.extId);
+		const actualParent = parentId ? this.items.get(parentId.toString()) as ByNameTestItemElement : undefined;
 		if (!actualParent) {
 			return new ByNameTestItemElement(item, null, r => this.changes.addedOrRemoved(r));
 		}

src/vs/workbench/contrib/testing/browser/testingDecorations.ts

@@ -41,6 +41,7 @@ import { testingRunAllIcon, testingRunIcon, testingStatesToIcons } from 'vs/work
 import { testMessageSeverityColors } from 'vs/workbench/contrib/testing/browser/theme';
 import { DefaultGutterClickAction, getTestingConfiguration, TestingConfigKeys } from 'vs/workbench/contrib/testing/common/configuration';
 import { labelForTestInState, Testing } from 'vs/workbench/contrib/testing/common/constants';
+import { TestId } from 'vs/workbench/contrib/testing/common/testId';
 import { ITestDecoration as IPublicTestDecoration, ITestingDecorationsService, TestDecorations } from 'vs/workbench/contrib/testing/common/testingDecorations';
 import { ITestingPeekOpener } from 'vs/workbench/contrib/testing/common/testingPeekOpener';
 import { isFailedState, maxPriority } from 'vs/workbench/contrib/testing/common/testingStates';
@@ -849,7 +850,7 @@ class MultiRunTestDecoration extends RunTestDecoration implements ITestDecoratio
 		const testItems = this.tests.map(testItem => ({
 			currentLabel: testItem.test.item.label,
 			testItem,
-			parent: testItem.test.parent,
+			parent: TestId.fromString(testItem.test.item.extId).parentId,
 		}));
 
 		const getLabelConflicts = (tests: typeof testItems) => {
@@ -865,9 +866,9 @@ class MultiRunTestDecoration extends RunTestDecoration implements ITestDecoratio
 		while ((conflicts = getLabelConflicts(testItems)).length && hasParent) {
 			for (const conflict of conflicts) {
 				if (conflict.parent) {
-					const parent = this.testService.collection.getNodeById(conflict.parent);
+					const parent = this.testService.collection.getNodeById(conflict.parent.toString());
 					conflict.currentLabel = parent?.item.label + ' > ' + conflict.currentLabel;
-					conflict.parent = parent?.parent ? parent.parent : null;
+					conflict.parent = conflict.parent.parentId;
 				} else {
 					hasParent = false;
 				}

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

@@ -89,11 +89,11 @@ export class MainThreadTestCollection extends AbstractIncrementalTestCollection<
 			for (const child of queue.pop()!) {
 				const item = this.items.get(child)!;
 				ops.push({
-					op: TestDiffOpType.Add, item: {
+					op: TestDiffOpType.Add,
+					item: {
 						controllerId: item.controllerId,
 						expand: item.expand,
 						item: item.item,
-						parent: item.parent,
 					}
 				});
 				queue.push(item.children);

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

@@ -84,6 +84,14 @@ export class TestId {
 		return base.toString() + TestIdPathParts.Delimiter + b;
 	}
 
+	/**
+	 * Cheaply gets the parent ID of a test identified with the string.
+	 */
+	public static parentId(idString: string) {
+		const idx = idString.lastIndexOf(TestIdPathParts.Delimiter);
+		return idx === -1 ? undefined : idString.slice(0, idx);
+	}
+
 	/**
 	 * Compares the position of the two ID strings.
 	 */
@@ -115,8 +123,8 @@ export class TestId {
 	/**
 	 * Gets the ID of the parent test.
 	 */
-	public get parentId(): TestId {
-		return this.viewEnd > 1 ? new TestId(this.path, this.viewEnd - 1) : this;
+	public get parentId(): TestId | undefined {
+		return this.viewEnd > 1 ? new TestId(this.path, this.viewEnd - 1) : undefined;
 	}
 
 	/**
@@ -150,6 +158,16 @@ export class TestId {
 		}
 	}
 
+	/**
+	 * Returns an iterable that yields IDs of the current item up to the root
+	 * item.
+	 */
+	public *idsToRoot() {
+		for (let i = this.viewEnd; i > 0; i--) {
+			yield new TestId(this.path, i);
+		}
+	}
+
 	/**
 	 * Compares the other test ID with this one.
 	 */

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

@@ -16,7 +16,6 @@ import { URI } from 'vs/base/common/uri';
  */
 interface CollectionItem<T> {
 	readonly fullId: TestId;
-	readonly parent: TestId | null;
 	actual: T;
 	expand: TestItemExpandState;
 	/**
@@ -351,7 +350,6 @@ export class TestItemCollection<T extends ITestItemLike> extends Disposable {
 			internal = {
 				fullId,
 				actual,
-				parent: parent ? fullId.parentId : null,
 				expandLevels: parent?.expandLevels /* intentionally undefined or 0 */ ? parent.expandLevels - 1 : undefined,
 				expand: TestItemExpandState.NotExpandable, // updated by `connectItemAndChildren`
 			};
@@ -362,7 +360,6 @@ export class TestItemCollection<T extends ITestItemLike> extends Disposable {
 			this.pushDiff({
 				op: TestDiffOpType.Add,
 				item: {
-					parent: internal.parent && internal.parent.toString(),
 					controllerId: this.options.controllerId,
 					expand: internal.expand,
 					item: this.options.toITestItem(actual),

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

@@ -7,14 +7,14 @@ import { newWriteableBufferStream, VSBuffer, VSBufferReadableStream, VSBufferWri
 import { Emitter } from 'vs/base/common/event';
 import { Lazy } from 'vs/base/common/lazy';
 import { DisposableStore } from 'vs/base/common/lifecycle';
-import { URI } from 'vs/base/common/uri';
 import { localize } from 'vs/nls';
 import { IComputedStateAccessor, refreshComputedState } from 'vs/workbench/contrib/testing/common/getComputedState';
 import { IObservableValue, MutableObservableValue, staticObservableValue } from 'vs/workbench/contrib/testing/common/observableValue';
 import { getMarkId, IRichLocation, ISerializedTestResults, ITestItem, ITestMessage, ITestOutputMessage, ITestRunTask, ITestTaskState, ResolvedTestRunRequest, TestItemExpandState, TestMessageType, TestResultItem, TestResultState } from 'vs/workbench/contrib/testing/common/testTypes';
 import { TestCoverage } from 'vs/workbench/contrib/testing/common/testCoverage';
 import { maxPriority, statesInOrder, terminalStatePriorities } from 'vs/workbench/contrib/testing/common/testingStates';
 import { removeAnsiEscapeCodes } from 'vs/base/common/strings';
+import { TestId } from 'vs/workbench/contrib/testing/common/testId';
 
 export interface ITestRunTaskResults extends ITestRunTask {
 	/**
@@ -88,10 +88,8 @@ export interface ITestResult {
 }
 
 export const resultItemParents = function* (results: ITestResult, item: TestResultItem) {
-	let i: TestResultItem | undefined = item;
-	while (i) {
-		yield i;
-		i = i.parent ? results.getStateById(i.parent) : undefined;
+	for (const id of TestId.fromString(item.item.extId).idsToRoot()) {
+		yield results.getStateById(id.toString())!;
 	}
 };
 
@@ -266,7 +264,6 @@ interface TestResultItemWithChildren extends TestResultItem {
 }
 
 const itemToNode = (controllerId: string, item: ITestItem, parent: string | null): TestResultItemWithChildren => ({
-	parent,
 	controllerId,
 	expand: TestItemExpandState.NotExpandable,
 	item: { ...item },
@@ -329,14 +326,11 @@ export class LiveTestResult implements ITestResult {
 		getParents: i => {
 			const { testById: testByExtId } = this;
 			return (function* () {
-				for (let parentId = i.parent; parentId;) {
-					const parent = testByExtId.get(parentId);
-					if (!parent) {
-						break;
+				const parentId = TestId.fromString(i.item.extId).parentId;
+				if (parentId) {
+					for (const id of parentId.idsToRoot()) {
+						yield testByExtId.get(id.toString())!;
 					}
-
-					yield parent;
-					parentId = parent.parent;
 				}
 			})();
 		},
@@ -665,11 +659,9 @@ export class HydratedTestResult implements ITestResult {
 		this.request = serialized.request;
 
 		for (const item of serialized.items) {
-			const cast: TestResultItem = { ...item } as any;
-			cast.item.uri = URI.revive(cast.item.uri);
-			cast.retired = true;
-			this.counts[item.ownComputedState]++;
-			this.testById.set(item.item.extId, cast);
+			const de = TestResultItem.deserialize(item);
+			this.counts[de.ownComputedState]++;
+			this.testById.set(item.item.extId, de);
 		}
 	}
 

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

@@ -77,10 +77,8 @@ export interface IMainThreadTestCollection extends AbstractIncrementalTestCollec
  * Iterates through the item and its parents to the root.
  */
 export const getCollectionItemParents = function* (collection: IMainThreadTestCollection, item: InternalTestItem) {
-	let i: InternalTestItem | undefined = item;
-	while (i) {
-		yield i;
-		i = i.parent ? collection.getNodeById(i.parent) : undefined;
+	for (const id of TestId.fromString(item.item.extId).idsToRoot()) {
+		yield collection.getNodeById(id.toString())!;
 	}
 };
 

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

@@ -8,6 +8,7 @@ import { MarshalledId } from 'vs/base/common/marshallingIds';
 import { URI, UriComponents } from 'vs/base/common/uri';
 import { IPosition } from 'vs/editor/common/core/position';
 import { IRange, Range } from 'vs/editor/common/core/range';
+import { TestId } from 'vs/workbench/contrib/testing/common/testId';
 
 export const enum TestResultState {
 	Unset = 0,
@@ -340,31 +341,27 @@ export interface InternalTestItem {
 	controllerId: string;
 	/** Expandability state */
 	expand: TestItemExpandState;
-	/** Parent ID, if any */
-	parent: string | null;
 	/** Raw test item properties */
 	item: ITestItem;
 }
 
 export namespace InternalTestItem {
 	export interface Serialized {
-		controllerId: string;
 		expand: TestItemExpandState;
-		parent: string | null;
 		item: ITestItem.Serialized;
 	}
 
 	export const serialize = (item: InternalTestItem): Serialized => ({
-		controllerId: item.controllerId,
 		expand: item.expand,
-		parent: item.parent,
 		item: ITestItem.serialize(item.item)
 	});
 
 	export const deserialize = (serialized: Serialized): InternalTestItem => ({
-		controllerId: serialized.controllerId,
+		// the `controllerId` is derived from the test.item.extId. It's redundant
+		// in the non-serialized InternalTestItem too, but there just because it's
+		// checked against in many hot paths.
+		controllerId: TestId.root(serialized.item.extId),
 		expand: serialized.expand,
-		parent: serialized.parent,
 		item: ITestItem.deserialize(serialized.item)
 	});
 }
@@ -469,6 +466,14 @@ export namespace TestResultItem {
 		tasks: original.tasks.map(ITestTaskState.serialize),
 		retired: original.retired,
 	});
+
+	export const deserialize = (serialized: Serialized): TestResultItem => ({
+		...InternalTestItem.deserialize(serialized),
+		ownComputedState: serialized.ownComputedState,
+		computedState: serialized.computedState,
+		tasks: serialized.tasks.map(ITestTaskState.deserialize),
+		retired: true,
+	});
 }
 
 export interface ISerializedTestResults {
@@ -684,13 +689,14 @@ export abstract class AbstractIncrementalTestCollection<T extends IncrementalTes
 			switch (op.op) {
 				case TestDiffOpType.Add: {
 					const internalTest = InternalTestItem.deserialize(op.item);
-					if (!internalTest.parent) {
+					const parentId = TestId.parentId(internalTest.item.extId)?.toString();
+					if (!parentId) {
 						const created = this.createItem(internalTest);
 						this.roots.add(created);
 						this.items.set(internalTest.item.extId, created);
 						changes.add(created);
-					} else if (this.items.has(internalTest.parent)) {
-						const parent = this.items.get(internalTest.parent)!;
+					} else if (this.items.has(parentId)) {
+						const parent = this.items.get(parentId)!;
 						parent.children.add(internalTest.item.extId);
 						const created = this.createItem(internalTest, parent);
 						this.items.set(internalTest.item.extId, created);
@@ -730,8 +736,9 @@ export abstract class AbstractIncrementalTestCollection<T extends IncrementalTes
 						break;
 					}
 
-					if (toRemove.parent) {
-						const parent = this.items.get(toRemove.parent)!;
+					const parentId = TestId.parentId(toRemove.item.extId)?.toString();
+					if (parentId) {
+						const parent = this.items.get(parentId)!;
 						parent.children.delete(toRemove.item.extId);
 					} else {
 						this.roots.delete(toRemove);