From 43798aff92eacfd0f6ab1090643e39acb6819774 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 27 Nov 2023 19:01:48 -0800 Subject: [PATCH] cherry-pick(#28360): Revert "chore(test runner): remove fake skipped test results (#27762)" (#28368) This reverts commit 210168e36db026448f419a53e86865335f1332be. Fixes #28321. --- 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, 72 insertions(+), 111 deletions(-) diff --git a/packages/playwright/src/common/test.ts b/packages/playwright/src/common/test.ts index 030ee77f42..a1afee5b08 100644 --- a/packages/playwright/src/common/test.ts +++ b/packages/playwright/src/common/test.ts @@ -239,9 +239,6 @@ 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[] = []; @@ -259,10 +256,16 @@ export class TestCase extends Base implements reporterTypes.TestCase { } outcome(): 'skipped' | 'expected' | 'unexpected' | 'flaky' { - const results = this.results.filter(result => result.status !== 'interrupted'); - if (results.every(result => result.status === 'skipped')) + // 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) return 'skipped'; - const failures = results.filter(result => result.status !== this.expectedStatus); + + const failures = results.filter(result => result.status !== 'skipped' && result.status !== 'interrupted' && 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 2850fcdc74..c5d82d810d 100644 --- a/packages/playwright/src/isomorphic/teleReceiver.ts +++ b/packages/playwright/src/isomorphic/teleReceiver.ts @@ -492,10 +492,16 @@ export class TeleTestCase implements reporterTypes.TestCase { } outcome(): 'skipped' | 'expected' | 'unexpected' | 'flaky' { - const results = this.results.filter(result => result.status !== 'interrupted'); - if (results.every(result => result.status === 'skipped')) + // 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) return 'skipped'; - const failures = results.filter(result => result.status !== this.expectedStatus); + + const failures = results.filter(result => result.status !== 'skipped' && result.status !== 'interrupted' && 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 2cf66f44cc..6fb315114e 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 !== test.expectedStatus; + const isFailure = result.status !== 'skipped' && result.status !== test.expectedStatus; if (isFailure) this._failedTests.add(test); this._reportTestEnd(test, result); @@ -369,17 +369,22 @@ class JobDispatcher { } result.errors = [...errors]; result.error = result.errors[0]; - result.status = 'failed'; + result.status = errors.length ? 'failed' : 'skipped'; this._reportTestEnd(test, result); this._failedTests.add(test); } - private _handleFatalErrors(errors: TestError[]) { - const test = this._remainingByTestId.values().next().value as TestCase | undefined; - if (test) { - this._failTestWithErrors(test, errors); + 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. + } this._remainingByTestId.delete(test.id); - } else if (errors.length) { + } + 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(); @@ -407,28 +412,23 @@ class JobDispatcher { } if (params.fatalErrors.length) { - // 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(); + // 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); } - // Handle tests that should be skipped because of the setup failure. - for (const testId of params.skipTestsDueToSetupFailure) - this._remainingByTestId.delete(testId); + this._massSkipTestsFromRemaining(new Set(params.skipTestsDueToSetupFailure), []); if (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(); - } + // 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]); } const retryCandidates = new Set(); @@ -446,22 +446,26 @@ 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) { - 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); - }); + // 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)); } - 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._runAttempts < test.retries + 1) + if (test.results.length < test.retries + 1) remaining.push(test); } diff --git a/packages/playwright/src/runner/failureTracker.ts b/packages/playwright/src/runner/failureTracker.ts index 2835403387..fc2202b3ea 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 !== test.expectedStatus) + if (result.status !== 'skipped' && result.status !== test.expectedStatus) ++this._failureCount; } diff --git a/tests/playwright-test/hooks.spec.ts b/tests/playwright-test/hooks.spec.ts index 59725ee0f5..dcc497f6dc 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.didNotRun).toBe(1); + expect(result.skipped).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.didNotRun).toBe(1); + expect(result.skipped).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.didNotRun).toBe(1); + expect(result.skipped).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.didNotRun).toBe(1); + expect(result.skipped).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 93f52010bd..d6cad8a080 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.didNotRun).toBe(1); - expect(result.output.split('\n')[2]).toBe('××F'); + expect(result.skipped).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 088a73ead1..51a8c9346e 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.didNotRun).toBe(1); + expect(result.skipped).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 ed3abc5e12..ce9a1aff9a 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.didNotRun).toBe(3); + expect(result.skipped).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 5059b8108b..085d175097 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.didNotRun).toBe(2); + expect(result.skipped).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.didNotRun).toBe(2); + expect(result.skipped).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.didNotRun).toBe(1); + expect(result.skipped).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.didNotRun).toBe(1); + expect(result.skipped).toBe(1); expect(result.outputLines).toEqual([ 'zero', 'one', @@ -394,55 +394,3 @@ 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); -});