From 210168e36db026448f419a53e86865335f1332be Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 24 Oct 2023 09:31:30 -0700 Subject: [PATCH] chore(test runner): remove fake skipped test results (#27762) Fixes #27455. --- packages/playwright/src/common/test.ts | 15 ++-- .../playwright/src/isomorphic/teleReceiver.ts | 12 +-- packages/playwright/src/runner/dispatcher.ts | 76 +++++++++---------- .../playwright/src/runner/failureTracker.ts | 2 +- tests/playwright-test/hooks.spec.ts | 8 +- tests/playwright-test/retry.spec.ts | 4 +- tests/playwright-test/runner.spec.ts | 4 +- tests/playwright-test/test-modifiers.spec.ts | 2 +- tests/playwright-test/test-serial.spec.ts | 60 ++++++++++++++- 9 files changed, 111 insertions(+), 72 deletions(-) diff --git a/packages/playwright/src/common/test.ts b/packages/playwright/src/common/test.ts index a1afee5b089a0..030ee77f428d0 100644 --- a/packages/playwright/src/common/test.ts +++ b/packages/playwright/src/common/test.ts @@ -239,6 +239,9 @@ export class TestCase extends Base implements reporterTypes.TestCase { _poolDigest = ''; _workerHash = ''; _projectId = ''; + // This is different from |results.length| because sometimes we do not run the test, but consume + // an attempt, for example when skipping tests in a serial suite after a failure. + _runAttempts = 0; // Annotations known statically before running the test, e.g. `test.skip()` or `test.describe.skip()`. _staticAnnotations: Annotation[] = []; @@ -256,16 +259,10 @@ export class TestCase extends Base implements reporterTypes.TestCase { } outcome(): 'skipped' | 'expected' | 'unexpected' | 'flaky' { - // Ignore initial skips that may be a result of "skipped because previous test in serial mode failed". - const results = [...this.results]; - while (results[0]?.status === 'skipped' || results[0]?.status === 'interrupted') - results.shift(); - - // All runs were skipped. - if (!results.length) + const results = this.results.filter(result => result.status !== 'interrupted'); + if (results.every(result => result.status === 'skipped')) return 'skipped'; - - const failures = results.filter(result => result.status !== 'skipped' && result.status !== 'interrupted' && result.status !== this.expectedStatus); + const failures = results.filter(result => result.status !== this.expectedStatus); if (!failures.length) // all passed return 'expected'; if (failures.length === results.length) // all failed diff --git a/packages/playwright/src/isomorphic/teleReceiver.ts b/packages/playwright/src/isomorphic/teleReceiver.ts index c7dfb2e605c2f..c2ff531fbfec6 100644 --- a/packages/playwright/src/isomorphic/teleReceiver.ts +++ b/packages/playwright/src/isomorphic/teleReceiver.ts @@ -493,16 +493,10 @@ export class TeleTestCase implements reporterTypes.TestCase { } outcome(): 'skipped' | 'expected' | 'unexpected' | 'flaky' { - // Ignore initial skips that may be a result of "skipped because previous test in serial mode failed". - const results = [...this.results]; - while (results[0]?.status === 'skipped' || results[0]?.status === 'interrupted') - results.shift(); - - // All runs were skipped. - if (!results.length) + const results = this.results.filter(result => result.status !== 'interrupted'); + if (results.every(result => result.status === 'skipped')) return 'skipped'; - - const failures = results.filter(result => result.status !== 'skipped' && result.status !== 'interrupted' && result.status !== this.expectedStatus); + const failures = results.filter(result => result.status !== this.expectedStatus); if (!failures.length) // all passed return 'expected'; if (failures.length === results.length) // all failed diff --git a/packages/playwright/src/runner/dispatcher.ts b/packages/playwright/src/runner/dispatcher.ts index 6fb315114e6fc..2cf66f44cc1a5 100644 --- a/packages/playwright/src/runner/dispatcher.ts +++ b/packages/playwright/src/runner/dispatcher.ts @@ -290,7 +290,7 @@ class JobDispatcher { test.expectedStatus = params.expectedStatus; test.annotations = params.annotations; test.timeout = params.timeout; - const isFailure = result.status !== 'skipped' && result.status !== test.expectedStatus; + const isFailure = result.status !== test.expectedStatus; if (isFailure) this._failedTests.add(test); this._reportTestEnd(test, result); @@ -369,22 +369,17 @@ class JobDispatcher { } result.errors = [...errors]; result.error = result.errors[0]; - result.status = errors.length ? 'failed' : 'skipped'; + result.status = 'failed'; this._reportTestEnd(test, result); this._failedTests.add(test); } - private _massSkipTestsFromRemaining(testIds: Set, errors: TestError[]) { - for (const test of this._remainingByTestId.values()) { - if (!testIds.has(test.id)) - continue; - if (!this._failureTracker.hasReachedMaxFailures()) { - this._failTestWithErrors(test, errors); - errors = []; // Only report errors for the first test. - } + private _handleFatalErrors(errors: TestError[]) { + const test = this._remainingByTestId.values().next().value as TestCase | undefined; + if (test) { + this._failTestWithErrors(test, errors); this._remainingByTestId.delete(test.id); - } - if (errors.length) { + } else if (errors.length) { // We had fatal errors after all tests have passed - most likely in some teardown. // Let's just fail the test run. this._failureTracker.onWorkerError(); @@ -412,23 +407,28 @@ class JobDispatcher { } if (params.fatalErrors.length) { - // In case of fatal errors, report first remaining test as failing with these errors, - // and all others as skipped. - this._massSkipTestsFromRemaining(new Set(this._remainingByTestId.keys()), params.fatalErrors); + // In case of fatal errors, report the first remaining test as failing with these errors, + // and "skip" all other tests to avoid running into the same issue over and over. + this._handleFatalErrors(params.fatalErrors); + this._remainingByTestId.clear(); } + // Handle tests that should be skipped because of the setup failure. - this._massSkipTestsFromRemaining(new Set(params.skipTestsDueToSetupFailure), []); + for (const testId of params.skipTestsDueToSetupFailure) + this._remainingByTestId.delete(testId); if (params.unexpectedExitError) { - // When worker exits during a test, we blame the test itself. - // - // The most common situation when worker exits while not running a test is: - // worker failed to require the test file (at the start) because of an exception in one of imports. - // In this case, "skip" all remaining tests, to avoid running into the same exception over and over. - if (this._currentlyRunning) - this._massSkipTestsFromRemaining(new Set([this._currentlyRunning.test.id]), [params.unexpectedExitError]); - else - this._massSkipTestsFromRemaining(new Set(this._remainingByTestId.keys()), [params.unexpectedExitError]); + if (this._currentlyRunning) { + // When worker exits during a test, we blame the test itself. + this._failTestWithErrors(this._currentlyRunning.test, [params.unexpectedExitError]); + this._remainingByTestId.delete(this._currentlyRunning.test.id); + } else { + // The most common situation when worker exits while not running a test is: + // worker failed to require the test file (at the start) because of an exception in one of imports. + // In this case, "skip" all remaining tests, to avoid running into the same exception over and over. + this._handleFatalErrors([params.unexpectedExitError]); + this._remainingByTestId.clear(); + } } const retryCandidates = new Set(); @@ -446,26 +446,22 @@ class JobDispatcher { serialSuitesWithFailures.add(outermostSerialSuite); } - // If we have failed tests that belong to a serial suite, - // we should skip all future tests from the same serial suite. - const testsBelongingToSomeSerialSuiteWithFailures = [...this._remainingByTestId.values()].filter(test => { - let parent: Suite | undefined = test.parent; - while (parent && !serialSuitesWithFailures.has(parent)) - parent = parent.parent; - return !!parent; - }); - this._massSkipTestsFromRemaining(new Set(testsBelongingToSomeSerialSuiteWithFailures.map(test => test.id)), []); - for (const serialSuite of serialSuitesWithFailures) { - // Add all tests from failed serial suites for possible retry. - // These will only be retried together, because they have the same - // "retries" setting and the same number of previous runs. - serialSuite.allTests().forEach(test => retryCandidates.add(test)); + serialSuite.allTests().forEach(test => { + // Skip remaining tests from serial suites with failures. + this._remainingByTestId.delete(test.id); + // Schedule them for the retry all together. + retryCandidates.add(test); + }); } + for (const test of this._job.tests) { + if (!this._remainingByTestId.has(test.id)) + ++test._runAttempts; + } const remaining = [...this._remainingByTestId.values()]; for (const test of retryCandidates) { - if (test.results.length < test.retries + 1) + if (test._runAttempts < test.retries + 1) remaining.push(test); } diff --git a/packages/playwright/src/runner/failureTracker.ts b/packages/playwright/src/runner/failureTracker.ts index fc2202b3ead9b..2835403387807 100644 --- a/packages/playwright/src/runner/failureTracker.ts +++ b/packages/playwright/src/runner/failureTracker.ts @@ -31,7 +31,7 @@ export class FailureTracker { } onTestEnd(test: TestCase, result: TestResult) { - if (result.status !== 'skipped' && result.status !== test.expectedStatus) + if (result.status !== test.expectedStatus) ++this._failureCount; } diff --git a/tests/playwright-test/hooks.spec.ts b/tests/playwright-test/hooks.spec.ts index dcc497f6dcfd2..59725ee0f5f4c 100644 --- a/tests/playwright-test/hooks.spec.ts +++ b/tests/playwright-test/hooks.spec.ts @@ -399,7 +399,7 @@ test('beforeAll failure should prevent the test, but not afterAll', async ({ run }); expect(result.exitCode).toBe(1); expect(result.failed).toBe(1); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.outputLines).toEqual([ 'beforeAll', 'afterAll', @@ -499,7 +499,7 @@ test('beforeAll timeout should be reported and prevent more tests', async ({ run }, { timeout: 1000 }); expect(result.exitCode).toBe(1); expect(result.failed).toBe(1); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.outputLines).toEqual([ 'beforeAll', 'afterAll', @@ -688,7 +688,7 @@ test('unhandled rejection during beforeAll should be reported and prevent more t }); expect(result.exitCode).toBe(1); expect(result.failed).toBe(1); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.outputLines).toEqual([ 'beforeAll', 'afterAll', @@ -801,7 +801,7 @@ test('beforeAll failure should only prevent tests that are affected', async ({ r }); expect(result.exitCode).toBe(1); expect(result.failed).toBe(1); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.passed).toBe(1); expect(result.outputLines).toEqual([ 'beforeAll', diff --git a/tests/playwright-test/retry.spec.ts b/tests/playwright-test/retry.spec.ts index d6cad8a0801c8..93f52010bd966 100644 --- a/tests/playwright-test/retry.spec.ts +++ b/tests/playwright-test/retry.spec.ts @@ -216,8 +216,8 @@ test('should retry beforeAll failure', async ({ runInlineTest }) => { expect(result.exitCode).toBe(1); expect(result.passed).toBe(0); expect(result.failed).toBe(1); - expect(result.skipped).toBe(1); - expect(result.output.split('\n')[2]).toBe('×°×°F°'); + expect(result.didNotRun).toBe(1); + expect(result.output.split('\n')[2]).toBe('××F'); expect(result.output).toContain('BeforeAll is bugged!'); }); diff --git a/tests/playwright-test/runner.spec.ts b/tests/playwright-test/runner.spec.ts index ff741dc5facb0..ceb03251c738c 100644 --- a/tests/playwright-test/runner.spec.ts +++ b/tests/playwright-test/runner.spec.ts @@ -111,7 +111,7 @@ test('should report subprocess creation error', async ({ runInlineTest }, testIn 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.output).toContain('Error: worker process exited unexpectedly (code=42, signal=null)'); }); @@ -634,9 +634,9 @@ test('should not hang on worker error in test file', async ({ runInlineTest }) = `, }, { 'timeout': 3000 }); expect(result.exitCode).toBe(1); + expect(result.results).toHaveLength(1); expect(result.results[0].status).toBe('failed'); expect(result.results[0].error.message).toContain('Error: worker process exited unexpectedly'); - expect(result.results[1].status).toBe('skipped'); }); test('fast double SIGINT should be ignored', async ({ interactWithTestRunner }) => { diff --git a/tests/playwright-test/test-modifiers.spec.ts b/tests/playwright-test/test-modifiers.spec.ts index ce9a1aff9a98c..ed3abc5e12255 100644 --- a/tests/playwright-test/test-modifiers.spec.ts +++ b/tests/playwright-test/test-modifiers.spec.ts @@ -640,7 +640,7 @@ test('static modifiers should be added in serial mode', async ({ runInlineTest } }); expect(result.exitCode).toBe(1); expect(result.passed).toBe(0); - expect(result.skipped).toBe(3); + expect(result.didNotRun).toBe(3); expect(result.report.suites[0].specs[0].tests[0].annotations).toEqual([{ type: 'slow' }]); expect(result.report.suites[0].specs[1].tests[0].annotations).toEqual([{ type: 'fixme' }]); expect(result.report.suites[0].specs[2].tests[0].annotations).toEqual([{ type: 'skip' }]); diff --git a/tests/playwright-test/test-serial.spec.ts b/tests/playwright-test/test-serial.spec.ts index 085d175097028..5059b8108b0c0 100644 --- a/tests/playwright-test/test-serial.spec.ts +++ b/tests/playwright-test/test-serial.spec.ts @@ -47,7 +47,7 @@ test('test.describe.serial should work', async ({ runInlineTest }) => { expect(result.exitCode).toBe(1); expect(result.passed).toBe(2); expect(result.failed).toBe(1); - expect(result.skipped).toBe(2); + expect(result.didNotRun).toBe(2); expect(result.outputLines).toEqual([ 'test1', 'test2', @@ -87,7 +87,7 @@ test('test.describe.serial should work in describe', async ({ runInlineTest }) = expect(result.exitCode).toBe(1); expect(result.passed).toBe(2); expect(result.failed).toBe(1); - expect(result.skipped).toBe(2); + expect(result.didNotRun).toBe(2); expect(result.outputLines).toEqual([ 'test1', 'test2', @@ -128,7 +128,7 @@ test('test.describe.serial should work with retry', async ({ runInlineTest }) => expect(result.passed).toBe(2); expect(result.flaky).toBe(1); expect(result.failed).toBe(1); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.outputLines).toEqual([ 'test1', 'test2', @@ -272,7 +272,7 @@ test('test.describe.serial should work with test.fail', async ({ runInlineTest } expect(result.exitCode).toBe(1); expect(result.passed).toBe(2); expect(result.failed).toBe(1); - expect(result.skipped).toBe(1); + expect(result.didNotRun).toBe(1); expect(result.outputLines).toEqual([ 'zero', 'one', @@ -394,3 +394,55 @@ test('test.describe.serial should work with fullyParallel', async ({ runInlineTe 'two', ]); }); + +test('serial fail + skip is failed', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test.describe.configure({ mode: 'serial', retries: 1 }); + test.describe.serial('serial suite', () => { + test('one', async () => { + expect(test.info().retry).toBe(0); + }); + test('two', async () => { + expect(1).toBe(2); + }); + test('three', async () => { + }); + }); + `, + }, { workers: 1 }); + expect(result.exitCode).toBe(1); + expect(result.passed).toBe(0); + expect(result.skipped).toBe(0); + expect(result.flaky).toBe(1); + expect(result.failed).toBe(1); + expect(result.interrupted).toBe(0); + expect(result.didNotRun).toBe(1); +}); + +test('serial skip + fail is failed', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test.describe.configure({ mode: 'serial', retries: 1 }); + test.describe.serial('serial suite', () => { + test('one', async () => { + expect(test.info().retry).toBe(1); + }); + test('two', async () => { + expect(1).toBe(2); + }); + test('three', async () => { + }); + }); + `, + }, { workers: 1 }); + expect(result.exitCode).toBe(1); + expect(result.passed).toBe(0); + expect(result.skipped).toBe(0); + expect(result.flaky).toBe(1); + expect(result.failed).toBe(1); + expect(result.interrupted).toBe(0); + expect(result.didNotRun).toBe(1); +});