Skip to content

Commit e4d0fee

Browse files
r3m0tr3m0t
committed
Correct updates to TestItem.error and TestItem.sortText (Fixes microsoft#166190)
1 parent 3c4fc51 commit e4d0fee

File tree

7 files changed

+139
-15
lines changed

7 files changed

+139
-15
lines changed

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

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -256,18 +256,24 @@ suite('ExtHost Testing', () => {
256256
single.collectDiff();
257257

258258
const oldA = single.root.children.get('id-a') as TestItemImpl;
259-
const newA = new TestItemImpl('ctrlId', 'id-a', 'Hello world', undefined);
259+
const uri = single.root.children.get('id-a')!.uri;
260+
const newA = new TestItemImpl('ctrlId', 'id-a', 'Hello world', uri);
260261
newA.children.replace([...oldA.children].map(([_, item]) => item));
261262
single.root.children.replace([
262263
newA,
263-
new TestItemImpl('ctrlId', 'id-b', single.root.children.get('id-b')!.label, undefined),
264+
new TestItemImpl('ctrlId', 'id-b', single.root.children.get('id-b')!.label, uri),
264265
]);
265266

266267
assert.deepStrictEqual(single.collectDiff(), [
267268
{
268269
op: TestDiffOpType.Update,
269270
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), expand: TestItemExpandState.Expanded, item: { label: 'Hello world' } },
270271
},
272+
{
273+
op: TestDiffOpType.DocumentSynced,
274+
docv: undefined,
275+
uri: uri
276+
}
271277
]);
272278

273279
newA.label = 'still connected';
@@ -287,10 +293,11 @@ suite('ExtHost Testing', () => {
287293
single.collectDiff();
288294

289295
const oldA = single.root.children.get('id-a')!;
290-
const newA = new TestItemImpl('ctrlId', 'id-a', single.root.children.get('id-a')!.label, undefined);
296+
const uri = oldA.uri;
297+
const newA = new TestItemImpl('ctrlId', 'id-a', single.root.children.get('id-a')!.label, uri);
291298
const oldAA = oldA.children.get('id-aa')!;
292299
const oldAB = oldA.children.get('id-ab')!;
293-
const newAB = new TestItemImpl('ctrlId', 'id-ab', 'Hello world', undefined);
300+
const newAB = new TestItemImpl('ctrlId', 'id-ab', 'Hello world', uri);
294301
newA.children.replace([oldAA, newAB]);
295302
single.root.children.replace([newA, single.root.children.get('id-b')!]);
296303

@@ -303,6 +310,11 @@ suite('ExtHost Testing', () => {
303310
op: TestDiffOpType.Update,
304311
item: { extId: TestId.fromExtHostTestItem(oldAB, 'ctrlId').toString(), item: { label: 'Hello world' } },
305312
},
313+
{
314+
op: TestDiffOpType.DocumentSynced,
315+
docv: undefined,
316+
uri: uri
317+
}
306318
]);
307319

308320
oldAA.label = 'still connected1';
@@ -393,14 +405,15 @@ suite('ExtHost Testing', () => {
393405
]);
394406

395407
// sends on a child replacement
396-
const a2 = new TestItemImpl('ctrlId', 'id-a', 'a', URI.file('/'));
408+
const uri = URI.file('/');
409+
const a2 = new TestItemImpl('ctrlId', 'id-a', 'a', uri);
397410
a2.range = a.range;
398411
single.root.children.replace([a2, single.root.children.get('id-b')!]);
399412
assert.deepStrictEqual(single.collectDiff(), [
400413
{
401414
op: TestDiffOpType.DocumentSynced,
402415
docv: undefined,
403-
uri: URI.file('/')
416+
uri
404417
},
405418
]);
406419
});

‎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: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,12 @@ const diffableProps: { [K in keyof ITestItem]?: (a: ITestItem[K], b: ITestItem[K
115115
label: strictEqualComparator,
116116
description: strictEqualComparator,
117117
error: strictEqualComparator,
118+
sortText: strictEqualComparator,
119+
uri: (a, b) => {
120+
if (a === b) { return true; }
121+
if (!a || !b) { return false; }
122+
return a.toString() === b.toString();
123+
},
118124
tags: (a, b) => {
119125
if (a.length !== b.length) {
120126
return false;

‎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)