From 794c293bb5dbd64c349dec21163f832172acee2e Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Tue, 10 Dec 2024 16:05:00 -0600 Subject: [PATCH] testing: sort completed tests by start sequence, not completion time (#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 --- .../contrib/testing/common/testResult.ts | 1 + .../contrib/testing/common/testResultService.ts | 17 ++++++++++++++--- .../test/common/testResultService.test.ts | 10 +++++++--- .../test/common/testResultStorage.test.ts | 1 + 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/vs/workbench/contrib/testing/common/testResult.ts b/src/vs/workbench/contrib/testing/common/testResult.ts index 53353b58d9891..ed3db4926678a 100644 --- a/src/vs/workbench/contrib/testing/common/testResult.ts +++ b/src/vs/workbench/contrib/testing/common/testResult.ts @@ -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(); diff --git a/src/vs/workbench/contrib/testing/common/testResultService.ts b/src/vs/workbench/contrib/testing/common/testResultService.ts index 8a836a390866b..8790f01262f2d 100644 --- a/src/vs/workbench/contrib/testing/common/testResultService.ts +++ b/src/vs/workbench/contrib/testing/common/testResultService.ts @@ -78,6 +78,7 @@ export class TestResultService extends Disposable implements ITestResultService private _results: ITestResult[] = []; private readonly _resultsDisposables: DisposableStore[] = []; private testChangeEmitter = this._register(new Emitter()); + private insertOrderCounter = 0; /** * @inheritdoc @@ -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; @@ -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)); } /** @@ -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() { diff --git a/src/vs/workbench/contrib/testing/test/common/testResultService.test.ts b/src/vs/workbench/contrib/testing/test/common/testResultService.test.ts index 38c0a2c83c252..ac0ba381e3bc1 100644 --- a/src/vs/workbench/contrib/testing/test/common/testResultService.test.ts +++ b/src/vs/workbench/contrib/testing/test/common/testResultService.test.ts @@ -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); } @@ -263,6 +265,7 @@ suite('Workbench - Test Results Service', () => { '', false, defaultOpts([]), + insertCounter++, NullTelemetryService, )); results.clear(); @@ -270,12 +273,13 @@ suite('Workbench - Test Results Service', () => { 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, )); @@ -283,7 +287,7 @@ suite('Workbench - Test Results Service', () => { 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({ diff --git a/src/vs/workbench/contrib/testing/test/common/testResultStorage.test.ts b/src/vs/workbench/contrib/testing/test/common/testResultStorage.test.ts index 01218dbbe721c..34c9fb80d0d35 100644 --- a/src/vs/workbench/contrib/testing/test/common/testResultStorage.test.ts +++ b/src/vs/workbench/contrib/testing/test/common/testResultStorage.test.ts @@ -25,6 +25,7 @@ suite('Workbench - Test Result Storage', () => { '', true, { targets: [], group: TestRunProfileBitset.Run }, + 1, NullTelemetryService, ));