From d67515f6c18f136b1e72d9f3b31f01a1add5f736 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 20 Oct 2023 17:01:46 -0700 Subject: [PATCH] chore(test runner): do not produce some of the fake skipped test results (#27730) --- packages/playwright/src/reporters/base.ts | 9 ++++++++- packages/playwright/src/reporters/markdown.ts | 3 ++- packages/playwright/src/runner/dispatcher.ts | 14 +++++++------- packages/playwright/src/runner/tasks.ts | 8 +------- tests/playwright-test/deps.spec.ts | 10 +++++----- tests/playwright-test/hooks.spec.ts | 2 +- tests/playwright-test/max-failures.spec.ts | 6 ++---- tests/playwright-test/playwright-test-fixtures.ts | 3 +++ tests/playwright-test/reporter-base.spec.ts | 2 +- tests/playwright-test/runner.spec.ts | 6 +++--- 10 files changed, 33 insertions(+), 30 deletions(-) diff --git a/packages/playwright/src/reporters/base.ts b/packages/playwright/src/reporters/base.ts index 205e58da09..15bb85159f 100644 --- a/packages/playwright/src/reporters/base.ts +++ b/packages/playwright/src/reporters/base.ts @@ -35,6 +35,7 @@ type ErrorDetails = { }; type TestSummary = { + didNotRun: number; skipped: number; expected: number; interrupted: TestCase[]; @@ -158,7 +159,7 @@ export class BaseReporter implements ReporterV2 { return fileDurations.filter(([, duration]) => duration > threshold).slice(0, count); } - protected generateSummaryMessage({ skipped, expected, interrupted, unexpected, flaky, fatalErrors }: TestSummary) { + protected generateSummaryMessage({ didNotRun, skipped, expected, interrupted, unexpected, flaky, fatalErrors }: TestSummary) { const tokens: string[] = []; if (unexpected.length) { tokens.push(colors.red(` ${unexpected.length} failed`)); @@ -177,6 +178,8 @@ export class BaseReporter implements ReporterV2 { } if (skipped) tokens.push(colors.yellow(` ${skipped} skipped`)); + if (didNotRun) + tokens.push(colors.yellow(` ${didNotRun} did not run`)); if (expected) tokens.push(colors.green(` ${expected} passed`) + colors.dim(` (${milliseconds(this.result.duration)})`)); if (this.result.status === 'timedout') @@ -188,6 +191,7 @@ export class BaseReporter implements ReporterV2 { } protected generateSummary(): TestSummary { + let didNotRun = 0; let skipped = 0; let expected = 0; const interrupted: TestCase[] = []; @@ -202,6 +206,8 @@ export class BaseReporter implements ReporterV2 { if (test.results.some(result => !!result.error)) interruptedToPrint.push(test); interrupted.push(test); + } else if (!test.results.length) { + ++didNotRun; } else { ++skipped; } @@ -215,6 +221,7 @@ export class BaseReporter implements ReporterV2 { const failuresToPrint = [...unexpected, ...flaky, ...interruptedToPrint]; return { + didNotRun, skipped, expected, interrupted, diff --git a/packages/playwright/src/reporters/markdown.ts b/packages/playwright/src/reporters/markdown.ts index 97c25424ca..52f67a548e 100644 --- a/packages/playwright/src/reporters/markdown.ts +++ b/packages/playwright/src/reporters/markdown.ts @@ -63,7 +63,8 @@ class MarkdownReporter extends BaseReporter { lines.push(``); } const skipped = summary.skipped ? `, ${summary.skipped} skipped` : ''; - lines.push(`**${summary.expected} passed${skipped}**`); + const didNotRun = summary.didNotRun ? `, ${summary.didNotRun} did not run` : ''; + lines.push(`**${summary.expected} passed${skipped}${didNotRun}**`); lines.push(`:heavy_check_mark::heavy_check_mark::heavy_check_mark:`); lines.push(``); diff --git a/packages/playwright/src/runner/dispatcher.ts b/packages/playwright/src/runner/dispatcher.ts index bcf9b3c261..a89f996fad 100644 --- a/packages/playwright/src/runner/dispatcher.ts +++ b/packages/playwright/src/runner/dispatcher.ts @@ -140,11 +140,6 @@ export class Dispatcher { if (this._workerSlots.some(w => w.busy)) return; - for (const test of this._allTests) { - // Emulate skipped test run if we have stopped early. - if (!test.results.length) - test._appendTestResult().status = 'skipped'; - } this._finished.resolve(); } @@ -513,8 +508,13 @@ class JobDispatcher { // with skipped tests mixed in-between non-skipped. This makes // for a better reporter experience. const allTestsSkipped = this._job.tests.every(test => test.expectedStatus === 'skipped'); - if (allTestsSkipped) { - this._massSkipTestsFromRemaining(new Set(this._remainingByTestId.keys()), []); + if (allTestsSkipped && !this._failureTracker.hasReachedMaxFailures()) { + for (const test of this._job.tests) { + const result = test._appendTestResult(); + this._reporter.onTestBegin(test, result); + result.status = 'skipped'; + this._reportTestEnd(test, result); + } return true; } return false; diff --git a/packages/playwright/src/runner/tasks.ts b/packages/playwright/src/runner/tasks.ts index 1a72d858bb..14195201c8 100644 --- a/packages/playwright/src/runner/tasks.ts +++ b/packages/playwright/src/runner/tasks.ts @@ -274,14 +274,8 @@ function createRunTestsTask(): Task { extraEnvByProjectId.set(project.id, extraEnv); const hasFailedDeps = project.deps.some(p => !successfulProjects.has(p)); - if (!hasFailedDeps) { + if (!hasFailedDeps) phaseTestGroups.push(...testGroups); - } else { - for (const testGroup of testGroups) { - for (const test of testGroup.tests) - test._appendTestResult().status = 'skipped'; - } - } } if (phaseTestGroups.length) { diff --git a/tests/playwright-test/deps.spec.ts b/tests/playwright-test/deps.spec.ts index f417185e5f..26417bd7bc 100644 --- a/tests/playwright-test/deps.spec.ts +++ b/tests/playwright-test/deps.spec.ts @@ -133,7 +133,7 @@ test('should not run project if dependency failed', async ({ runInlineTest }) => expect(result.exitCode).toBe(1); expect(result.passed).toBe(1); expect(result.failed).toBe(1); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.output).toContain('Failed project B'); expect(result.outputLines).toEqual(['A', 'B']); }); @@ -297,7 +297,7 @@ test('should not filter dependency by only 3', async ({ runInlineTest }) => { expect(result.outputLines).toEqual(['setup 2 in setup']); }); -test('should report skipped dependent tests', async ({ runInlineTest }) => { +test('should not report skipped dependent tests', async ({ runInlineTest }) => { const result = await runInlineTest({ 'playwright.config.ts': ` module.exports = { projects: [ @@ -318,8 +318,8 @@ test('should report skipped dependent tests', async ({ runInlineTest }) => { }); expect(result.exitCode).toBe(1); expect(result.passed).toBe(0); - expect(result.skipped).toBe(1); - expect(result.results.length).toBe(2); + expect(result.didNotRun).toBe(1); + expect(result.results.length).toBe(1); }); test('should report circular dependencies', async ({ runInlineTest }) => { @@ -499,7 +499,7 @@ test('should run teardown after failure', async ({ runInlineTest }) => { expect(result.exitCode).toBe(1); expect(result.passed).toBe(1); expect(result.failed).toBe(1); - expect(result.skipped).toBe(2); + expect(result.didNotRun).toBe(2); expect(result.outputLines).toEqual(['A', 'D']); }); diff --git a/tests/playwright-test/hooks.spec.ts b/tests/playwright-test/hooks.spec.ts index 012c72032b..dcc497f6dc 100644 --- a/tests/playwright-test/hooks.spec.ts +++ b/tests/playwright-test/hooks.spec.ts @@ -370,7 +370,7 @@ test('max-failures should still run afterEach/afterAll', async ({ runInlineTest expect(result.exitCode).toBe(1); expect(result.passed).toBe(0); expect(result.failed).toBe(1); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.outputLines).toEqual([ 'test', 'afterEach', diff --git a/tests/playwright-test/max-failures.spec.ts b/tests/playwright-test/max-failures.spec.ts index f61c8f3fd3..c3bf4af834 100644 --- a/tests/playwright-test/max-failures.spec.ts +++ b/tests/playwright-test/max-failures.spec.ts @@ -38,8 +38,6 @@ test('max-failures should work', async ({ runInlineTest }) => { expect(result.exitCode).toBe(1); expect(result.failed).toBe(8); expect(result.output.split('\n').filter(l => l.includes('expect(')).length).toBe(16); - expect(result.report.suites[0].specs.map(spec => spec.tests[0].results.length)).toEqual(new Array(10).fill(1)); - expect(result.report.suites[1].specs.map(spec => spec.tests[0].results.length)).toEqual(new Array(10).fill(1)); }); test('-x should work', async ({ runInlineTest }) => { @@ -111,7 +109,7 @@ test('max-failures should stop workers', async ({ runInlineTest }) => { expect(result.passed).toBe(2); expect(result.failed).toBe(1); expect(result.interrupted).toBe(1); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.output).toContain('%%interrupted'); expect(result.output).not.toContain('%%skipped'); }); @@ -177,7 +175,7 @@ test('max-failures should work across phases', async ({ runInlineTest }) => { expect(result.exitCode).toBe(1); expect(result.failed).toBe(1); expect(result.passed).toBe(2); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.output).toContain('running a'); expect(result.output).toContain('running b'); expect(result.output).toContain('running c'); diff --git a/tests/playwright-test/playwright-test-fixtures.ts b/tests/playwright-test/playwright-test-fixtures.ts index de89c7c6cc..2badc4f3fb 100644 --- a/tests/playwright-test/playwright-test-fixtures.ts +++ b/tests/playwright-test/playwright-test-fixtures.ts @@ -43,6 +43,7 @@ export type RunResult = { flaky: number, skipped: number, interrupted: number, + didNotRun: number, report: JSONReport, results: any[], }; @@ -417,6 +418,7 @@ export function parseTestRunnerOutput(output: string) { const flaky = summary(/(\d+) flaky/g); const skipped = summary(/(\d+) skipped/g); const interrupted = summary(/(\d+) interrupted/g); + const didNotRun = summary(/(\d+) did not run/g); const strippedOutput = stripAnsi(output); return { @@ -428,5 +430,6 @@ export function parseTestRunnerOutput(output: string) { flaky, skipped, interrupted, + didNotRun, }; } diff --git a/tests/playwright-test/reporter-base.spec.ts b/tests/playwright-test/reporter-base.spec.ts index 7b0c5a40a4..8c8a2bc67d 100644 --- a/tests/playwright-test/reporter-base.spec.ts +++ b/tests/playwright-test/reporter-base.spec.ts @@ -205,7 +205,7 @@ for (const useIntermediateMergeReport of [false, true] as const) { expect(result.exitCode).toBe(1); expect(result.failed).toBe(1); expect(result.passed).toBe(0); - expect(result.skipped).toBe(2); + expect(result.didNotRun).toBe(2); expect(result.output).toContain('Testing stopped early after 1 maximum allowed failures.'); }); diff --git a/tests/playwright-test/runner.spec.ts b/tests/playwright-test/runner.spec.ts index 72a401a1cc..e3156c7cff 100644 --- a/tests/playwright-test/runner.spec.ts +++ b/tests/playwright-test/runner.spec.ts @@ -150,7 +150,7 @@ test('should ignore subprocess creation error because of SIGINT', async ({ inter const result = parseTestRunnerOutput(testProcess.output); expect(result.passed).toBe(0); expect(result.failed).toBe(0); - expect(result.skipped).toBe(2); + expect(result.didNotRun).toBe(2); expect(result.output).not.toContain('worker process exited unexpectedly'); }); @@ -190,7 +190,7 @@ test('sigint should stop workers', async ({ interactWithTestRunner }) => { const result = parseTestRunnerOutput(testProcess.output); expect(result.passed).toBe(0); expect(result.failed).toBe(0); - expect(result.skipped).toBe(2); + expect(result.didNotRun).toBe(2); expect(result.interrupted).toBe(2); expect(result.output).toContain('%%SEND-SIGINT%%1'); expect(result.output).toContain('%%SEND-SIGINT%%2'); @@ -206,7 +206,7 @@ test('sigint should stop workers', async ({ interactWithTestRunner }) => { const skipped2 = report.suites[1].specs[1]; expect(skipped2.title).toBe('skipped2'); - expect(skipped2.tests[0].results[0].workerIndex).toBe(-1); + expect(skipped2.tests[0].results).toHaveLength(0); }); test('should use the first occurring error when an unhandled exception was thrown', async ({ runInlineTest }) => {