fix(fixtures): tear down base fixture after error in derived (#29337)

Fixes https://github.com/microsoft/playwright/issues/29325

packages/playwright/src/worker/fixtureRunner.ts

@@ -163,17 +163,10 @@ class Fixture {
     await this._teardownWithDepsComplete;
   }
 
-  private _setTeardownDescription(timeoutManager: TimeoutManager) {
-    this._runnableDescription.phase = 'teardown';
-    timeoutManager.setCurrentFixture(this._runnableDescription);
-  }
-
   private async _teardownInternal(timeoutManager: TimeoutManager) {
     if (typeof this.registration.fn !== 'function')
       return;
     try {
-      for (const fixture of this._usages)
-        await fixture.teardown(timeoutManager);
       if (this._usages.size !== 0) {
         // TODO: replace with assert.
         console.error('Internal error: fixture integrity at', this._runnableDescription.title);  // eslint-disable-line no-console
@@ -192,6 +185,19 @@ class Fixture {
       this.runner.instanceForId.delete(this.registration.id);
     }
   }
+
+  private _setTeardownDescription(timeoutManager: TimeoutManager) {
+    this._runnableDescription.phase = 'teardown';
+    timeoutManager.setCurrentFixture(this._runnableDescription);
+  }
+
+  _collectFixturesInTeardownOrder(scope: FixtureScope, collector: Set<Fixture>) {
+    if (this.registration.scope !== scope)
+      return;
+    for (const fixture of this._usages)
+      fixture._collectFixturesInTeardownOrder(scope, collector);
+    collector.add(this);
+  }
 }
 
 export class FixtureRunner {
@@ -213,24 +219,16 @@ export class FixtureRunner {
     this.pool = pool;
   }
 
-  async teardownScope(scope: FixtureScope, timeoutManager: TimeoutManager) {
+  async teardownScope(scope: FixtureScope, timeoutManager: TimeoutManager, onFixtureError: (error: Error) => void) {
-    let error: Error | undefined;
     // Teardown fixtures in the reverse order.
     const fixtures = Array.from(this.instanceForId.values()).reverse();
-    for (const fixture of fixtures) {
+    const collector = new Set<Fixture>();
-      if (fixture.registration.scope === scope) {
+    for (const fixture of fixtures)
-        try {
+      fixture._collectFixturesInTeardownOrder(scope, collector);
-          await fixture.teardown(timeoutManager);
+    for (const fixture of collector)
-        } catch (e) {
+      await fixture.teardown(timeoutManager).catch(onFixtureError);
-          if (error === undefined)
-            error = e;
-        }
-      }
-    }
     if (scope === 'test')
       this.testScopeClean = true;
-    if (error !== undefined)
-      throw error;
   }
 
   async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise<object | null> {

packages/playwright/src/worker/workerMain.ts

@@ -31,6 +31,7 @@ import { applyRepeatEachIndex, bindFileSuiteToProject, filterTestsRemoveEmptySui
 import { PoolBuilder } from '../common/poolBuilder';
 import type { TestInfoError } from '../../types/test';
 import type { Location } from '../../types/testReporter';
+import type { FixtureScope } from '../common/fixtures';
 import { inheritFixutreNames } from '../common/fixtures';
 
 export class WorkerMain extends ProcessRunner {
@@ -144,13 +145,29 @@ export class WorkerMain extends ProcessRunner {
     }
   }
 
+  private async _teardownScope(scope: FixtureScope, testInfo: TestInfoImpl) {
+    const error = await this._teardownScopeAndReturnFirstError(scope, testInfo);
+    if (error)
+      throw error;
+  }
+
+  private async _teardownScopeAndReturnFirstError(scope: FixtureScope, testInfo: TestInfoImpl): Promise<Error | undefined> {
+    let error: Error | undefined;
+    await this._fixtureRunner.teardownScope(scope, testInfo._timeoutManager, e => {
+      testInfo._failWithError(e, true, false);
+      if (error === undefined)
+        error = e;
+    });
+    return error;
+  }
+
   private async _teardownScopes() {
     // TODO: separate timeout for teardown?
     const timeoutManager = new TimeoutManager(this._project.project.timeout);
     await timeoutManager.withRunnable({ type: 'teardown' }, async () => {
       const timeoutError = await timeoutManager.runWithTimeout(async () => {
-        await this._fixtureRunner.teardownScope('test', timeoutManager);
+        await this._fixtureRunner.teardownScope('test', timeoutManager, e => this._fatalErrors.push(serializeError(e)));
-        await this._fixtureRunner.teardownScope('worker', timeoutManager);
+        await this._fixtureRunner.teardownScope('worker', timeoutManager, e => this._fatalErrors.push(serializeError(e)));
       });
       if (timeoutError)
         this._fatalErrors.push(timeoutError);
@@ -421,9 +438,7 @@ export class WorkerMain extends ProcessRunner {
         // Teardown test-scoped fixtures. Attribute to 'test' so that users understand
         // they should probably increase the test timeout to fix this issue.
         debugTest(`tearing down test scope started`);
-        const testScopeError = await testInfo._runAndFailOnError(() => {
+        const testScopeError = await this._teardownScopeAndReturnFirstError('test', testInfo);
-          return this._fixtureRunner.teardownScope('test', testInfo._timeoutManager);
-        });
         debugTest(`tearing down test scope finished`);
         firstAfterHooksError = firstAfterHooksError || testScopeError;
 
@@ -454,9 +469,7 @@ export class WorkerMain extends ProcessRunner {
           await testInfo._timeoutManager.withRunnable({ type: 'teardown', slot: teardownSlot }, async () => {
             // Attribute to 'test' so that users understand they should probably increate the test timeout to fix this issue.
             debugTest(`tearing down test scope started`);
-            const testScopeError = await testInfo._runAndFailOnError(() => {
+            const testScopeError = await this._teardownScopeAndReturnFirstError('test', testInfo);
-              return this._fixtureRunner.teardownScope('test', testInfo._timeoutManager);
-            });
             debugTest(`tearing down test scope finished`);
             firstAfterHooksError = firstAfterHooksError || testScopeError;
 
@@ -467,9 +480,7 @@ export class WorkerMain extends ProcessRunner {
 
             // Attribute to 'teardown' because worker fixtures are not perceived as a part of a test.
             debugTest(`tearing down worker scope started`);
-            const workerScopeError = await testInfo._runAndFailOnError(() => {
+            const workerScopeError = await this._teardownScopeAndReturnFirstError('worker', testInfo);
-              return this._fixtureRunner.teardownScope('worker', testInfo._timeoutManager);
-            });
             debugTest(`tearing down worker scope finished`);
             firstAfterHooksError = firstAfterHooksError || workerScopeError;
           });
@@ -552,7 +563,7 @@ export class WorkerMain extends ProcessRunner {
             }
             // Each beforeAll/afterAll hook has its own scope for test fixtures. Attribute to the same runnable and timeSlot.
             // Note: we must teardown even after hook fails, because we'll run more hooks.
-            await this._fixtureRunner.teardownScope('test', testInfo._timeoutManager);
+            await this._teardownScope('test', testInfo);
           }
         });
       }, allowSkips ? 'allowSkips' : undefined);

tests/playwright-test/fixture-errors.spec.ts

@@ -647,3 +647,32 @@ test('should provide helpful error message when digests do not match', async ({
   expect(result.failed).toBe(1);
   expect(result.output).toContain('Playwright detected inconsistent test.use() options.');
 });
+
+test('tear down base fixture after error in derived', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      import { test as base, expect } from '@playwright/test';
+      const test = base.extend({
+        context: async ({}, use, testInfo) => {
+          console.log('\\n%%context setup ' + testInfo.status);
+          await use();
+          console.log('\\n%%context teardown ' + testInfo.status);
+        },
+        page: async ({ context }, use, testInfo) => {
+          console.log('\\n%%page setup ' + testInfo.status);
+          await use();
+          console.log('\\n%%page teardown ' + testInfo.status);
+          throw new Error('Error in page teardown');
+        },
+      });
+      test('test', async ({ page }) => {});
+    `
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.outputLines).toEqual([
+    'context setup passed',
+    'page setup passed',
+    'page teardown passed',
+    'context teardown failed',
+  ]);
+});