diff --git a/packages/playwright-test/src/dispatcher.ts b/packages/playwright-test/src/dispatcher.ts index 0c139a6db7..a6c88f90ab 100644 --- a/packages/playwright-test/src/dispatcher.ts +++ b/packages/playwright-test/src/dispatcher.ts @@ -18,7 +18,7 @@ import child_process from 'child_process'; import path from 'path'; import { EventEmitter } from 'events'; import { RunPayload, TestBeginPayload, TestEndPayload, DonePayload, TestOutputPayload, WorkerInitParams, StepBeginPayload, StepEndPayload, SerializedLoaderData, TeardownErrorsPayload } from './ipc'; -import type { TestResult, Reporter, TestStep } from '../types/testReporter'; +import type { TestResult, Reporter, TestStep, TestError } from '../types/testReporter'; import { Suite, TestCase } from './test'; import { Loader } from './loader'; import { ManualPromise } from 'playwright-core/lib/utils/async'; @@ -278,7 +278,7 @@ export class Dispatcher { // - there are no remaining // - we are here not because something failed // - no unrecoverable worker error - if (!remaining.length && !failedTestIds.size && !params.fatalErrors.length && !params.skipRemaining) { + if (!remaining.length && !failedTestIds.size && !params.fatalErrors.length && !params.skipTestsDueToSetupFailure.length) { if (this._isWorkerRedundant(worker)) worker.stop(); doneWithJob(); @@ -288,41 +288,46 @@ export class Dispatcher { // When worker encounters error, we will stop it and create a new one. worker.stop(true /* didFail */); - // In case of fatal error, report first remaining test as failing with this error, - // and all others as skipped. - if (params.fatalErrors.length || params.skipRemaining) { - let shouldAddFatalErrorsToNextTest = params.fatalErrors.length > 0; - for (const test of remaining) { - if (this._hasReachedMaxFailures()) - break; - const data = this._testById.get(test._id)!; - const runData = data.resultByWorkerIndex.get(worker.workerIndex); - // There might be a single test that has started but has not finished yet. - let result: TestResult; - if (runData) { - result = runData.result; - } else { - result = data.test._appendTestResult(); - this._reporter.onTestBegin?.(test, result); + const massSkipTestsFromRemaining = (testIds: Set, errors: TestError[]) => { + remaining = remaining.filter(test => { + if (!testIds.has(test._id)) + return true; + if (!this._hasReachedMaxFailures()) { + const data = this._testById.get(test._id)!; + const runData = data.resultByWorkerIndex.get(worker.workerIndex); + // There might be a single test that has started but has not finished yet. + let result: TestResult; + if (runData) { + result = runData.result; + } else { + result = data.test._appendTestResult(); + this._reporter.onTestBegin?.(test, result); + } + result.errors = [...errors]; + result.error = result.errors[0]; + result.status = errors.length ? 'failed' : 'skipped'; + this._reportTestEnd(test, result); + failedTestIds.add(test._id); + errors = []; // Only report errors for the first test. } - result.errors = shouldAddFatalErrorsToNextTest ? [...params.fatalErrors] : []; - result.error = result.errors[0]; - result.status = shouldAddFatalErrorsToNextTest ? 'failed' : 'skipped'; - this._reportTestEnd(test, result); - failedTestIds.add(test._id); - shouldAddFatalErrorsToNextTest = false; - } - if (shouldAddFatalErrorsToNextTest) { - // We had a fatal error after all tests have passed - most likely in the afterAll hook. + return false; + }); + 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._hasWorkerErrors = true; for (const error of params.fatalErrors) this._reporter.onError?.(error); } - // Since we pretend that all remaining tests failed/skipped, there is nothing else to run, - // except for possible retries. - remaining = []; + }; + + if (params.fatalErrors.length) { + // In case of fatal errors, report first remaining test as failing with these errors, + // and all others as skipped. + massSkipTestsFromRemaining(new Set(remaining.map(test => test._id)), params.fatalErrors); } + // Handle tests that should be skipped because of the setup failure. + massSkipTestsFromRemaining(new Set(params.skipTestsDueToSetupFailure), []); const retryCandidates = new Set(); const serialSuitesWithFailures = new Set(); @@ -384,7 +389,7 @@ export class Dispatcher { worker.on('done', onDone); const onExit = (expectedly: boolean) => { - onDone({ skipRemaining: false, fatalErrors: expectedly ? [] : [{ value: 'Worker process exited unexpectedly' }] }); + onDone({ skipTestsDueToSetupFailure: [], fatalErrors: expectedly ? [] : [{ value: 'Worker process exited unexpectedly' }] }); }; worker.on('exit', onExit); diff --git a/packages/playwright-test/src/ipc.ts b/packages/playwright-test/src/ipc.ts index 91d49ab332..9a1f21e6d8 100644 --- a/packages/playwright-test/src/ipc.ts +++ b/packages/playwright-test/src/ipc.ts @@ -76,7 +76,7 @@ export type RunPayload = { export type DonePayload = { fatalErrors: TestError[]; - skipRemaining: boolean; + skipTestsDueToSetupFailure: string[]; // test ids }; export type TestOutputPayload = { diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerRunner.ts index 7f1b221bd9..bda76cbce3 100644 --- a/packages/playwright-test/src/workerRunner.ts +++ b/packages/playwright-test/src/workerRunner.ts @@ -39,9 +39,9 @@ export class WorkerRunner extends EventEmitter { // Accumulated fatal errors that cannot be attributed to a test. private _fatalErrors: TestError[] = []; - // Whether we should skip running remaining tests in the group because + // Whether we should skip running remaining tests in this suite because // of a setup error, usually beforeAll hook. - private _skipRemainingTests = false; + private _skipRemainingTestsInSuite: Suite | undefined; // The stage of the full cleanup. Once "finished", we can safely stop running anything. private _didRunFullCleanup = false; // Whether the worker was requested to stop. @@ -134,8 +134,8 @@ export class WorkerRunner extends EventEmitter { async runTestGroup(runPayload: RunPayload) { this._runFinished = new ManualPromise(); + const entries = new Map(runPayload.entries.map(e => [ e.testId, e ])); try { - const entries = new Map(runPayload.entries.map(e => [ e.testId, e ])); await this._loadIfNeeded(); const fileSuite = await this._loader.loadTestFile(runPayload.file, 'worker'); const suite = this._project.cloneFileSuite(fileSuite, this._params.repeatEachIndex, test => { @@ -148,8 +148,14 @@ export class WorkerRunner extends EventEmitter { this._activeSuites = new Set(); this._didRunFullCleanup = false; const tests = suite.allTests().filter(test => entries.has(test._id)); - for (let i = 0; i < tests.length; i++) - await this._runTest(tests[i], entries.get(tests[i]._id)!.retry, tests[i + 1]); + for (let i = 0; i < tests.length; i++) { + // Do not run tests after full cleanup, because we are entirely done. + if (this._isStopped && this._didRunFullCleanup) + break; + const entry = entries.get(tests[i]._id)!; + entries.delete(tests[i]._id); + await this._runTest(tests[i], entry.retry, tests[i + 1]); + } } } catch (e) { // In theory, we should run above code without any errors. @@ -157,16 +163,22 @@ export class WorkerRunner extends EventEmitter { // but not in the runner, let's do a fatal error. this.unhandledError(e); } finally { - this._reportDone(); + const donePayload: DonePayload = { + fatalErrors: this._fatalErrors, + skipTestsDueToSetupFailure: [], + }; + for (const test of this._skipRemainingTestsInSuite?.allTests() || []) { + if (entries.has(test._id)) + donePayload.skipTestsDueToSetupFailure.push(test._id); + } + this.emit('done', donePayload); + this._fatalErrors = []; + this._skipRemainingTestsInSuite = undefined; this._runFinished.resolve(); } } private async _runTest(test: TestCase, retry: number, nextTest: TestCase | undefined) { - // Do not run tests after full cleanup, because we are entirely done. - if (this._isStopped && this._didRunFullCleanup) - return; - let lastStepId = 0; const testInfo = new TestInfoImpl(this._loader, this._params, test, retry, data => { const stepId = `${data.category}@${data.title}@${++lastStepId}`; @@ -254,8 +266,7 @@ export class WorkerRunner extends EventEmitter { return; } - // Assume beforeAll failed until we actually finish it successfully. - let didFailBeforeAll = true; + let didFailBeforeAllForSuite: Suite | undefined; let shouldRunAfterEachHooks = false; await testInfo._runWithTimeout(async () => { @@ -263,7 +274,7 @@ export class WorkerRunner extends EventEmitter { // Getting here means that worker is requested to stop, but was not able to // run full cleanup yet. Skip the test, but run the cleanup. testInfo.status = 'skipped'; - didFailBeforeAll = false; + didFailBeforeAllForSuite = undefined; return; } @@ -283,14 +294,18 @@ export class WorkerRunner extends EventEmitter { continue; const extraAnnotations: Annotation[] = []; this._extraSuiteAnnotations.set(suite, extraAnnotations); + didFailBeforeAllForSuite = suite; // Assume failure, unless reset below. await this._runModifiersForSuite(suite, testInfo, 'worker', extraAnnotations); } // Run "beforeAll" hooks, unless already run during previous tests. - for (const suite of suites) + for (const suite of suites) { + didFailBeforeAllForSuite = suite; // Assume failure, unless reset below. await this._runBeforeAllHooksForSuite(suite, testInfo); - // Running "beforeAll" succeeded! - didFailBeforeAll = false; + } + + // Running "beforeAll" succeeded for all suites! + didFailBeforeAllForSuite = undefined; // Run "beforeEach" modifiers. for (const suite of suites) @@ -313,11 +328,11 @@ export class WorkerRunner extends EventEmitter { beforeHooksStep.complete(maybeError); // Second complete is a no-op. }); - if (didFailBeforeAll) { + if (didFailBeforeAllForSuite) { // This will inform dispatcher that we should not run more tests from this group // because we had a beforeAll error. // This behavior avoids getting the same common error for each test. - this._skipRemainingTests = true; + this._skipRemainingTestsInSuite = didFailBeforeAllForSuite; } const afterHooksStep = testInfo._addStep({ @@ -457,13 +472,6 @@ export class WorkerRunner extends EventEmitter { if (error) throw error; } - - private _reportDone() { - const donePayload: DonePayload = { fatalErrors: this._fatalErrors, skipRemaining: this._skipRemainingTests }; - this.emit('done', donePayload); - this._fatalErrors = []; - this._skipRemainingTests = false; - } } function buildTestBeginPayload(testInfo: TestInfoImpl): TestBeginPayload { diff --git a/tests/playwright-test/hooks.spec.ts b/tests/playwright-test/hooks.spec.ts index 9d0ecda089..56ae48cefe 100644 --- a/tests/playwright-test/hooks.spec.ts +++ b/tests/playwright-test/hooks.spec.ts @@ -752,3 +752,34 @@ test('test.setTimeout should work separately in afterAll', async ({ runInlineTes '%%afterAll', ]); }); + +test('beforeAll failure should only prevent tests that are affected', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.test.js': ` + const { test } = pwt; + test.describe('suite', () => { + test.beforeAll(async () => { + console.log('\\n%%beforeAll'); + throw new Error('oh my'); + }); + test('failed', () => { + console.log('\\n%%test1'); + }); + test('skipped', () => { + console.log('\\n%%test2'); + }); + }); + test('passed', () => { + console.log('\\n%%test3'); + }); + `, + }); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + expect(result.skipped).toBe(1); + expect(result.passed).toBe(1); + expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([ + '%%beforeAll', + '%%test3', + ]); +}); diff --git a/tests/playwright-test/playwright-test-fixtures.ts b/tests/playwright-test/playwright-test-fixtures.ts index e0c6dbad76..9bf60ddcac 100644 --- a/tests/playwright-test/playwright-test-fixtures.ts +++ b/tests/playwright-test/playwright-test-fixtures.ts @@ -284,10 +284,10 @@ export function createWhiteImage(width: number, height: number) { } export function paintBlackPixels(image: Buffer, blackPixelsCount: number): Buffer { - image = PNG.sync.read(image); + const png = PNG.sync.read(image); for (let i = 0; i < blackPixelsCount; ++i) { for (let j = 0; j < 3; ++j) - image.data[i * 4 + j] = 0; + png.data[i * 4 + j] = 0; } - return PNG.sync.write(image); + return PNG.sync.write(png); }