Skip to content

Commit 24828a9

Browse files
connor4312connor4312
authored
Merge pull request microsoft#167100 from microsoft/connor4312/pr-166191
Correct updates to TestItem.error and TestItem.sortText
2 parents 96d0557 + d83c814 commit 24828a9

File tree

7 files changed

+171
-17
lines changed

7 files changed

+171
-17
lines changed

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

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -251,23 +251,56 @@ suite('ExtHost Testing', () => {
251251
]);
252252
});
253253

254+
test('replaces on uri change', () => {
255+
single.expand(single.root.id, Infinity);
256+
single.collectDiff();
257+
258+
const oldA = single.root.children.get('id-a') as TestItemImpl;
259+
const uri = single.root.children.get('id-a')!.uri?.with({ path: '/different' });
260+
const newA = new TestItemImpl('ctrlId', 'id-a', 'Hello world', uri);
261+
newA.children.replace([...oldA.children].map(([_, item]) => item));
262+
single.root.children.replace([...single.root.children].map(([id, i]) => id === 'id-a' ? newA : i));
263+
264+
assert.deepStrictEqual(single.collectDiff(), [
265+
{ op: TestDiffOpType.Remove, itemId: new TestId(['ctrlId', 'id-a']).toString() },
266+
{
267+
op: TestDiffOpType.Add,
268+
item: { controllerId: 'ctrlId', expand: TestItemExpandState.NotExpandable, item: { ...convert.TestItem.from(newA) } }
269+
},
270+
{
271+
op: TestDiffOpType.Add,
272+
item: { controllerId: 'ctrlId', expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(newA.children.get('id-aa') as TestItemImpl) }
273+
},
274+
{
275+
op: TestDiffOpType.Add,
276+
item: { controllerId: 'ctrlId', expand: TestItemExpandState.NotExpandable, item: convert.TestItem.from(newA.children.get('id-ab') as TestItemImpl) }
277+
},
278+
]);
279+
});
280+
254281
test('treats in-place replacement as mutation', () => {
255282
single.expand(single.root.id, Infinity);
256283
single.collectDiff();
257284

258285
const oldA = single.root.children.get('id-a') as TestItemImpl;
259-
const newA = new TestItemImpl('ctrlId', 'id-a', 'Hello world', undefined);
286+
const uri = single.root.children.get('id-a')!.uri;
287+
const newA = new TestItemImpl('ctrlId', 'id-a', 'Hello world', uri);
260288
newA.children.replace([...oldA.children].map(([_, item]) => item));
261289
single.root.children.replace([
262290
newA,
263-
new TestItemImpl('ctrlId', 'id-b', single.root.children.get('id-b')!.label, undefined),
291+
new TestItemImpl('ctrlId', 'id-b', single.root.children.get('id-b')!.label, uri),
264292
]);
265293

266294
assert.deepStrictEqual(single.collectDiff(), [
267295
{
268296
op: TestDiffOpType.Update,
269297
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), expand: TestItemExpandState.Expanded, item: { label: 'Hello world' } },
270298
},
299+
{
300+
op: TestDiffOpType.DocumentSynced,
301+
docv: undefined,
302+
uri: uri
303+
}
271304
]);
272305

273306
newA.label = 'still connected';
@@ -287,10 +320,11 @@ suite('ExtHost Testing', () => {
287320
single.collectDiff();
288321

289322
const oldA = single.root.children.get('id-a')!;
290-
const newA = new TestItemImpl('ctrlId', 'id-a', single.root.children.get('id-a')!.label, undefined);
323+
const uri = oldA.uri;
324+
const newA = new TestItemImpl('ctrlId', 'id-a', single.root.children.get('id-a')!.label, uri);
291325
const oldAA = oldA.children.get('id-aa')!;
292326
const oldAB = oldA.children.get('id-ab')!;
293-
const newAB = new TestItemImpl('ctrlId', 'id-ab', 'Hello world', undefined);
327+
const newAB = new TestItemImpl('ctrlId', 'id-ab', 'Hello world', uri);
294328
newA.children.replace([oldAA, newAB]);
295329
single.root.children.replace([newA, single.root.children.get('id-b')!]);
296330

@@ -303,6 +337,11 @@ suite('ExtHost Testing', () => {
303337
op: TestDiffOpType.Update,
304338
item: { extId: TestId.fromExtHostTestItem(oldAB, 'ctrlId').toString(), item: { label: 'Hello world' } },
305339
},
340+
{
341+
op: TestDiffOpType.DocumentSynced,
342+
docv: undefined,
343+
uri: uri
344+
}
306345
]);
307346

308347
oldAA.label = 'still connected1';
@@ -393,14 +432,15 @@ suite('ExtHost Testing', () => {
393432
]);
394433

395434
// sends on a child replacement
396-
const a2 = new TestItemImpl('ctrlId', 'id-a', 'a', URI.file('/'));
435+
const uri = URI.file('/');
436+
const a2 = new TestItemImpl('ctrlId', 'id-a', 'a', uri);
397437
a2.range = a.range;
398438
single.root.children.replace([a2, single.root.children.get('id-b')!]);
399439
assert.deepStrictEqual(single.collectDiff(), [
400440
{
401441
op: TestDiffOpType.DocumentSynced,
402442
docv: undefined,
403-
uri: URI.file('/')
443+
uri
404444
},
405445
]);
406446
});

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,9 @@ export class HierarchicalByLocationProjection extends Disposable implements ITes
162162
} else {
163163
this.changes.updated(existing);
164164
}
165+
if (patch.item?.sortText || (patch.item?.label && !existing.sortText)) {
166+
this.changes.sortKeyUpdated(existing);
167+
}
165168
break;
166169
}
167170

‎src/vs/workbench/contrib/testing/browser/explorerProjections/hierarchalNodes.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,21 @@ export class ByLocationTestItemElement extends TestItemTreeElement {
1818
protected readonly addedOrRemoved: (n: TestExplorerTreeElement) => void,
1919
) {
2020
super({ ...test, item: { ...test.item } }, parent);
21-
this.updateErrorVisiblity();
21+
this.updateErrorVisibility();
2222
}
2323

2424
public update(patch: ITestItemUpdate) {
2525
applyTestItemUpdate(this.test, patch);
26-
this.updateErrorVisiblity();
26+
this.updateErrorVisibility(patch);
2727
}
2828

29-
private updateErrorVisiblity() {
30-
if (this.errorChild && !this.test.item.error) {
29+
private updateErrorVisibility(patch?: ITestItemUpdate) {
30+
if (this.errorChild && (!this.test.item.error || patch?.item?.error)) {
3131
this.addedOrRemoved(this.errorChild);
3232
this.children.delete(this.errorChild);
3333
this.errorChild = undefined;
34-
} else if (this.test.item.error && !this.errorChild) {
34+
}
35+
if (this.test.item.error && !this.errorChild) {
3536
this.errorChild = new TestTreeErrorMessage(this.test.item.error, this);
3637
this.children.add(this.errorChild);
3738
this.addedOrRemoved(this.errorChild);

‎src/vs/workbench/contrib/testing/browser/explorerProjections/nodeHelper.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ const pruneNodesNotInTree = (nodes: Set<TestExplorerTreeElement | null>, tree: O
7070
export class NodeChangeList<T extends TestItemTreeElement> {
7171
private changedParents = new Set<T | null>();
7272
private updatedNodes = new Set<TestExplorerTreeElement>();
73+
private resortedNodes = new Set<TestExplorerTreeElement | null>();
7374
private omittedNodes = new WeakSet<TestExplorerTreeElement>();
7475
private isFirstApply = true;
7576

@@ -81,13 +82,18 @@ export class NodeChangeList<T extends TestItemTreeElement> {
8182
this.changedParents.add(this.getNearestNotOmittedParent(node));
8283
}
8384

85+
public sortKeyUpdated(node: TestExplorerTreeElement) {
86+
this.resortedNodes.add(node.parent);
87+
}
88+
8489
public applyTo(
8590
tree: ObjectTree<TestExplorerTreeElement, any>,
8691
renderNode: NodeRenderFn,
8792
roots: () => Iterable<T>,
8893
) {
8994
pruneNodesNotInTree(this.changedParents, tree);
9095
pruneNodesNotInTree(this.updatedNodes, tree);
96+
pruneNodesNotInTree(this.resortedNodes, tree);
9197

9298
const diffDepth = this.isFirstApply ? Infinity : 0;
9399
this.isFirstApply = false;
@@ -112,8 +118,15 @@ export class NodeChangeList<T extends TestItemTreeElement> {
112118
}
113119
}
114120

121+
for (const node of this.resortedNodes) {
122+
if (node && tree.hasElement(node)) {
123+
tree.resort(node, false);
124+
}
125+
}
126+
115127
this.changedParents.clear();
116128
this.updatedNodes.clear();
129+
this.resortedNodes.clear();
117130
}
118131

119132
private getNearestNotOmittedParent(node: TestExplorerTreeElement | null) {

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

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ const diffableProps: { [K in keyof ITestItem]?: (a: ITestItem[K], b: ITestItem[K
115115
label: strictEqualComparator,
116116
description: strictEqualComparator,
117117
error: strictEqualComparator,
118+
sortText: strictEqualComparator,
118119
tags: (a, b) => {
119120
if (a.length !== b.length) {
120121
return false;
@@ -128,9 +129,11 @@ const diffableProps: { [K in keyof ITestItem]?: (a: ITestItem[K], b: ITestItem[K
128129
},
129130
};
130131

132+
const diffableEntries = Object.entries(diffableProps) as readonly [keyof ITestItem, (a: any, b: any) => boolean][];
133+
131134
const diffTestItems = (a: ITestItem, b: ITestItem) => {
132135
let output: Record<string, unknown> | undefined;
133-
for (const [key, cmp] of Object.entries(diffableProps) as [keyof ITestItem, (a: any, b: any) => boolean][]) {
136+
for (const [key, cmp] of diffableEntries) {
134137
if (!cmp(a[key], b[key])) {
135138
if (output) {
136139
output[key] = b[key];
@@ -334,7 +337,7 @@ export class TestItemCollection<T extends ITestItemLike> extends Disposable {
334337
}
335338
}
336339

337-
private upsertItem(actual: T, parent: CollectionItem<T> | undefined) {
340+
private upsertItem(actual: T, parent: CollectionItem<T> | undefined): void {
338341
const fullId = TestId.fromExtHostTestItem(actual, this.root.id, parent?.actual);
339342

340343
// If this test item exists elsewhere in the tree already (exists at an
@@ -377,6 +380,12 @@ export class TestItemCollection<T extends ITestItemLike> extends Disposable {
377380
}
378381

379382
// Case 3: upsert of an existing item by ID, with a new instance
383+
if (internal.actual.uri?.toString() !== actual.uri?.toString()) {
384+
// If the item has a new URI, re-insert it; we don't support updating
385+
// URIs on existing test items.
386+
this.removeItem(fullId.toString());
387+
return this.upsertItem(actual, parent);
388+
}
380389
const oldChildren = this.options.getChildren(internal.actual);
381390
const oldActual = internal.actual;
382391
const update = diffTestItems(this.options.toITestItem(oldActual), this.options.toITestItem(actual));

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

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,5 +154,73 @@ suite('Workbench - Testing Explorer Hierarchal by Location Projection', () => {
154154
{ e: 'b', data: String(TestResultState.Unset) }
155155
]);
156156
});
157+
158+
test('applies test changes (resort)', async () => {
159+
harness.flush();
160+
harness.tree.expand(harness.projection.getElementByTestId(new TestId(['ctrlId', 'id-a']).toString())!);
161+
assert.deepStrictEqual(harness.flush(), [
162+
{ e: 'a', children: [{ e: 'aa' }, { e: 'ab' }] }, { e: 'b' }
163+
]);
164+
// sortText causes order to change
165+
harness.pushDiff({
166+
op: TestDiffOpType.Update,
167+
item: { extId: new TestId(['ctrlId', 'id-a', 'id-aa']).toString(), item: { sortText: "z" } }
168+
}, {
169+
op: TestDiffOpType.Update,
170+
item: { extId: new TestId(['ctrlId', 'id-a', 'id-ab']).toString(), item: { sortText: "a" } }
171+
});
172+
assert.deepStrictEqual(harness.flush(), [
173+
{ e: 'a', children: [{ e: 'ab' }, { e: 'aa' }] }, { e: 'b' }
174+
]);
175+
// label causes order to change
176+
harness.pushDiff({
177+
op: TestDiffOpType.Update,
178+
item: { extId: new TestId(['ctrlId', 'id-a', 'id-aa']).toString(), item: { sortText: undefined, label: "z" } }
179+
}, {
180+
op: TestDiffOpType.Update,
181+
item: { extId: new TestId(['ctrlId', 'id-a', 'id-ab']).toString(), item: { sortText: undefined, label: "a" } }
182+
});
183+
assert.deepStrictEqual(harness.flush(), [
184+
{ e: 'a', children: [{ e: 'a' }, { e: 'z' }] }, { e: 'b' }
185+
]);
186+
harness.pushDiff({
187+
op: TestDiffOpType.Update,
188+
item: { extId: new TestId(['ctrlId', 'id-a', 'id-aa']).toString(), item: { label: "a2" } }
189+
}, {
190+
op: TestDiffOpType.Update,
191+
item: { extId: new TestId(['ctrlId', 'id-a', 'id-ab']).toString(), item: { label: "z2" } }
192+
});
193+
assert.deepStrictEqual(harness.flush(), [
194+
{ e: 'a', children: [{ e: 'a2' }, { e: 'z2' }] }, { e: 'b' }
195+
]);
196+
});
197+
198+
test('applies test changes (error)', async () => {
199+
harness.flush();
200+
assert.deepStrictEqual(harness.flush(), [
201+
{ e: 'a' }, { e: 'b' }
202+
]);
203+
// sortText causes order to change
204+
harness.pushDiff({
205+
op: TestDiffOpType.Update,
206+
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), item: { error: "bad" } }
207+
});
208+
assert.deepStrictEqual(harness.flush(), [
209+
{ e: 'a' }, { e: 'b' }
210+
]);
211+
harness.tree.expand(harness.projection.getElementByTestId(new TestId(['ctrlId', 'id-a']).toString())!);
212+
assert.deepStrictEqual(harness.flush(), [
213+
{ e: 'a', children: [{ e: 'bad' }, { e: 'aa' }, { e: 'ab' }] }, { e: 'b' }
214+
]);
215+
harness.pushDiff({
216+
op: TestDiffOpType.Update,
217+
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), item: { error: "badder" } }
218+
});
219+
assert.deepStrictEqual(harness.flush(), [
220+
{ e: 'a', children: [{ e: 'badder' }, { e: 'aa' }, { e: 'ab' }] }, { e: 'b' }
221+
]);
222+
223+
});
224+
157225
});
158226

‎src/vs/workbench/contrib/testing/test/browser/testObjectTree.ts

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,12 @@ import { ObjectTree } from 'vs/base/browser/ui/tree/objectTree';
77
import { Emitter } from 'vs/base/common/event';
88
import { Disposable } from 'vs/base/common/lifecycle';
99
import { IWorkspaceFoldersChangeEvent } from 'vs/platform/workspace/common/workspace';
10-
import { ITestTreeProjection, TestExplorerTreeElement, TestItemTreeElement } from 'vs/workbench/contrib/testing/browser/explorerProjections/index';
10+
import { ITestTreeProjection, TestExplorerTreeElement, TestItemTreeElement, TestTreeErrorMessage } from 'vs/workbench/contrib/testing/browser/explorerProjections/index';
1111
import { MainThreadTestCollection } from 'vs/workbench/contrib/testing/common/mainThreadTestCollection';
1212
import { TestsDiff, TestsDiffOp } from 'vs/workbench/contrib/testing/common/testTypes';
1313
import { ITestService } from 'vs/workbench/contrib/testing/common/testService';
1414
import { testStubs } from 'vs/workbench/contrib/testing/test/common/testStubs';
15+
import { ITreeSorter } from 'vs/base/browser/ui/tree/tree';
1516

1617
type SerializedTree = { e: string; children?: SerializedTree[]; data?: string };
1718

@@ -20,7 +21,7 @@ element.style.height = '1000px';
2021
element.style.width = '200px';
2122

2223
export class TestObjectTree<T> extends ObjectTree<T, any> {
23-
constructor(serializer: (node: T) => string) {
24+
constructor(serializer: (node: T) => string, sorter?: ITreeSorter<T>) {
2425
super(
2526
'test',
2627
element,
@@ -40,7 +41,7 @@ export class TestObjectTree<T> extends ObjectTree<T, any> {
4041
}
4142
],
4243
{
43-
sorter: {
44+
sorter: sorter ?? {
4445
compare: (a, b) => serializer(a).localeCompare(serializer(b))
4546
}
4647
}
@@ -74,6 +75,24 @@ export class TestObjectTree<T> extends ObjectTree<T, any> {
7475

7576
const pos = (element: Element) => Number(element.parentElement!.parentElement!.getAttribute('aria-posinset'));
7677

78+
79+
class ByLabelTreeSorter implements ITreeSorter<TestExplorerTreeElement> {
80+
public compare(a: TestExplorerTreeElement, b: TestExplorerTreeElement): number {
81+
if (a instanceof TestTreeErrorMessage || b instanceof TestTreeErrorMessage) {
82+
return (a instanceof TestTreeErrorMessage ? -1 : 0) + (b instanceof TestTreeErrorMessage ? 1 : 0);
83+
}
84+
85+
if (a instanceof TestItemTreeElement && b instanceof TestItemTreeElement && a.test.item.uri && b.test.item.uri && a.test.item.uri.toString() === b.test.item.uri.toString() && a.test.item.range && b.test.item.range) {
86+
const delta = a.test.item.range.startLineNumber - b.test.item.range.startLineNumber;
87+
if (delta !== 0) {
88+
return delta;
89+
}
90+
}
91+
92+
return (a.sortText || a.label).localeCompare(b.sortText || b.label);
93+
}
94+
}
95+
7796
// names are hard
7897
export class TestTreeTestHarness<T extends ITestTreeProjection = ITestTreeProjection> extends Disposable {
7998
private readonly onDiff = this._register(new Emitter<TestsDiff>());
@@ -100,7 +119,8 @@ export class TestTreeTestHarness<T extends ITestTreeProjection = ITestTreeProjec
100119
collection,
101120
onDidProcessDiff: this.onDiff.event,
102121
} as any));
103-
this.tree = this._register(new TestObjectTree(t => 'label' in t ? t.label : t.message.toString()));
122+
const sorter = new ByLabelTreeSorter();
123+
this.tree = this._register(new TestObjectTree(t => 'label' in t ? t.label : t.message.toString(), sorter));
104124
this._register(this.tree.onDidChangeCollapseState(evt => {
105125
if (evt.node.element instanceof TestItemTreeElement) {
106126
this.projection.expandElement(evt.node.element, evt.deep ? Infinity : 0);

0 commit comments

Comments
 (0)