From ee82837fb5aac44478bd75cd0788731bcf61b214 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 5 Jul 2022 17:15:28 -0700 Subject: [PATCH] fix(fixtures): do not run user function when required fixture has failed (#15385) Currently, it is possible to run a function, e.g. a second `beforeEach` hook, that will receive `null` for the fixture that has already failed before. This PR skips running any user function that uses a fixture that has already failed, just like if the fixture would be initialized again and failing for the second time. --- packages/playwright-test/src/fixtures.ts | 21 ++++++-- packages/playwright-test/src/workerRunner.ts | 4 ++ tests/playwright-test/fixture-errors.spec.ts | 52 ++++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/packages/playwright-test/src/fixtures.ts b/packages/playwright-test/src/fixtures.ts index 83843c3071..ad34752633 100644 --- a/packages/playwright-test/src/fixtures.ts +++ b/packages/playwright-test/src/fixtures.ts @@ -54,6 +54,7 @@ class Fixture { runner: FixtureRunner; registration: FixtureRegistration; value: any; + failed = false; _useFuncFinished: ManualPromise | undefined; _selfTeardownComplete: Promise | undefined; @@ -94,6 +95,10 @@ class Fixture { // Otherwise worker-scope fixtures will retain test-scope fixtures forever. this._deps.add(dep); params[name] = dep.value; + if (dep.failed) { + this.failed = true; + return; + } } let called = false; @@ -112,6 +117,7 @@ class Fixture { const info = this.registration.scope === 'worker' ? workerInfo : testInfo; testInfo._timeoutManager.setCurrentFixture(this._runnableDescription); this._selfTeardownComplete = Promise.resolve().then(() => this.registration.fn(params, useFunc, info)).catch((e: any) => { + this.failed = true; if (!useFuncStarted.isDone()) useFuncStarted.reject(e); else @@ -307,7 +313,7 @@ export class FixtureRunner { throw error; } - async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise { + async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise { // Install automatic fixtures. for (const registration of this.pool!.registrations.values()) { if (registration.auto === false) @@ -317,8 +323,11 @@ export class FixtureRunner { shouldRun = registration.scope === 'worker' || registration.auto === 'all-hooks-included'; else if (autoFixtures === 'worker') shouldRun = registration.scope === 'worker'; - if (shouldRun) - await this.setupFixtureForRegistration(registration, testInfo); + if (shouldRun) { + const fixture = await this.setupFixtureForRegistration(registration, testInfo); + if (fixture.failed) + return null; + } } // Install used fixtures. @@ -327,6 +336,8 @@ export class FixtureRunner { for (const name of names) { const registration = this.pool!.registrations.get(name)!; const fixture = await this.setupFixtureForRegistration(registration, testInfo); + if (fixture.failed) + return null; params[name] = fixture.value; } return params; @@ -334,6 +345,10 @@ export class FixtureRunner { async resolveParametersAndRunFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only') { const params = await this.resolveParametersForFunction(fn, testInfo, autoFixtures); + if (params === null) { + // Do not run the function when fixture setup has already failed. + return null; + } return fn(params, testInfo); } diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerRunner.ts index f59d29b218..1aca6598aa 100644 --- a/packages/playwright-test/src/workerRunner.ts +++ b/packages/playwright-test/src/workerRunner.ts @@ -351,6 +351,10 @@ export class WorkerRunner extends EventEmitter { testInfo._timeoutManager.setCurrentRunnable({ type: 'test' }); const params = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo, 'test'); beforeHooksStep.complete({}); // Report fixture hooks step as completed. + if (params === null) { + // Fixture setup failed, we should not run the test now. + return; + } // Now run the test itself. const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]"). diff --git a/tests/playwright-test/fixture-errors.spec.ts b/tests/playwright-test/fixture-errors.spec.ts index fd9b39bb03..661deede08 100644 --- a/tests/playwright-test/fixture-errors.spec.ts +++ b/tests/playwright-test/fixture-errors.spec.ts @@ -532,3 +532,55 @@ test('should report worker fixture teardown with debug info', async ({ runInline 'Worker teardown timeout of 1000ms exceeded.', ].join('\n')); }); + +test('should not run user fn when require fixture has failed', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + const test = pwt.test.extend({ + foo: [ async ({ }, use) => { + console.log('\\n%%foo'); + throw new Error('A test error!'); + await use(); + }, { scope: 'test' } ], + bar: [ async ({ foo }, use) => { + console.log('\\n%%bar-' + foo); + await use(); + }, { scope: 'test' } ], + }); + + test.skip(({ foo }) => { + console.log('\\n%%skip-' + foo); + return true; + }); + + test.beforeEach(({ foo }) => { + console.log('\\n%%beforeEach1-' + foo); + }); + + test.beforeEach(({ foo }) => { + console.log('\\n%%beforeEach2-' + foo); + }); + + test.beforeEach(({ bar }) => { + console.log('\\n%%beforeEach3-' + bar); + }); + + test.afterEach(({ foo }) => { + console.log('\\n%%afterEach1-' + foo); + }); + + test.afterEach(({ bar }) => { + console.log('\\n%%afterEach2-' + bar); + }); + + test('should not run', async ({ bar }) => { + console.log('\\n%%test-' + bar); + }); + `, + }); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([ + '%%foo', + ]); +});