Skip to content

Commit

Permalink
testing: sort completed tests by start sequence, not completion time (#…
Browse files Browse the repository at this point in the history
…235759)

* testing: sort completed tests by start sequence, not completion time

Fixes #235667

The root cause of the issue is the separate test run that playwright does:

https://github.com/microsoft/playwright-vscode/blob/4fba0d0873ee9bf5de17219fa2f48201fd16162f/src/extension.ts#L403-L422.

VS Code by default only shows failure messages from the last-ended test
run, which was the original (unused) test run VS Code created, because
it ends automatically after the runHandler's promise resolves. A good
change to make, which happens to fix this bug, is ensuring results are
retained in the order the user started the test runs versus the order
in which they ended.

Note that playwright is able to avoid the duplicate run since #213182,
but I think this is still a sensible change to make.

* fix other test
  • Loading branch information
connor4312 authored Dec 10, 2024
1 parent 475acb4 commit 794c293
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 6 deletions.
1 change: 1 addition & 0 deletions src/vs/workbench/contrib/testing/common/testResult.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ export class LiveTestResult extends Disposable implements ITestResult {
public readonly id: string,
public readonly persist: boolean,
public readonly request: ResolvedTestRunRequest,
public readonly insertOrder: number,
@ITelemetryService private readonly telemetry: ITelemetryService,
) {
super();
Expand Down
17 changes: 14 additions & 3 deletions src/vs/workbench/contrib/testing/common/testResultService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export class TestResultService extends Disposable implements ITestResultService
private _results: ITestResult[] = [];
private readonly _resultsDisposables: DisposableStore[] = [];
private testChangeEmitter = this._register(new Emitter<TestResultItemChange>());
private insertOrderCounter = 0;

/**
* @inheritdoc
Expand Down Expand Up @@ -139,7 +140,7 @@ export class TestResultService extends Disposable implements ITestResultService
public createLiveResult(req: ResolvedTestRunRequest | ExtensionRunTestsRequest) {
if ('targets' in req) {
const id = generateUuid();
return this.push(new LiveTestResult(id, true, req, this.telemetryService));
return this.push(new LiveTestResult(id, true, req, this.insertOrderCounter++, this.telemetryService));
}

let profile: ITestRunProfile | undefined;
Expand All @@ -164,7 +165,7 @@ export class TestResultService extends Disposable implements ITestResultService
});
}

return this.push(new LiveTestResult(req.id, req.persist, resolved, this.telemetryService));
return this.push(new LiveTestResult(req.id, req.persist, resolved, this.insertOrderCounter++, this.telemetryService));
}

/**
Expand Down Expand Up @@ -251,7 +252,17 @@ export class TestResultService extends Disposable implements ITestResultService
}

private resort() {
this.results.sort((a, b) => (b.completedAt ?? Number.MAX_SAFE_INTEGER) - (a.completedAt ?? Number.MAX_SAFE_INTEGER));
this.results.sort((a, b) => {
// Running tests should always be sorted higher:
if (!!a.completedAt !== !!b.completedAt) {
return a.completedAt === undefined ? -1 : 1;
}

// Otherwise sort by insertion order, hydrated tests are always last:
const aComp = a instanceof LiveTestResult ? a.insertOrder : -1;
const bComp = b instanceof LiveTestResult ? b.insertOrder : -1;
return bComp - aComp;
});
}

private updateIsRunning() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,15 @@ suite('Workbench - Test Results Service', () => {
}]
});

let insertCounter = 0;

class TestLiveTestResult extends LiveTestResult {
constructor(
id: string,
persist: boolean,
request: ResolvedTestRunRequest,
) {
super(id, persist, request, NullTelemetryService);
super(id, persist, request, insertCounter++, NullTelemetryService);
ds.add(this);
}

Expand Down Expand Up @@ -263,27 +265,29 @@ suite('Workbench - Test Results Service', () => {
'',
false,
defaultOpts([]),
insertCounter++,
NullTelemetryService,
));
results.clear();

assert.deepStrictEqual(results.results, [r2]);
});

test('keeps ongoing tests on top', async () => {
test('keeps ongoing tests on top, restored order when done', async () => {
results.push(r);
const r2 = results.push(new LiveTestResult(
'',
false,
defaultOpts([]),
insertCounter++,
NullTelemetryService,
));

assert.deepStrictEqual(results.results, [r2, r]);
r2.markComplete();
assert.deepStrictEqual(results.results, [r, r2]);
r.markComplete();
assert.deepStrictEqual(results.results, [r, r2]);
assert.deepStrictEqual(results.results, [r2, r]);
});

const makeHydrated = async (completedAt = 42, state = TestResultState.Passed) => new HydratedTestResult({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ suite('Workbench - Test Result Storage', () => {
'',
true,
{ targets: [], group: TestRunProfileBitset.Run },
1,
NullTelemetryService,
));

Expand Down

0 comments on commit 794c293

Please sign in to comment.