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

@@ -54,6 +54,7 @@ class Fixture {
   runner: FixtureRunner;
   registration: FixtureRegistration;
   value: any;
+  failed = false;
 
   _useFuncFinished: ManualPromise<void> | undefined;
   _selfTeardownComplete: Promise<void> | 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<object> {
+  async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise<object | null> {
     // 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)
+      if (shouldRun) {
-        await this.setupFixtureForRegistration(registration, testInfo);
+        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);
   }
 

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]").

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',
+  ]);
+});