Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

testing: greatly improve decoration syncing #161446

Merged
merged 2 commits into from Sep 22, 2022
Merged

testing: greatly improve decoration syncing #161446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7db88bf
testing: greatly improve decoration syncing
connor4312 Sep 21, 2022
37ff9c7
tweak
connor4312 Sep 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/vs/workbench/api/common/extHostTestItem.ts
View file
Open in desktop
Expand Up @@ -68,7 +68,24 @@ const evSetProps = <T>(fn: (newValue: T) => Partial<ITestItem>): (newValue: T) =
v => ({ op: TestItemEventOp.SetProp, update: fn(v) });

const makePropDescriptors = (api: IExtHostTestItemApi, label: string): { [K in keyof Required<WritableProps>]: PropertyDescriptor } => ({
range: testItemPropAccessor<'range'>(api, undefined, propComparators.range, evSetProps(r => ({ range: editorRange.Range.lift(Convert.Range.from(r)) }))),
range: (() => {
let value: vscode.Range | undefined;
const updateProps = evSetProps<vscode.Range | undefined>(r => ({ range: editorRange.Range.lift(Convert.Range.from(r)) }));
return {
enumerable: true,
configurable: false,
get() {
return value;
},
set(newValue: vscode.Range | undefined) {
api.listener?.({ op: TestItemEventOp.DocumentSynced });
if (!propComparators.range(value, newValue)) {
value = newValue;
api.listener?.(updateProps(newValue));
}
},
};
})(),
label: testItemPropAccessor<'label'>(api, label, propComparators.label, evSetProps(label => ({ label }))),
description: testItemPropAccessor<'description'>(api, undefined, propComparators.description, evSetProps(description => ({ description }))),
sortText: testItemPropAccessor<'sortText'>(api, undefined, propComparators.sortText, evSetProps(sortText => ({ sortText }))),
Expand Down
67 changes: 60 additions & 7 deletions src/vs/workbench/api/test/browser/extHostTesting.test.ts
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@ import { TestDiffOpType, TestItemExpandState, TestMessageType, TestsDiff } from
import { TestId } from 'vs/workbench/contrib/testing/common/testId';
import type { TestItem, TestRunRequest } from 'vscode';
import { ExtHostDocumentsAndEditors } from 'vs/workbench/api/common/extHostDocumentsAndEditors';
import * as editorRange from 'vs/editor/common/core/range';

const simplify = (item: TestItem) => ({
id: item.id,
Expand Down Expand Up @@ -153,7 +154,7 @@ suite('ExtHost Testing', () => {
assert.deepStrictEqual(single.collectDiff(), [
{
op: TestDiffOpType.Update,
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), docv: undefined, item: { description: 'Hello world' } },
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), item: { description: 'Hello world' } },
}
]);
});
Expand Down Expand Up @@ -254,15 +255,15 @@ suite('ExtHost Testing', () => {
assert.deepStrictEqual(single.collectDiff(), [
{
op: TestDiffOpType.Update,
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), expand: TestItemExpandState.Expanded, docv: undefined, item: { label: 'Hello world' } },
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), expand: TestItemExpandState.Expanded, item: { label: 'Hello world' } },
},
]);

newA.label = 'still connected';
assert.deepStrictEqual(single.collectDiff(), [
{
op: TestDiffOpType.Update,
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), docv: undefined, item: { label: 'still connected' } }
item: { extId: new TestId(['ctrlId', 'id-a']).toString(), item: { label: 'still connected' } }
},
]);

Expand All @@ -289,7 +290,7 @@ suite('ExtHost Testing', () => {
},
{
op: TestDiffOpType.Update,
item: { extId: TestId.fromExtHostTestItem(oldAB, 'ctrlId').toString(), docv: undefined, item: { label: 'Hello world' } },
item: { extId: TestId.fromExtHostTestItem(oldAB, 'ctrlId').toString(), item: { label: 'Hello world' } },
},
]);

Expand All @@ -299,11 +300,11 @@ suite('ExtHost Testing', () => {
assert.deepStrictEqual(single.collectDiff(), [
{
op: TestDiffOpType.Update,
item: { extId: new TestId(['ctrlId', 'id-a', 'id-aa']).toString(), docv: undefined, item: { label: 'still connected1' } }
item: { extId: new TestId(['ctrlId', 'id-a', 'id-aa']).toString(), item: { label: 'still connected1' } }
},
{
op: TestDiffOpType.Update,
item: { extId: new TestId(['ctrlId', 'id-a', 'id-ab']).toString(), docv: undefined, item: { label: 'still connected2' } }
item: { extId: new TestId(['ctrlId', 'id-a', 'id-ab']).toString(), item: { label: 'still connected2' } }
},
]);

Expand Down Expand Up @@ -333,13 +334,65 @@ suite('ExtHost Testing', () => {
assert.deepStrictEqual(single.collectDiff(), [
{
op: TestDiffOpType.Update,
item: { extId: new TestId(['ctrlId', 'id-a', 'id-b']).toString(), docv: undefined, item: { label: 'still connected' } }
item: { extId: new TestId(['ctrlId', 'id-a', 'id-b']).toString(), item: { label: 'still connected' } }
},
]);

assert.deepStrictEqual([...single.root.children].map(([_, item]) => item), [single.root.children.get('id-a')]);
assert.deepStrictEqual(b.parent, a);
});

test('sends document sync events', async () => {
await single.expand(single.root.id, 0);
single.collectDiff();

const a = single.root.children.get('id-a') as TestItemImpl;
a.range = new Range(new Position(0, 0), new Position(1, 0));

assert.deepStrictEqual(single.collectDiff(), [
{
op: TestDiffOpType.DocumentSynced,
docv: undefined,
uri: URI.file('/')
},
{
op: TestDiffOpType.Update,
item: {
extId: new TestId(['ctrlId', 'id-a']).toString(),
item: {
range: editorRange.Range.lift({
endColumn: 1,
endLineNumber: 2,
startColumn: 1,
startLineNumber: 1
})
}
},
},
]);

// sends on replace even if it's a no-op
a.range = a.range;
assert.deepStrictEqual(single.collectDiff(), [
{
op: TestDiffOpType.DocumentSynced,
docv: undefined,
uri: URI.file('/')
},
]);

// sends on a child replacement
const a2 = new TestItemImpl('ctrlId', 'id-a', 'a', URI.file('/'));
a2.range = a.range;
single.root.children.replace([a2, single.root.children.get('id-b')!]);
assert.deepStrictEqual(single.collectDiff(), [
{
op: TestDiffOpType.DocumentSynced,
docv: undefined,
uri: URI.file('/')
},
]);
});
});


Expand Down
32 changes: 26 additions & 6 deletions src/vs/workbench/contrib/testing/browser/testingDecorations.ts
View file
Open in desktop
Expand Up @@ -115,14 +115,13 @@ export class TestingDecorationService extends Disposable implements ITestingDeco
// is up to date. This prevents issues, as in #138632, #138835, #138922.
this._register(this.testService.onWillProcessDiff(diff => {
for (const entry of diff) {
if (entry.op !== TestDiffOpType.Update || entry.item.docv === undefined || entry.item.item?.range === undefined) {
if (entry.op !== TestDiffOpType.DocumentSynced) {
continue;
}

const uri = this.testService.collection.getNodeById(entry.item.extId)?.item.uri;
const rec = uri && this.decorationCache.get(uri);
const rec = this.decorationCache.get(entry.uri);
if (rec) {
rec.rangeUpdateVersionId = entry.item.docv;
rec.rangeUpdateVersionId = entry.docv;
}
}

Expand Down Expand Up @@ -193,7 +192,7 @@ export class TestingDecorationService extends Disposable implements ITestingDeco
const uriStr = model.uri.toString();
const cached = this.decorationCache.get(model.uri);
const testRangesUpdated = cached?.rangeUpdateVersionId === model.getVersionId();
const lastDecorations = cached?.value ?? new TestDecorations();
const lastDecorations = cached?.value ?? new TestDecorations<ITestDecoration>();
const newDecorations = new TestDecorations<ITestDecoration>();

model.changeDecorations(accessor => {
Expand All @@ -210,7 +209,7 @@ export class TestingDecorationService extends Disposable implements ITestingDeco

for (const [line, tests] of runDecorations.lines()) {
const multi = tests.length > 1;
let existing = lastDecorations.findOnLine(line, d => multi ? d instanceof MultiRunTestDecoration : d instanceof RunSingleTestDecoration) as RunTestDecoration | undefined;
let existing = lastDecorations.value.find(d => d instanceof RunTestDecoration && d.exactlyContainsTests(tests)) as RunTestDecoration | undefined;

// see comment in the constructor for what's going on here
if (existing && testRangesUpdated && model.getDecorationRange(existing.id)?.startLineNumber !== line) {
Expand Down Expand Up @@ -652,6 +651,27 @@ abstract class RunTestDecoration {
return true;
}

public exactlyContainsTests(tests: readonly { test: IncrementalTestCollectionItem }[]): boolean {
if (tests.length !== this.tests.length) {
return false;
}
if (tests.length === 1) {
return tests[0].test.item.extId === this.tests[0].test.item.extId;
}

const ownTests = new Set();
for (const t of this.tests) {
ownTests.add(t.test.item.extId);
}
for (const t of tests) {
if (!ownTests.delete(t.test.item.extId)) {
return false;
}
}

return true;
}

/**
* Updates the decoration to match the new set of tests.
* @returns true if options were changed, false otherwise
Expand Down
60 changes: 49 additions & 11 deletions src/vs/workbench/contrib/testing/common/testItemCollection.ts
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@ export const enum TestItemEventOp {
RemoveChild,
SetProp,
Bulk,
DocumentSynced,
}

export interface ITestItemUpsertChild {
Expand Down Expand Up @@ -65,13 +66,18 @@ export interface ITestItemBulkReplace {
ops: (ITestItemUpsertChild | ITestItemRemoveChild)[];
}

export interface ITestItemDocumentSynced {
op: TestItemEventOp.DocumentSynced;
}

export type ExtHostTestItemEvent =
| ITestItemSetTags
| ITestItemUpsertChild
| ITestItemRemoveChild
| ITestItemUpdateCanResolveChildren
| ITestItemSetProp
| ITestItemBulkReplace;
| ITestItemBulkReplace
| ITestItemDocumentSynced;

export interface ITestItemApi<T> {
controllerId: string;
Expand Down Expand Up @@ -201,17 +207,32 @@ export class TestItemCollection<T extends ITestItemLike> extends Disposable {
* Pushes a new diff entry onto the collected diff list.
*/
public pushDiff(diff: TestsDiffOp) {
// Try to merge updates, since they're invoked per-property
const last = this.diff[this.diff.length - 1];
if (last && diff.op === TestDiffOpType.Update) {
if (last.op === TestDiffOpType.Update && last.item.extId === diff.item.extId) {
applyTestItemUpdate(last.item, diff.item);
return;
switch (diff.op) {
case TestDiffOpType.DocumentSynced: {
for (const existing of this.diff) {
if (existing.op === TestDiffOpType.DocumentSynced && existing.uri === diff.uri) {
existing.docv = diff.docv;
return;
}
}

break;
}
case TestDiffOpType.Update: {
// Try to merge updates, since they're invoked per-property
const last = this.diff[this.diff.length - 1];
if (last) {
if (last.op === TestDiffOpType.Update && last.item.extId === diff.item.extId) {
applyTestItemUpdate(last.item, diff.item);
return;
}

if (last.op === TestDiffOpType.Add && last.item.item.extId === diff.item.extId) {
applyTestItemUpdate(last.item, diff.item);
return;
if (last.op === TestDiffOpType.Add && last.item.item.extId === diff.item.extId) {
applyTestItemUpdate(last.item, diff.item);
return;
}
}
break;
}
}

Expand Down Expand Up @@ -291,15 +312,29 @@ export class TestItemCollection<T extends ITestItemLike> extends Disposable {
item: {
extId: internal.fullId.toString(),
item: evt.update,
docv: this.options.getDocumentVersion(internal.actual.uri),
}
});
break;

case TestItemEventOp.DocumentSynced:
this.documentSynced(internal.actual.uri);
break;

default:
assertNever(evt);
}
}

private documentSynced(uri: URI | undefined) {
if (uri) {
this.pushDiff({
op: TestDiffOpType.DocumentSynced,
uri,
docv: this.options.getDocumentVersion(uri)
});
}
}

private upsertItem(actual: T, parent: CollectionItem<T> | undefined) {
const fullId = TestId.fromExtHostTestItem(actual, this.root.id, parent?.actual);

Expand Down Expand Up @@ -370,6 +405,9 @@ export class TestItemCollection<T extends ITestItemLike> extends Disposable {
this.removeItem(TestId.joinToString(fullId, child.id));
}
}

// Mark ranges in the document as synced (#161320)
this.documentSynced(internal.actual.uri);
}

private diffTagRefs(newTags: readonly ITestTag[], oldTags: readonly ITestTag[], extId: string) {
Expand Down