feat: separate timeout for after hooks (#29828)

Instead of sharing the timeout with the test, and then extending it when
test times out, we give after hooks a separate timeout.

packages/playwright/src/worker/fixtureRunner.ts

@@ -30,7 +30,6 @@ class Fixture {
 
   private _useFuncFinished: ManualPromise<void> | undefined;
   private _selfTeardownComplete: Promise<void> | undefined;
-  private _teardownWithDepsComplete: Promise<void> | undefined;
   private _setupDescription: FixtureDescription;
   private _teardownDescription: FixtureDescription;
   private _shouldGenerateStep = false;
@@ -135,9 +134,7 @@ class Fixture {
       stepCategory: this._shouldGenerateStep ? 'fixture' : undefined,
     }, async () => {
       testInfo._timeoutManager.setCurrentFixture(this._teardownDescription);
-      if (!this._teardownWithDepsComplete)
+      await this._teardownInternal();
-        this._teardownWithDepsComplete = this._teardownInternal();
-      await this._teardownWithDepsComplete;
       testInfo._timeoutManager.setCurrentFixture(undefined);
     });
   }
@@ -153,6 +150,7 @@ class Fixture {
       }
       if (this._useFuncFinished) {
         this._useFuncFinished.resolve();
+        this._useFuncFinished = undefined;
         await this._selfTeardownComplete;
       }
     } finally {
@@ -205,22 +203,12 @@ export class FixtureRunner {
   }
 
   async teardownScope(scope: FixtureScope, testInfo: TestInfoImpl) {
-    if (scope === 'worker') {
-      const collector = new Set<Fixture>();
-      for (const fixture of this.instanceForId.values())
-        fixture._collectFixturesInTeardownOrder('test', collector);
-      // Clean up test-scoped fixtures that did not teardown because of timeout in one of them.
-      // This preserves fixture integrity for worker fixtures.
-      for (const fixture of collector)
-        fixture._cleanupInstance();
-      this.testScopeClean = true;
-    }
-
     // Teardown fixtures in the reverse order.
     const fixtures = Array.from(this.instanceForId.values()).reverse();
     const collector = new Set<Fixture>();
     for (const fixture of fixtures)
       fixture._collectFixturesInTeardownOrder(scope, collector);
+    try {
       let firstError: Error | undefined;
       for (const fixture of collector) {
         try {
@@ -231,10 +219,16 @@ export class FixtureRunner {
           firstError = firstError ?? error;
         }
       }
-    if (scope === 'test')
-      this.testScopeClean = true;
       if (firstError)
         throw firstError;
+    } finally {
+      // To preserve fixtures integrity, forcefully cleanup fixtures that did not teardown
+      // due to a timeout in one of them.
+      for (const fixture of collector)
+        fixture._cleanupInstance();
+      if (scope === 'test')
+        this.testScopeClean = true;
+    }
   }
 
   async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise<object | null> {

packages/playwright/src/worker/workerMain.ts

@@ -364,8 +364,9 @@ export class WorkerMain extends ProcessRunner {
     // Update duration, so it is available in fixture teardown and afterEach hooks.
     testInfo.duration = testInfo._timeoutManager.defaultSlotTimings().elapsed | 0;
 
-    // A timed-out test gets a full additional timeout to run after hooks.
+    // After hooks get an additional timeout.
-    const afterHooksSlot = testInfo._didTimeout ? { timeout: this._project.project.timeout, elapsed: 0 } : undefined;
+    const afterHooksTimeout = calculateMaxTimeout(this._project.project.timeout, testInfo.timeout);
+    const afterHooksSlot = { timeout: afterHooksTimeout, elapsed: 0 };
     await testInfo._runAsStage({
       title: 'After Hooks',
       stepCategory: 'hook',
@@ -467,7 +468,7 @@ export class WorkerMain extends ProcessRunner {
       await testInfo._tracing.stopIfNeeded();
     }).catch(() => {});  // Ignore top-level error.
 
-    testInfo.duration = testInfo._timeoutManager.defaultSlotTimings().elapsed | 0;
+    testInfo.duration = (testInfo._timeoutManager.defaultSlotTimings().elapsed + afterHooksSlot.elapsed) | 0;
 
     this._currentTest = null;
     setCurrentTestInfo(null);
@@ -624,4 +625,9 @@ function formatTestTitle(test: TestCase, projectName: string) {
   return `${projectTitle}${location} › ${titles.join(' › ')}`;
 }
 
+function calculateMaxTimeout(t1: number, t2: number) {
+  // Zero means "no timeout".
+  return (!t1 || !t2) ? 0 : Math.max(t1, t2);
+}
+
 export const create = (params: WorkerInitParams) => new WorkerMain(params);

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

@@ -425,6 +425,38 @@ test('should give enough time for fixture teardown', async ({ runInlineTest }) =
       });
     `,
   });
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(1);
+  expect(result.outputLines).toEqual([
+    'teardown start',
+    'teardown finished',
+  ]);
+});
+
+test('should not give enough time for second fixture teardown after timeout', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      import { test as base, expect } from '@playwright/test';
+      const test = base.extend({
+        fixture2: async ({ }, use) => {
+          await use();
+          console.log('\\n%%teardown2 start');
+          await new Promise(f => setTimeout(f, 3000));
+          console.log('\\n%%teardown2 finished');
+        },
+        fixture: async ({ fixture2 }, use) => {
+          await use();
+          console.log('\\n%%teardown start');
+          await new Promise(f => setTimeout(f, 3000));
+          console.log('\\n%%teardown finished');
+        },
+      });
+      test('fast enough but close', async ({ fixture }) => {
+        test.setTimeout(3000);
+        await new Promise(f => setTimeout(f, 2000));
+      });
+    `,
+  }, { timeout: 2000 });
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
   expect(result.output).toContain('Test finished within timeout of 3000ms, but tearing down "fixture" ran out of time.');

tests/playwright-test/reporter-html.spec.ts

@@ -2052,9 +2052,9 @@ for (const useIntermediateMergeReport of [false, true] as const) {
 
       // Failing test first, then sorted by the run order.
       await expect(page.locator('.test-file-test')).toHaveText([
-        /main › fails\d+m?smain.spec.ts:9/,
+        /main › fails\d+m?s?main.spec.ts:9/,
-        /main › first › passes\d+m?sfirst.ts:12/,
+        /main › first › passes\d+m?s?first.ts:12/,
-        /main › second › passes\d+m?ssecond.ts:5/,
+        /main › second › passes\d+m?s?second.ts:5/,
       ]);
     });