chore(test runner): run hooks/modifiers as a part of the test (#12329)

chore(test runner): run hooks/modifiers as a part of the test

This moves `beforeAll`, `afterAll` and some modifiers from running
as a separate entity into running inside a test.

Pros:
- All errors are reported as test errors.
- All artifacts are collected as test artifacts.
- Reporters support this out of the box.

Details:
- Each test computes the necessary hooks to run and runs them.
- Teardown is usually performed during the test (on test failure or worker stop).
- `skipRemaining` is added to `DonePayload` to preserve the behavior
  where `beforeAll` hook failure skips subsequent tests.
  This behavior can now be improved to only target tests affected by this hook.

packages/playwright-test/src/dispatcher.ts

@@ -59,15 +59,8 @@ export class Dispatcher {
     this._queue = testGroups;
     for (const group of testGroups) {
       this._queueHashCount.set(group.workerHash, 1 + (this._queueHashCount.get(group.workerHash) || 0));
-      for (const test of group.tests) {
+      for (const test of group.tests)
         this._testById.set(test._id, { test, resultByWorkerIndex: new Map() });
-        for (let suite: Suite | undefined = test.parent; suite; suite = suite.parent) {
-          for (const hook of suite.hooks) {
-            if (!this._testById.has(hook._id))
-              this._testById.set(hook._id, { test: hook, resultByWorkerIndex: new Map() });
-          }
-        }
-      }
     }
   }
 
@@ -184,25 +177,20 @@ export class Dispatcher {
 
     const remainingByTestId = new Map(testGroup.tests.map(e => [ e._id, e ]));
     const failedTestIds = new Set<string>();
-    let runningHookId: string | undefined;
 
     const onTestBegin = (params: TestBeginPayload) => {
       const data = this._testById.get(params.testId)!;
-      if (data.test._type !== 'test')
-        runningHookId = params.testId;
       if (this._hasReachedMaxFailures())
         return;
       const result = data.test._appendTestResult();
       data.resultByWorkerIndex.set(worker.workerIndex, { result, stepStack: new Set(), steps: new Map() });
       result.workerIndex = worker.workerIndex;
       result.startTime = new Date(params.startWallTime);
-      if (data.test._type === 'test')
       this._reporter.onTestBegin?.(data.test, result);
     };
     worker.addListener('testBegin', onTestBegin);
 
     const onTestEnd = (params: TestEndPayload) => {
-      runningHookId = undefined;
       remainingByTestId.delete(params.testId);
       if (this._hasReachedMaxFailures())
         return;
@@ -224,7 +212,7 @@ export class Dispatcher {
       test.annotations = params.annotations;
       test.timeout = params.timeout;
       const isFailure = result.status !== 'skipped' && result.status !== test.expectedStatus;
-      if (isFailure && test._type === 'test')
+      if (isFailure)
         failedTestIds.add(params.testId);
       this._reportTestEnd(test, result);
     };
@@ -290,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) {
+      if (!remaining.length && !failedTestIds.size && !params.fatalErrors.length && !params.skipRemaining) {
         if (this._isWorkerRedundant(worker))
           worker.stop();
         doneWithJob();
@@ -302,18 +290,8 @@ export class Dispatcher {
 
       // In case of fatal error, report first remaining test as failing with this error,
       // and all others as skipped.
-      if (params.fatalErrors.length) {
+      if (params.fatalErrors.length || params.skipRemaining) {
-        // Perhaps we were running a hook - report it as failed.
+        let shouldAddFatalErrorsToNextTest = params.fatalErrors.length > 0;
-        if (runningHookId) {
-          const data = this._testById.get(runningHookId)!;
-          const { result } = data.resultByWorkerIndex.get(worker.workerIndex)!;
-          result.errors = [...params.fatalErrors];
-          result.error = result.errors[0];
-          result.status = 'failed';
-          this._reporter.onTestEnd?.(data.test, result);
-        }
-
-        let first = true;
         for (const test of remaining) {
           if (this._hasReachedMaxFailures())
             break;
@@ -325,24 +303,23 @@ export class Dispatcher {
             result = runData.result;
           } else {
             result = data.test._appendTestResult();
-            if (test._type === 'test')
             this._reporter.onTestBegin?.(test, result);
           }
-          result.errors = [...params.fatalErrors];
+          result.errors = shouldAddFatalErrorsToNextTest ? [...params.fatalErrors] : [];
           result.error = result.errors[0];
-          result.status = first ? 'failed' : 'skipped';
+          result.status = shouldAddFatalErrorsToNextTest ? 'failed' : 'skipped';
           this._reportTestEnd(test, result);
           failedTestIds.add(test._id);
-          first = false;
+          shouldAddFatalErrorsToNextTest = false;
         }
-        if (first) {
+        if (shouldAddFatalErrorsToNextTest) {
           // We had a fatal error after all tests have passed - most likely in the afterAll hook.
           // 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, there is nothing else to run,
+        // Since we pretend that all remaining tests failed/skipped, there is nothing else to run,
         // except for possible retries.
         remaining = [];
       }
@@ -375,7 +352,6 @@ export class Dispatcher {
 
         // Emulate a "skipped" run, and drop this test from remaining.
         const result = test._appendTestResult();
-        if (test._type === 'test')
         this._reporter.onTestBegin?.(test, result);
         result.status = 'skipped';
         this._reportTestEnd(test, result);
@@ -408,7 +384,7 @@ export class Dispatcher {
     worker.on('done', onDone);
 
     const onExit = (expectedly: boolean) => {
-      onDone({ fatalErrors: expectedly ? [] : [{ value: 'Worker process exited unexpectedly' }] });
+      onDone({ skipRemaining: false, fatalErrors: expectedly ? [] : [{ value: 'Worker process exited unexpectedly' }] });
     };
     worker.on('exit', onExit);
 
@@ -460,9 +436,8 @@ export class Dispatcher {
   }
 
   private _reportTestEnd(test: TestCase, result: TestResult) {
-    if (test._type === 'test' && result.status !== 'skipped' && result.status !== test.expectedStatus)
+    if (result.status !== 'skipped' && result.status !== test.expectedStatus)
       ++this._failureCount;
-    if (test._type === 'test')
     this._reporter.onTestEnd?.(test, result);
     const maxFailures = this._loader.fullConfig().maxFailures;
     if (maxFailures && this._failureCount === maxFailures)

packages/playwright-test/src/fixtures.ts

@@ -51,7 +51,7 @@ class Fixture {
     this.value = null;
   }
 
-  async setup(workerInfo: WorkerInfo, testInfo: TestInfo | undefined) {
+  async setup(testInfo: TestInfo) {
     if (typeof this.registration.fn !== 'function') {
       this.value = this.registration.fn;
       return;
@@ -60,7 +60,7 @@ class Fixture {
     const params: { [key: string]: any } = {};
     for (const name of this.registration.deps) {
       const registration = this.runner.pool!.resolveDependency(this.registration, name)!;
-      const dep = await this.runner.setupFixtureForRegistration(registration, workerInfo, testInfo);
+      const dep = await this.runner.setupFixtureForRegistration(registration, testInfo);
       dep.usages.add(this);
       params[name] = dep.value;
     }
@@ -77,6 +77,7 @@ class Fixture {
       useFuncStarted.resolve();
       await this._useFuncFinished;
     };
+    const workerInfo: WorkerInfo = { config: testInfo.config, parallelIndex: testInfo.parallelIndex, workerIndex: testInfo.workerIndex, project: testInfo.project };
     const info = this.registration.scope === 'worker' ? workerInfo : testInfo;
     this._selfTeardownComplete = Promise.resolve().then(() => this.registration.fn(params, useFunc, info)).catch((e: any) => {
       if (!useFuncStarted.isDone())
@@ -261,12 +262,12 @@ export class FixtureRunner {
       throw error;
   }
 
-  async resolveParametersForFunction(fn: Function, workerInfo: WorkerInfo, testInfo: TestInfo | undefined): Promise<object> {
+  async resolveParametersForFunction(fn: Function, testInfo: TestInfo): Promise<object> {
     // Install all automatic fixtures.
     for (const registration of this.pool!.registrations.values()) {
       const shouldSkip = !testInfo && registration.scope === 'test';
       if (registration.auto && !shouldSkip)
-        await this.setupFixtureForRegistration(registration, workerInfo, testInfo);
+        await this.setupFixtureForRegistration(registration, testInfo);
     }
 
     // Install used fixtures.
@@ -274,18 +275,18 @@ export class FixtureRunner {
     const params: { [key: string]: any } = {};
     for (const name of names) {
       const registration = this.pool!.registrations.get(name)!;
-      const fixture = await this.setupFixtureForRegistration(registration, workerInfo, testInfo);
+      const fixture = await this.setupFixtureForRegistration(registration, testInfo);
       params[name] = fixture.value;
     }
     return params;
   }
 
-  async resolveParametersAndRunFunction(fn: Function, workerInfo: WorkerInfo, testInfo: TestInfo | undefined) {
+  async resolveParametersAndRunFunction(fn: Function, testInfo: TestInfo) {
-    const params = await this.resolveParametersForFunction(fn, workerInfo, testInfo);
+    const params = await this.resolveParametersForFunction(fn, testInfo);
-    return fn(params, testInfo || workerInfo);
+    return fn(params, testInfo);
   }
 
-  async setupFixtureForRegistration(registration: FixtureRegistration, workerInfo: WorkerInfo, testInfo: TestInfo | undefined): Promise<Fixture> {
+  async setupFixtureForRegistration(registration: FixtureRegistration, testInfo: TestInfo): Promise<Fixture> {
     if (registration.scope === 'test')
       this.testScopeClean = false;
 
@@ -295,7 +296,7 @@ export class FixtureRunner {
 
     fixture = new Fixture(this, registration);
     this.instanceForId.set(registration.id, fixture);
-    await fixture.setup(workerInfo, testInfo);
+    await fixture.setup(testInfo);
     return fixture;
   }
 

packages/playwright-test/src/index.ts

@@ -423,7 +423,7 @@ export const test = _baseTest.extend<TestFixtures, WorkerFixtures>({
     }));
 
     // 7. Cleanup created contexts when we know it's safe - this will produce nice error message.
-    if (hookType(testInfo) === 'beforeAll' && testInfo.status === 'timedOut') {
+    if (testInfo.status === 'timedOut' && testInfo.errors.some(error => error.message?.match(/Timeout of \d+ms exceeded in beforeAll hook./))) {
       const anyContext = leftoverContexts[0];
       const pendingCalls = anyContext ? formatPendingCalls((anyContext as any)._connection.pendingProtocolCalls()) : '';
       await Promise.all(leftoverContexts.filter(c => createdContexts.has(c)).map(c => c.close()));
@@ -519,9 +519,10 @@ function formatStackFrame(frame: StackFrame) {
 }
 
 function hookType(testInfo: TestInfo): 'beforeAll' | 'afterAll' | undefined {
-  if (testInfo.title.startsWith('beforeAll'))
+  const impl = testInfo as import('./testInfo').TestInfoImpl;
+  if (impl._currentRunnable?.type === 'beforeAll')
     return 'beforeAll';
-  if (testInfo.title.startsWith('afterAll'))
+  if (impl._currentRunnable?.type === 'afterAll')
     return 'afterAll';
 }
 

packages/playwright-test/src/ipc.ts

@@ -76,6 +76,7 @@ export type RunPayload = {
 
 export type DonePayload = {
   fatalErrors: TestError[];
+  skipRemaining: boolean;
 };
 
 export type TestOutputPayload = {

packages/playwright-test/src/project.ts

@@ -52,10 +52,8 @@ export class ProjectImpl {
       for (const parent of parents) {
         if (parent._use.length)
           pool = new FixturePool(parent._use, pool, parent._isDescribe);
-        for (const hook of parent._eachHooks)
+        for (const hook of parent._hooks)
           pool.validateFunction(hook.fn, hook.type + ' hook', hook.location);
-        for (const hook of parent.hooks)
-          pool.validateFunction(hook.fn, hook._type + ' hook', hook.location);
         for (const modifier of parent._modifiers)
           pool.validateFunction(modifier.fn, modifier.type + ' modifier', modifier.location);
       }
@@ -94,15 +92,6 @@ export class ProjectImpl {
     }
     if (!to._entries.length)
       return false;
-    for (const hook of from.hooks) {
-      const clone = hook._clone();
-      clone.retries = 1;
-      clone._pool = this.buildPool(hook);
-      clone._projectIndex = this.index;
-      clone._id = `${hook._ordinalInFile}@${hook._requireFile}#run${this.index}-repeat${repeatEachIndex}`;
-      clone.repeatEachIndex = repeatEachIndex;
-      to._addAllHook(clone);
-    }
     return true;
   }
 

packages/playwright-test/src/reporters/line.ts

@@ -61,7 +61,6 @@ class LineReporter extends BaseReporter {
 
   override onTestEnd(test: TestCase, result: TestResult) {
     super.onTestEnd(test, result);
-    if (!test.title.startsWith('beforeAll') && !test.title.startsWith('afterAll'))
     ++this._current;
     const retriesSuffix = this.totalTestCount < this._current ? ` (retries)` : ``;
     const title = `[${this._current}/${this.totalTestCount}]${retriesSuffix} ${formatTestTitle(this.config, test)}`;

packages/playwright-test/src/test.ts

@@ -17,7 +17,7 @@
 import type { FixturePool } from './fixtures';
 import * as reporterTypes from '../types/testReporter';
 import type { TestTypeImpl } from './testType';
-import { Annotations, FixturesWithLocation, Location, TestCaseType } from './types';
+import { Annotation, FixturesWithLocation, Location } from './types';
 import { FullProject } from './types';
 
 class Base {
@@ -45,10 +45,9 @@ export class Suite extends Base implements reporterTypes.Suite {
   _use: FixturesWithLocation[] = [];
   _isDescribe = false;
   _entries: (Suite | TestCase)[] = [];
-  hooks: TestCase[] = [];
+  _hooks: { type: 'beforeEach' | 'afterEach' | 'beforeAll' | 'afterAll', fn: Function, location: Location }[] = [];
-  _eachHooks: { type: 'beforeEach' | 'afterEach', fn: Function, location: Location }[] = [];
   _timeout: number | undefined;
-  _annotations: Annotations = [];
+  _annotations: Annotation[] = [];
   _modifiers: Modifier[] = [];
   _parallelMode: 'default' | 'serial' | 'parallel' = 'default';
   _projectConfig: FullProject | undefined;
@@ -66,11 +65,6 @@ export class Suite extends Base implements reporterTypes.Suite {
     this._entries.push(suite);
   }
 
-  _addAllHook(hook: TestCase) {
-    hook.parent = this;
-    this.hooks.push(hook);
-  }
-
   allTests(): TestCase[] {
     const result: TestCase[] = [];
     const visit = (suite: Suite) => {
@@ -107,7 +101,7 @@ export class Suite extends Base implements reporterTypes.Suite {
     suite.location = this.location;
     suite._requireFile = this._requireFile;
     suite._use = this._use.slice();
-    suite._eachHooks = this._eachHooks.slice();
+    suite._hooks = this._hooks.slice();
     suite._timeout = this._timeout;
     suite._annotations = this._annotations.slice();
     suite._modifiers = this._modifiers.slice();
@@ -130,11 +124,10 @@ export class TestCase extends Base implements reporterTypes.TestCase {
 
   expectedStatus: reporterTypes.TestStatus = 'passed';
   timeout = 0;
-  annotations: Annotations = [];
+  annotations: Annotation[] = [];
   retries = 0;
   repeatEachIndex = 0;
 
-  _type: TestCaseType;
   _ordinalInFile: number;
   _testType: TestTypeImpl;
   _id = '';
@@ -142,9 +135,8 @@ export class TestCase extends Base implements reporterTypes.TestCase {
   _pool: FixturePool | undefined;
   _projectIndex = 0;
 
-  constructor(type: TestCaseType, title: string, fn: Function, ordinalInFile: number, testType: TestTypeImpl, location: Location) {
+  constructor(title: string, fn: Function, ordinalInFile: number, testType: TestTypeImpl, location: Location) {
     super(title);
-    this._type = type;
     this.fn = fn;
     this._ordinalInFile = ordinalInFile;
     this._testType = testType;
@@ -174,7 +166,7 @@ export class TestCase extends Base implements reporterTypes.TestCase {
   }
 
   _clone(): TestCase {
-    const test = new TestCase(this._type, this.title, this.fn, this._ordinalInFile, this._testType, this.location);
+    const test = new TestCase(this.title, this.fn, this._ordinalInFile, this._testType, this.location);
     test._only = this._only;
     test._requireFile = this._requireFile;
     test.expectedStatus = this.expectedStatus;

packages/playwright-test/src/testInfo.ts

@@ -25,8 +25,13 @@ import { WorkerInitParams } from './ipc';
 import { Loader } from './loader';
 import { ProjectImpl } from './project';
 import { TestCase } from './test';
-import { Annotations, TestStepInternal } from './types';
+import { Annotation, TestStepInternal, Location } from './types';
-import { addSuffixToFilePath, formatLocation, getContainedPath, monotonicTime, sanitizeForFilePath, serializeError, trimLongString } from './util';
+import { addSuffixToFilePath, getContainedPath, monotonicTime, sanitizeForFilePath, serializeError, trimLongString } from './util';
+
+type RunnableDescription = {
+  type: 'test' | 'beforeAll' | 'afterAll' | 'beforeEach' | 'afterEach' | 'slow' | 'skip' | 'fail' | 'fixme' | 'teardown';
+  location?: Location;
+};
 
 export class TestInfoImpl implements TestInfo {
   private _projectImpl: ProjectImpl;
@@ -36,6 +41,7 @@ export class TestInfoImpl implements TestInfo {
   readonly _startTime: number;
   readonly _startWallTime: number;
   private _hasHardError: boolean = false;
+  _currentRunnable: RunnableDescription | undefined;
 
   // ------------ TestInfo fields ------------
   readonly repeatEachIndex: number;
@@ -52,7 +58,7 @@ export class TestInfoImpl implements TestInfo {
   readonly fn: Function;
   expectedStatus: TestStatus;
   duration: number = 0;
-  readonly annotations: Annotations = [];
+  readonly annotations: Annotation[] = [];
   readonly attachments: TestInfo['attachments'] = [];
   status: TestStatus = 'passed';
   readonly stdout: TestInfo['stdout'] = [];
@@ -116,7 +122,7 @@ export class TestInfoImpl implements TestInfo {
 
       const relativeTestFilePath = path.relative(this.project.testDir, test._requireFile.replace(/\.(spec|test)\.(js|ts|mjs)$/, ''));
       const sanitizedRelativePath = relativeTestFilePath.replace(process.platform === 'win32' ? new RegExp('\\\\', 'g') : new RegExp('/', 'g'), '-');
-      const fullTitleWithoutSpec = test.titlePath().slice(1).join(' ') + (test._type === 'test' ? '' : '-worker' + this.workerIndex);
+      const fullTitleWithoutSpec = test.titlePath().slice(1).join(' ');
 
       let testOutputDir = trimLongString(sanitizedRelativePath + '-' + sanitizeForFilePath(fullTitleWithoutSpec));
       if (uniqueProjectNamePathSegment)
@@ -170,13 +176,15 @@ export class TestInfoImpl implements TestInfo {
       // Do not overwrite existing failure upon hook/teardown timeout.
       if (this.status === 'passed') {
         this.status = 'timedOut';
-        if (this._test._type === 'test') {
+        const title = titleForRunnable(this._currentRunnable);
-          this.errors.push({ message: colors.red(`Timeout of ${this.timeout}ms exceeded.`) });
+        const suffix = title ? ` in ${title}` : '';
-        } else {
+        const message = colors.red(`Timeout of ${this.timeout}ms exceeded${suffix}.`);
-          // Include location for the hook to distinguish between multiple hooks.
+        const location = this._currentRunnable?.location;
-          const message = colors.red(`Timeout of ${this.timeout}ms exceeded in ${this._test._type} hook.`);
+        this.errors.push({
-          this.errors.push({ message: message, stack: message + `\n    at ${formatLocation(this._test.location)}.` });
+          message,
-        }
+          // Include location for hooks and modifiers to distinguish between them.
+          stack: location ? message + `\n    at ${location.file}:${location.line}:${location.column}` : undefined,
+        });
       }
     }
     this.duration = monotonicTime() - this._startTime;
@@ -282,3 +290,24 @@ export class TestInfoImpl implements TestInfo {
 
 class SkipError extends Error {
 }
+
+function titleForRunnable(runnable: RunnableDescription | undefined): string {
+  if (!runnable)
+    return '';
+  switch (runnable.type) {
+    case 'test':
+      return '';
+    case 'beforeAll':
+    case 'beforeEach':
+    case 'afterAll':
+    case 'afterEach':
+      return runnable.type + ' hook';
+    case 'teardown':
+      return 'fixtures teardown';
+    case 'skip':
+    case 'slow':
+    case 'fixme':
+    case 'fail':
+      return runnable.type + ' modifier';
+  }
+}

packages/playwright-test/src/testType.ts

@@ -81,7 +81,7 @@ export class TestTypeImpl {
   private _createTest(type: 'default' | 'only' | 'skip' | 'fixme', location: Location, title: string, fn: Function) {
     throwIfRunningInsideJest();
     const suite = this._ensureCurrentSuite(location, 'test()');
-    const test = new TestCase('test', title, fn, nextOrdinalInFile(suite._requireFile), this, location);
+    const test = new TestCase(title, fn, nextOrdinalInFile(suite._requireFile), this, location);
     test._requireFile = suite._requireFile;
     suite._addTest(test);
 
@@ -130,15 +130,7 @@ export class TestTypeImpl {
 
   private _hook(name: 'beforeEach' | 'afterEach' | 'beforeAll' | 'afterAll', location: Location, fn: Function) {
     const suite = this._ensureCurrentSuite(location, `test.${name}()`);
-    if (name === 'beforeAll' || name === 'afterAll') {
+    suite._hooks.push({ type: name, fn, location });
-      const sameTypeCount = suite.hooks.filter(hook => hook._type === name).length;
-      const suffix = sameTypeCount ? String(sameTypeCount) : '';
-      const hook = new TestCase(name, name + suffix, fn, nextOrdinalInFile(suite._requireFile), this, location);
-      hook._requireFile = suite._requireFile;
-      suite._addAllHook(hook);
-    } else {
-      suite._eachHooks.push({ type: name, fn, location });
-    }
   }
 
   private _configure(location: Location, options: { mode?: 'parallel' | 'serial' }) {

packages/playwright-test/src/types.ts

@@ -23,7 +23,7 @@ export type FixturesWithLocation = {
   fixtures: Fixtures;
   location: Location;
 };
-export type Annotations = { type: string, description?: string }[];
+export type Annotation = { type: string, description?: string };
 
 export interface TestStepInternal {
   complete(error?: Error | TestError): void;
@@ -33,5 +33,3 @@ export interface TestStepInternal {
   forceNoParent: boolean;
   location?: Location;
 }
-
-export type TestCaseType = 'beforeAll' | 'afterAll' | 'test';

packages/playwright-test/src/worker.ts

@@ -84,7 +84,7 @@ process.on('message', async message => {
   }
   if (message.method === 'run') {
     const runPayload = message.params as RunPayload;
-    await workerRunner!.run(runPayload);
+    await workerRunner!.runTestGroup(runPayload);
   }
 });
 

packages/playwright-test/src/workerRunner.ts

@@ -18,15 +18,15 @@ import rimraf from 'rimraf';
 import util from 'util';
 import colors from 'colors/safe';
 import { EventEmitter } from 'events';
-import { serializeError, formatLocation } from './util';
+import { serializeError } from './util';
-import { TestBeginPayload, TestEndPayload, RunPayload, TestEntry, DonePayload, WorkerInitParams, StepBeginPayload, StepEndPayload, TeardownErrorsPayload } from './ipc';
+import { TestBeginPayload, TestEndPayload, RunPayload, DonePayload, WorkerInitParams, StepBeginPayload, StepEndPayload, TeardownErrorsPayload } from './ipc';
 import { setCurrentTestInfo } from './globals';
 import { Loader } from './loader';
-import { Modifier, Suite, TestCase } from './test';
+import { Suite, TestCase } from './test';
-import { Annotations, TestError, TestInfo, TestStepInternal, WorkerInfo } from './types';
+import { Annotation, TestError, TestStepInternal } from './types';
 import { ProjectImpl } from './project';
 import { FixtureRunner } from './fixtures';
-import { raceAgainstTimeout } from 'playwright-core/lib/utils/async';
+import { ManualPromise, raceAgainstTimeout } from 'playwright-core/lib/utils/async';
 import { TestInfoImpl } from './testInfo';
 
 const removeFolderAsync = util.promisify(rimraf);
@@ -35,15 +35,25 @@ export class WorkerRunner extends EventEmitter {
   private _params: WorkerInitParams;
   private _loader!: Loader;
   private _project!: ProjectImpl;
-  private _workerInfo!: WorkerInfo;
   private _fixtureRunner: FixtureRunner;
 
-  private _failedTest: TestInfoImpl | undefined;
+  // Accumulated fatal errors that cannot be attributed to a test.
   private _fatalErrors: TestError[] = [];
-  private _entries = new Map<string, TestEntry>();
+  // Whether we should skip running remaining tests in the group because
+  // of a setup error, usually beforeAll hook.
+  private _skipRemainingTests = false;
+  // The stage of the full cleanup. Once "finished", we can safely stop running anything.
+  private _didRunFullCleanup = false;
+  // Whether the worker was requested to stop.
   private _isStopped = false;
-  private _runFinished = Promise.resolve();
+  // This promise resolves once the single "run test group" call finishes.
+  private _runFinished = new ManualPromise<void>();
   _currentTest: TestInfoImpl | null = null;
+  // Dynamic annotations originated by modifiers with a callback, e.g. `test.skip(() => true)`.
+  private _extraSuiteAnnotations = new Map<Suite, Annotation[]>();
+  // Suites that had their beforeAll hooks, but not afterAll hooks executed.
+  // These suites still need afterAll hooks to be executed for the proper cleanup.
+  private _activeSuites = new Set<Suite>();
 
   constructor(params: WorkerInitParams) {
     super();
@@ -100,7 +110,7 @@ export class WorkerRunner extends EventEmitter {
     const isExpectError = (error instanceof Error) && !!(error as any).matcherResult;
     const isCurrentTestExpectedToFail = this._currentTest?.expectedStatus === 'failed';
     const shouldConsiderAsTestError = isExpectError || !isCurrentTestExpectedToFail;
-    if (this._currentTest && this._currentTest._test._type === 'test' && shouldConsiderAsTestError) {
+    if (this._currentTest && shouldConsiderAsTestError) {
       this._currentTest._failWithError(serializeError(error), true /* isHardError */);
     } else {
       // No current test - fatal error.
@@ -116,102 +126,43 @@ export class WorkerRunner extends EventEmitter {
 
     this._loader = await Loader.deserialize(this._params.loader);
     this._project = this._loader.projects()[this._params.projectIndex];
-
-    this._workerInfo = {
-      workerIndex: this._params.workerIndex,
-      parallelIndex: this._params.parallelIndex,
-      project: this._project.config,
-      config: this._loader.fullConfig(),
-    };
   }
 
-  async run(runPayload: RunPayload) {
+  async runTestGroup(runPayload: RunPayload) {
-    let runFinishedCallback = () => {};
+    this._runFinished = new ManualPromise<void>();
-    this._runFinished = new Promise(f => runFinishedCallback = f);
     try {
-      this._entries = new Map(runPayload.entries.map(e => [ e.testId, e ]));
+      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 => {
-        if (!this._entries.has(test._id))
+        if (!entries.has(test._id))
           return false;
         return true;
       });
       if (suite) {
-        const firstPool = suite.allTests()[0]._pool!;
+        this._extraSuiteAnnotations = new Map();
-        this._fixtureRunner.setPool(firstPool);
+        this._activeSuites = new Set();
-        await this._runSuite(suite, []);
+        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]);
       }
-      if (this._failedTest)
-        await this._teardownScopes();
     } catch (e) {
       // In theory, we should run above code without any errors.
       // However, in the case we screwed up, or loadTestFile failed in the worker
       // but not in the runner, let's do a fatal error.
       this.unhandledError(e);
     } finally {
-      if (this._failedTest) {
-        // Now that we did run all hooks and teared down scopes, we can
-        // report the failure, possibly with any error details revealed by teardown.
-        this.emit('testEnd', buildTestEndPayload(this._failedTest));
-      }
       this._reportDone();
-      runFinishedCallback();
+      this._runFinished.resolve();
     }
   }
 
-  private async _runSuite(suite: Suite, annotations: Annotations) {
+  private async _runTest(test: TestCase, retry: number, nextTest: TestCase | undefined) {
-    // When stopped, do not run a suite. But if we have started running the suite with hooks,
+    // Do not run tests after full cleanup, because we are entirely done.
-    // always finish the hooks.
+    if (this._isStopped && this._didRunFullCleanup)
-    if (this._isStopped)
       return;
-    annotations = annotations.concat(suite._annotations);
 
-    const allSkipped = suite.allTests().every(test => {
-      const runEntry = this._entries.get(test._id);
-      return !runEntry || test.expectedStatus === 'skipped';
-    });
-    if (allSkipped) {
-      // This avoids running beforeAll/afterAll hooks.
-      annotations.push({ type: 'skip' });
-    }
-
-    for (const beforeAllModifier of suite._modifiers) {
-      if (!this._fixtureRunner.dependsOnWorkerFixturesOnly(beforeAllModifier.fn, beforeAllModifier.location))
-        continue;
-      // TODO: separate timeout for beforeAll modifiers?
-      const result = await raceAgainstTimeout(() => this._fixtureRunner.resolveParametersAndRunFunction(beforeAllModifier.fn, this._workerInfo, undefined), this._project.config.timeout);
-      if (result.timedOut) {
-        this._fatalErrors.push(serializeError(new Error(`Timeout of ${this._project.config.timeout}ms exceeded while running ${beforeAllModifier.type} modifier\n    at ${formatLocation(beforeAllModifier.location)}`)));
-        this.stop();
-      } else if (!!result.result) {
-        annotations.push({ type: beforeAllModifier.type, description: beforeAllModifier.description });
-      }
-    }
-
-    for (const hook of suite.hooks) {
-      if (hook._type !== 'beforeAll')
-        continue;
-      const firstTest = suite.allTests()[0];
-      await this._runTestOrAllHook(hook, annotations, this._entries.get(firstTest._id)?.retry || 0);
-    }
-    for (const entry of suite._entries) {
-      if (entry instanceof Suite) {
-        await this._runSuite(entry, annotations);
-      } else {
-        const runEntry = this._entries.get(entry._id);
-        if (runEntry && !this._isStopped)
-          await this._runTestOrAllHook(entry, annotations, runEntry.retry);
-      }
-    }
-    for (const hook of suite.hooks) {
-      if (hook._type !== 'afterAll')
-        continue;
-      await this._runTestOrAllHook(hook, annotations, 0);
-    }
-  }
-
-  private async _runTestOrAllHook(test: TestCase, annotations: Annotations, retry: number) {
     let lastStepId = 0;
     const testInfo = new TestInfoImpl(this._loader, this._params, test, retry, data => {
       const stepId = `${data.category}@${data.title}@${++lastStepId}`;
@@ -247,16 +198,7 @@ export class WorkerRunner extends EventEmitter {
       return step;
     });
 
-    // Inherit test.setTimeout() from parent suites.
+    const processAnnotation = (annotation: Annotation) => {
-    for (let suite: Suite | undefined = test.parent; suite; suite = suite.parent) {
-      if (suite._timeout !== undefined) {
-        testInfo.setTimeout(suite._timeout);
-        break;
-      }
-    }
-
-    // Process annotations defined on parent suites.
-    for (const annotation of annotations) {
       testInfo.annotations.push(annotation);
       switch (annotation.type) {
         case 'fixme':
@@ -271,11 +213,35 @@ export class WorkerRunner extends EventEmitter {
           testInfo.setTimeout(testInfo.timeout * 3);
           break;
       }
+    };
+
+    if (!this._isStopped) {
+      // Update the fixture pool - it may differ between tests, but only in test-scoped fixtures.
+      this._fixtureRunner.setPool(test._pool!);
+    }
+
+    const suites = getSuites(test);
+    const reversedSuites = suites.slice().reverse();
+
+    // Inherit test.setTimeout() from parent suites, deepest has the priority.
+    for (const suite of reversedSuites) {
+      if (suite._timeout !== undefined) {
+        testInfo.setTimeout(suite._timeout);
+        break;
+      }
+    }
+
+    // Process existing annotations defined on parent suites.
+    for (const suite of suites) {
+      for (const annotation of suite._annotations)
+        processAnnotation(annotation);
+      const extraAnnotations = this._extraSuiteAnnotations.get(suite) || [];
+      for (const annotation of extraAnnotations)
+        processAnnotation(annotation);
     }
 
     this._currentTest = testInfo;
     setCurrentTestInfo(testInfo);
-
     this.emit('testBegin', buildTestBeginPayload(testInfo));
 
     if (testInfo.expectedStatus === 'skipped') {
@@ -284,32 +250,137 @@ export class WorkerRunner extends EventEmitter {
       return;
     }
 
-    // Update the fixture pool - it may differ between tests, but only in test-scoped fixtures.
+    // Assume beforeAll failed until we actually finish it successfully.
-    this._fixtureRunner.setPool(test._pool!);
+    let didFailBeforeAll = true;
+    let shouldRunAfterEachHooks = false;
 
-    await testInfo._runWithTimeout(() => this._runTestWithBeforeHooks(test, testInfo));
+    await testInfo._runWithTimeout(async () => {
+      if (this._isStopped) {
+        // 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;
+        return;
+      }
+
+      const beforeHooksStep = testInfo._addStep({
+        category: 'hook',
+        title: 'Before Hooks',
+        canHaveChildren: true,
+        forceNoParent: true
+      });
+
+      // Note: wrap all preparation steps together, because failure in any of them
+      // prevents further setup and/or test from running.
+      const maybeError = await testInfo._runFn(async () => {
+        // Run "beforeAll" modifiers on parent suites, unless already run during previous tests.
+        for (const suite of suites) {
+          if (this._extraSuiteAnnotations.has(suite))
+            continue;
+          const extraAnnotations: Annotation[] = [];
+          this._extraSuiteAnnotations.set(suite, extraAnnotations);
+          await this._runModifiersForSuite(suite, testInfo, 'worker', extraAnnotations);
+        }
+
+        // Run "beforeAll" hooks, unless already run during previous tests.
+        for (const suite of suites)
+          await this._runBeforeAllHooksForSuite(suite, testInfo);
+        // Running "beforeAll" succeeded!
+        didFailBeforeAll = false;
+
+        // Run "beforeEach" modifiers.
+        for (const suite of suites)
+          await this._runModifiersForSuite(suite, testInfo, 'test');
+
+        // Run "beforeEach" hooks. Once started with "beforeEach", we must run all "afterEach" hooks as well.
+        shouldRunAfterEachHooks = true;
+        await this._runEachHooksForSuites(suites, 'beforeEach', testInfo);
+
+        // Setup fixtures required by the test.
+        testInfo._currentRunnable = { type: 'test' };
+        const params = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo);
+        beforeHooksStep.complete(); // Report fixture hooks step as completed.
+
+        // Now run the test itself.
+        const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]").
+        await fn(params, testInfo);
+      }, 'allowSkips');
+
+      beforeHooksStep.complete(maybeError); // Second complete is a no-op.
+    });
+
+    if (didFailBeforeAll) {
+      // 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;
+    }
+
+    const afterHooksStep = testInfo._addStep({
+      category: 'hook',
+      title: 'After Hooks',
+      canHaveChildren: true,
+      forceNoParent: true
+    });
+    let firstAfterHooksError: TestError | undefined;
 
     if (testInfo.status === 'timedOut') {
       // A timed-out test gets a full additional timeout to run after hooks.
       testInfo._timeoutRunner.resetTimeout(testInfo.timeout);
     }
-    await testInfo._runWithTimeout(() => this._runAfterHooks(test, testInfo));
+    await testInfo._runWithTimeout(async () => {
+      // Note: do not wrap all teardown steps together, because failure in any of them
+      // does not prevent further teardown steps from running.
 
-    this._currentTest = null;
+      // Run "afterEach" hooks, unless we failed at beforeAll stage.
-    setCurrentTestInfo(null);
+      if (shouldRunAfterEachHooks) {
+        const afterEachError = await testInfo._runFn(() => this._runEachHooksForSuites(reversedSuites, 'afterEach', testInfo));
+        firstAfterHooksError = firstAfterHooksError || afterEachError;
+      }
+
+      // Run "afterAll" hooks for suites that are not shared with the next test.
+      const nextSuites = new Set(getSuites(nextTest));
+      for (const suite of reversedSuites) {
+        if (!nextSuites.has(suite)) {
+          const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo);
+          firstAfterHooksError = firstAfterHooksError || afterAllError;
+        }
+      }
+
+      // Teardown test-scoped fixtures.
+      testInfo._currentRunnable = { type: 'teardown' };
+      const testScopeError = await testInfo._runFn(() => this._fixtureRunner.teardownScope('test'));
+      firstAfterHooksError = firstAfterHooksError || testScopeError;
+    });
 
     const isFailure = testInfo.status !== 'skipped' && testInfo.status !== testInfo.expectedStatus;
-    if (isFailure) {
+    if (isFailure)
-      // Delay reporting testEnd result until after teardownScopes is done.
+      this._isStopped = true;
-      this._failedTest = testInfo;
+
-      if (test._type !== 'test') {
+    if (this._isStopped) {
-        // beforeAll/afterAll hook failure skips any remaining tests in the worker.
+      // Run all remaining "afterAll" hooks and teardown all fixtures when worker is shutting down.
-        this._fatalErrors.push(...testInfo.errors);
+      // Mark as "cleaned up" early to avoid running cleanup twice.
+      this._didRunFullCleanup = true;
+
+      // Give it more time for the full cleanup.
+      testInfo._timeoutRunner.resetTimeout(this._project.config.timeout);
+      await testInfo._runWithTimeout(async () => {
+        for (const suite of reversedSuites) {
+          const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo);
+          firstAfterHooksError = firstAfterHooksError || afterAllError;
         }
-      this.stop();
+        testInfo._currentRunnable = { type: 'teardown' };
-    } else {
+        const testScopeError = await testInfo._runFn(() => this._fixtureRunner.teardownScope('test'));
+        firstAfterHooksError = firstAfterHooksError || testScopeError;
+        const workerScopeError = await testInfo._runFn(() => this._fixtureRunner.teardownScope('worker'));
+        firstAfterHooksError = firstAfterHooksError || workerScopeError;
+      });
+    }
+
+    afterHooksStep.complete(firstAfterHooksError);
+    this._currentTest = null;
+    setCurrentTestInfo(null);
     this.emit('testEnd', buildTestEndPayload(testInfo));
-    }
 
     const preserveOutput = this._loader.fullConfig().preserveOutput === 'always' ||
       (this._loader.fullConfig().preserveOutput === 'failures-only' && isFailure);
@@ -317,65 +388,63 @@ export class WorkerRunner extends EventEmitter {
       await removeFolderAsync(testInfo.outputDir).catch(e => {});
   }
 
-  private async _runTestWithBeforeHooks(test: TestCase, testInfo: TestInfoImpl) {
+  private async _runModifiersForSuite(suite: Suite, testInfo: TestInfoImpl, scope: 'worker' | 'test', extraAnnotations?: Annotation[]) {
-    const step = testInfo._addStep({
+    for (const modifier of suite._modifiers) {
-      category: 'hook',
+      const actualScope = this._fixtureRunner.dependsOnWorkerFixturesOnly(modifier.fn, modifier.location) ? 'worker' : 'test';
-      title: 'Before Hooks',
+      if (actualScope !== scope)
-      canHaveChildren: true,
+        continue;
-      forceNoParent: true
+      testInfo._currentRunnable = { type: modifier.type, location: modifier.location };
-    });
+      const result = await this._fixtureRunner.resolveParametersAndRunFunction(modifier.fn, testInfo);
-    const maybeError = await testInfo._runFn(async () => {
+      if (result && extraAnnotations)
-      if (test._type === 'test') {
+        extraAnnotations.push({ type: modifier.type, description: modifier.description });
-        const beforeEachModifiers: Modifier[] = [];
+      testInfo[modifier.type](!!result, modifier.description);
-        for (let s: Suite | undefined = test.parent; s; s = s.parent) {
-          const modifiers = s._modifiers.filter(modifier => !this._fixtureRunner.dependsOnWorkerFixturesOnly(modifier.fn, modifier.location));
-          beforeEachModifiers.push(...modifiers.reverse());
     }
-        beforeEachModifiers.reverse();
-        for (const modifier of beforeEachModifiers) {
-          const result = await this._fixtureRunner.resolveParametersAndRunFunction(modifier.fn, this._workerInfo, testInfo);
-          testInfo[modifier.type](!!result, modifier.description!);
-        }
-        await this._runHooks(test.parent!, 'beforeEach', testInfo);
   }
 
-      const params = await this._fixtureRunner.resolveParametersForFunction(test.fn, this._workerInfo, testInfo);
+  private async _runBeforeAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl) {
-      step.complete(); // Report fixture hooks step as completed.
+    if (this._activeSuites.has(suite))
-      const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]").
+      return;
-      await fn(params, testInfo);
+    this._activeSuites.add(suite);
-    }, 'allowSkips');
+    let beforeAllError: Error | undefined;
-    step.complete(maybeError); // Second complete is a no-op.
+    for (const hook of suite._hooks) {
-  }
+      if (hook.type !== 'beforeAll')
-
+        continue;
-  private async _runAfterHooks(test: TestCase, testInfo: TestInfoImpl) {
-    const step = testInfo._addStep({
-      category: 'hook',
-      title: 'After Hooks',
-      canHaveChildren: true,
-      forceNoParent: true
-    });
-
-    let teardownError1: TestError | undefined;
-    if (test._type === 'test')
-      teardownError1 = await testInfo._runFn(() => this._runHooks(test.parent!, 'afterEach', testInfo));
-    // Continue teardown even after the failure.
-
-    const teardownError2 = await testInfo._runFn(() => this._fixtureRunner.teardownScope('test'));
-    step.complete(teardownError1 || teardownError2);
-  }
-
-  private async _runHooks(suite: Suite, type: 'beforeEach' | 'afterEach', testInfo: TestInfo) {
-    const all = [];
-    for (let s: Suite | undefined = suite; s; s = s.parent) {
-      const funcs = s._eachHooks.filter(e => e.type === type).map(e => e.fn);
-      all.push(...funcs.reverse());
-    }
-    if (type === 'beforeEach')
-      all.reverse();
-    let error: Error | undefined;
-    for (const hook of all) {
       try {
-        await this._fixtureRunner.resolveParametersAndRunFunction(hook, this._workerInfo, testInfo);
+        testInfo._currentRunnable = { type: 'beforeAll', location: hook.location };
+        await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo);
+      } catch (e) {
+        // Always run all the hooks, and capture the first error.
+        beforeAllError = beforeAllError || e;
+      }
+    }
+    if (beforeAllError)
+      throw beforeAllError;
+  }
+
+  private async _runAfterAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl) {
+    if (!this._activeSuites.has(suite))
+      return;
+    this._activeSuites.delete(suite);
+    let firstError: TestError | undefined;
+    for (const hook of suite._hooks) {
+      if (hook.type !== 'afterAll')
+        continue;
+      const afterAllError = await testInfo._runFn(async () => {
+        testInfo._currentRunnable = { type: 'afterAll', location: hook.location };
+        await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo);
+      });
+      firstError = firstError || afterAllError;
+    }
+    return firstError;
+  }
+
+  private async _runEachHooksForSuites(suites: Suite[], type: 'beforeEach' | 'afterEach', testInfo: TestInfoImpl) {
+    const hooks = suites.map(suite => suite._hooks.filter(hook => hook.type === type)).flat();
+    let error: Error | undefined;
+    for (const hook of hooks) {
+      try {
+        testInfo._currentRunnable = { type, location: hook.location };
+        await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo);
       } catch (e) {
         // Always run all the hooks, and capture the first error.
         error = error || e;
@@ -386,10 +455,10 @@ export class WorkerRunner extends EventEmitter {
   }
 
   private _reportDone() {
-    const donePayload: DonePayload = { fatalErrors: this._fatalErrors };
+    const donePayload: DonePayload = { fatalErrors: this._fatalErrors, skipRemaining: this._skipRemainingTests };
     this.emit('done', donePayload);
     this._fatalErrors = [];
-    this._failedTest = undefined;
+    this._skipRemainingTests = false;
   }
 }
 
@@ -417,3 +486,11 @@ function buildTestEndPayload(testInfo: TestInfoImpl): TestEndPayload {
     }))
   };
 }
+
+function getSuites(test: TestCase | undefined): Suite[] {
+  const suites: Suite[] = [];
+  for (let suite: Suite | undefined = test?.parent; suite; suite = suite.parent)
+    suites.push(suite);
+  suites.reverse();  // Put root suite first.
+  return suites;
+}

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

@@ -453,7 +453,26 @@ test('should not report fixture teardown error twice', async ({ runInlineTest })
   expect(result.failed).toBe(1);
   expect(result.output).toContain('Error: Oh my error');
   expect(stripAnsi(result.output)).toContain(`throw new Error('Oh my error')`);
-  expect(countTimes(result.output, 'Oh my error')).toBe(2);
+  expect(countTimes(stripAnsi(result.output), 'Oh my error')).toBe(2);
+});
+
+test('should not report fixture teardown timeout twice', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      const test = pwt.test.extend({
+        fixture: async ({ }, use) => {
+          await use();
+          await new Promise(() => {});
+        },
+      });
+      test('good', async ({ fixture }) => {
+      });
+    `,
+  }, { reporter: 'list', timeout: 1000 });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.output).toContain('while shutting down environment');
+  expect(countTimes(result.output, 'while shutting down environment')).toBe(1);
 });
 
 test('should handle fixture teardown error after test timeout and continue', async ({ runInlineTest }) => {

tests/playwright-test/fixtures.spec.ts

@@ -316,23 +316,23 @@ test('automatic fixtures should work', async ({ runInlineTest }) => {
       });
       test.beforeEach(async ({}) => {
         expect(counterWorker).toBe(1);
-        expect(counterTest === 2 || counterTest === 3).toBe(true);
+        expect(counterTest === 1 || counterTest === 2).toBe(true);
       });
       test('test 1', async ({}) => {
         expect(counterWorker).toBe(1);
-        expect(counterTest).toBe(2);
+        expect(counterTest).toBe(1);
       });
       test('test 2', async ({}) => {
         expect(counterWorker).toBe(1);
-        expect(counterTest).toBe(3);
+        expect(counterTest).toBe(2);
       });
       test.afterEach(async ({}) => {
         expect(counterWorker).toBe(1);
-        expect(counterTest === 2 || counterTest === 3).toBe(true);
+        expect(counterTest === 1 || counterTest === 2).toBe(true);
       });
       test.afterAll(async ({}) => {
         expect(counterWorker).toBe(1);
-        expect(counterTest).toBe(4);
+        expect(counterTest).toBe(2);
       });
     `
   });

tests/playwright-test/hooks.spec.ts

@@ -63,14 +63,10 @@ test('hooks should work with fixtures', async ({ runInlineTest }) => {
           '+w',
           '+t',
           'beforeAll-17-42',
-          '-t',
+          'beforeEach-17-42',
-          '+t',
+          'test-17-42',
-          'beforeEach-17-43',
+          'afterEach-17-42',
-          'test-17-43',
+          'afterAll-17-42',
-          'afterEach-17-43',
-          '-t',
-          '+t',
-          'afterAll-17-44',
           '-t',
           '+t',
         ]);
@@ -95,13 +91,13 @@ test('afterEach failure should not prevent other hooks and fixtures teardown', a
     'a.test.js': `
       const { test } = require('./helper');
       test.describe('suite', () => {
-        test.afterEach(async () => {
-          console.log('afterEach1');
-        });
         test.afterEach(async () => {
           console.log('afterEach2');
           throw new Error('afterEach2');
         });
+        test.afterEach(async () => {
+          console.log('afterEach1');
+        });
         test('one', async ({foo}) => {
           console.log('test');
           expect(true).toBe(true);
@@ -313,7 +309,7 @@ test('beforeAll hook should get retry index of the first test', async ({ runInli
   ]);
 });
 
-test('afterAll exception should fail the run', async ({ runInlineTest }) => {
+test('afterAll exception should fail the test', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.js': `
       const { test } = pwt;
@@ -325,7 +321,8 @@ test('afterAll exception should fail the run', async ({ runInlineTest }) => {
     `,
   });
   expect(result.exitCode).toBe(1);
-  expect(result.passed).toBe(1);
+  expect(result.passed).toBe(0);
+  expect(result.failed).toBe(1);
   expect(result.output).toContain('From the afterAll');
 });
 
@@ -370,13 +367,17 @@ test('beforeAll failure should prevent the test, but not afterAll', async ({ run
       test.afterAll(() => {
         console.log('\\n%%afterAll');
       });
+      test('failed', () => {
+        console.log('\\n%%test1');
+      });
       test('skipped', () => {
-        console.log('\\n%%test');
+        console.log('\\n%%test2');
       });
     `,
   });
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
+  expect(result.skipped).toBe(1);
   expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
     '%%beforeAll',
     '%%afterAll',
@@ -454,7 +455,7 @@ test('afterAll error should not mask beforeAll', async ({ runInlineTest }) => {
   expect(result.output).toContain('from beforeAll');
 });
 
-test('beforeAll timeout should be reported', async ({ runInlineTest }) => {
+test('beforeAll timeout should be reported and prevent more tests', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.js': `
       const { test } = pwt;
@@ -465,41 +466,61 @@ test('beforeAll timeout should be reported', async ({ runInlineTest }) => {
       test.afterAll(() => {
         console.log('\\n%%afterAll');
       });
+      test('failed', () => {
+        console.log('\\n%%test1');
+      });
       test('skipped', () => {
-        console.log('\\n%%test');
+        console.log('\\n%%test2');
       });
     `,
   }, { timeout: 1000 });
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
+  expect(result.skipped).toBe(1);
   expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
     '%%beforeAll',
     '%%afterAll',
   ]);
   expect(result.output).toContain('Timeout of 1000ms exceeded in beforeAll hook.');
+  expect(result.output).toContain(`a.test.js:6:12`);
+  expect(stripAnsi(result.output)).toContain(`> 6 |       test.beforeAll(async () => {`);
 });
 
-test('afterAll timeout should be reported', async ({ runInlineTest }, testInfo) => {
+test('afterAll timeout should be reported, run other afterAll hooks, and continue testing', async ({ runInlineTest }, testInfo) => {
   const result = await runInlineTest({
     'a.test.js': `
       const { test } = pwt;
+      test.describe('suite', () => {
         test.afterAll(async () => {
-        console.log('\\n%%afterAll');
+          console.log('\\n%%afterAll1');
           await new Promise(f => setTimeout(f, 5000));
         });
         test('runs', () => {
-        console.log('\\n%%test');
+          console.log('\\n%%test1');
+        });
+      });
+      test.afterAll(async () => {
+        console.log('\\n%%afterAll2');
+      });
+      test('does not run', () => {
+        console.log('\\n%%test2');
       });
     `,
   }, { timeout: 1000 });
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.skipped).toBe(0);
   expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
-    '%%test',
+    '%%test1',
-    '%%afterAll',
+    '%%afterAll1',
+    '%%afterAll2',
+    '%%test2',
+    '%%afterAll2',
   ]);
   expect(result.output).toContain('Timeout of 1000ms exceeded in afterAll hook.');
-  expect(result.output).toContain(`at a.test.js:6:12`);
+  expect(result.output).toContain(`a.test.js:7:14`);
+  expect(stripAnsi(result.output)).toContain(`>  7 |         test.afterAll(async () => {`);
 });
 
 test('beforeAll and afterAll timeouts at the same time should be reported', async ({ runInlineTest }) => {
@@ -606,13 +627,46 @@ test('should not hang and report results when worker process suddenly exits duri
   const result = await runInlineTest({
     'a.spec.js': `
       const { test } = pwt;
-      test('passed', () => {});
+      test('failing due to afterall', () => {});
       test.afterAll(() => { process.exit(0); });
     `
   }, { reporter: 'line' });
   expect(result.exitCode).toBe(1);
-  expect(result.passed).toBe(1);
+  expect(result.passed).toBe(0);
+  expect(result.failed).toBe(1);
   expect(result.output).toContain('Worker process exited unexpectedly');
-  expect(stripAnsi(result.output)).toContain('[1/1] a.spec.js:6:7 › passed');
+  expect(stripAnsi(result.output)).toContain('[1/1] a.spec.js:6:7 › failing due to afterall');
-  expect(stripAnsi(result.output)).toContain('[1/1] a.spec.js:7:12 › afterAll');
+});
+
+test('unhandled rejection during beforeAll should be reported and prevent more tests', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.js': `
+      const { test } = pwt;
+      test.beforeAll(async () => {
+        console.log('\\n%%beforeAll');
+        Promise.resolve().then(() => {
+          throw new Error('Oh my');
+        });
+        await new Promise(f => setTimeout(f, 100));
+      });
+      test.afterAll(() => {
+        console.log('\\n%%afterAll');
+      });
+      test('failed', () => {
+        console.log('\\n%%test1');
+      });
+      test('skipped', () => {
+        console.log('\\n%%test2');
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.skipped).toBe(1);
+  expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%beforeAll',
+    '%%afterAll',
+  ]);
+  expect(result.output).toContain('Error: Oh my');
+  expect(stripAnsi(result.output)).toContain(`>  9 |           throw new Error('Oh my');`);
 });

tests/playwright-test/playwright.artifacts.spec.ts

@@ -46,6 +46,7 @@ const testFiles = {
       });
 
       test.afterAll(async () => {
+        await page.setContent('Reset!');
         await page.close();
       });
 
@@ -145,10 +146,6 @@ test('should work with screenshot: on', async ({ runInlineTest }, testInfo) => {
     '  test-failed-1.png',
     'artifacts-persistent-passing',
     '  test-finished-1.png',
-    'artifacts-shared-afterAll-worker0',
-    '  test-finished-1.png',
-    'artifacts-shared-beforeAll-worker0',
-    '  test-finished-1.png',
     'artifacts-shared-shared-failing',
     '  test-failed-1.png',
     'artifacts-shared-shared-passing',
@@ -214,10 +211,6 @@ test('should work with trace: on', async ({ runInlineTest }, testInfo) => {
     '  trace.zip',
     'artifacts-persistent-passing',
     '  trace.zip',
-    'artifacts-shared-afterAll-worker0',
-    '  trace.zip',
-    'artifacts-shared-beforeAll-worker0',
-    '  trace.zip',
     'artifacts-shared-shared-failing',
     '  trace.zip',
     'artifacts-shared-shared-passing',
@@ -277,8 +270,6 @@ test('should work with trace: on-first-retry', async ({ runInlineTest }, testInf
     '  trace.zip',
     'artifacts-persistent-failing-retry1',
     '  trace.zip',
-    'artifacts-shared-beforeAll-worker1-retry1',
-    '  trace.zip',
     'artifacts-shared-shared-failing-retry1',
     '  trace.zip',
     'artifacts-two-contexts-failing-retry1',

tests/playwright-test/reporter.spec.ts

@@ -343,15 +343,11 @@ test('should report api steps', async ({ runInlineTest }) => {
     `%% end {\"title\":\"browserContext.close\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"After Hooks\",\"category\":\"hook\",\"steps\":[{\"title\":\"apiRequestContext.dispose\",\"category\":\"pw:api\"},{\"title\":\"browserContext.close\",\"category\":\"pw:api\"}]}`,
     `%% begin {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
-    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"browser.newPage\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"browser.newPage\",\"category\":\"pw:api\"}`,
     `%% begin {\"title\":\"page.setContent\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"page.setContent\",\"category\":\"pw:api\"}`,
-    `%% begin {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
+    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\",\"steps\":[{\"title\":\"browser.newPage\",\"category\":\"pw:api\"},{\"title\":\"page.setContent\",\"category\":\"pw:api\"}]}`,
-    `%% end {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
-    `%% begin {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
-    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"page.click(button)\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"page.click(button)\",\"category\":\"pw:api\"}`,
     `%% begin {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
@@ -361,13 +357,9 @@ test('should report api steps', async ({ runInlineTest }) => {
     `%% begin {\"title\":\"page.click(button)\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"page.click(button)\",\"category\":\"pw:api\"}`,
     `%% begin {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
-    `%% end {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
-    `%% begin {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
-    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"page.close\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"page.close\",\"category\":\"pw:api\"}`,
-    `%% begin {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
+    `%% end {\"title\":\"After Hooks\",\"category\":\"hook\",\"steps\":[{\"title\":\"page.close\",\"category\":\"pw:api\"}]}`,
-    `%% end {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
   ]);
 });
 

tests/playwright-test/test-modifiers.spec.ts

@@ -330,6 +330,38 @@ test('modifier timeout should be reported', async ({ runInlineTest }) => {
   }, { timeout: 2000 });
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
-  expect(result.output).toContain('Error: Timeout of 2000ms exceeded while running skip modifier');
+  expect(result.output).toContain('Timeout of 2000ms exceeded in skip modifier.');
   expect(stripAnsi(result.output)).toContain('6 |       test.skip(async () => new Promise(() => {}));');
 });
+
+test('should not run hooks if modifier throws', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      const { test } = pwt;
+      test.skip(() => {
+        console.log('%%modifier');
+        throw new Error('Oh my');
+      });
+      test.beforeAll(() => {
+        console.log('%%beforeEach');
+      });
+      test.beforeEach(() => {
+        console.log('%%beforeEach');
+      });
+      test.afterEach(() => {
+        console.log('%%afterEach');
+      });
+      test.afterAll(() => {
+        console.log('%%beforeEach');
+      });
+      test('skipped1', () => {
+        console.log('%%skipped1');
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%modifier',
+  ]);
+});

tests/playwright-test/test-output-dir.spec.ts

@@ -83,53 +83,27 @@ test('should include repeat token', async ({ runInlineTest }) => {
   expect(result.passed).toBe(3);
 });
 
-test('should be unique for beforeAll and afterAll hooks', async ({ runInlineTest }, testInfo) => {
+test('should be unique for beforeAll hook from different workers', async ({ runInlineTest }, testInfo) => {
   const result = await runInlineTest({
     'a.spec.js': `
       const { test } = pwt;
       test.beforeAll(({}, testInfo) => {
         console.log('\\n%%' + testInfo.outputDir);
       });
-      test.beforeAll(({}, testInfo) => {
-        console.log('\\n%%' + testInfo.outputDir);
-      });
-      test.afterAll(({}, testInfo) => {
-        console.log('\\n%%' + testInfo.outputDir);
-      });
-      test.afterAll(({}, testInfo) => {
-        console.log('\\n%%' + testInfo.outputDir);
-      });
-      test.describe('suite', () => {
-        test.beforeAll(({}, testInfo) => {
-          console.log('\\n%%' + testInfo.outputDir);
-        });
-        test.afterAll(({}, testInfo) => {
-          console.log('\\n%%' + testInfo.outputDir);
-        });
       test('fails', ({}, testInfo) => {
         expect(1).toBe(2);
       });
       test('passes', ({}, testInfo) => {
       });
-      });
     `
-  });
+  }, { retries: '1' });
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(1);
   expect(result.failed).toBe(1);
   expect(result.output.split('\n').filter(x => x.startsWith('%%'))).toEqual([
-    `%%${testInfo.outputPath('test-results', 'a-beforeAll-worker0')}`,
+    `%%${testInfo.outputPath('test-results', 'a-fails')}`,
-    `%%${testInfo.outputPath('test-results', 'a-beforeAll1-worker0')}`,
+    `%%${testInfo.outputPath('test-results', 'a-fails-retry1')}`,
-    `%%${testInfo.outputPath('test-results', 'a-suite-beforeAll-worker0')}`,
+    `%%${testInfo.outputPath('test-results', 'a-passes')}`,
-    `%%${testInfo.outputPath('test-results', 'a-suite-afterAll-worker0')}`,
-    `%%${testInfo.outputPath('test-results', 'a-afterAll-worker0')}`,
-    `%%${testInfo.outputPath('test-results', 'a-afterAll1-worker0')}`,
-    `%%${testInfo.outputPath('test-results', 'a-beforeAll-worker1')}`,
-    `%%${testInfo.outputPath('test-results', 'a-beforeAll1-worker1')}`,
-    `%%${testInfo.outputPath('test-results', 'a-suite-beforeAll-worker1')}`,
-    `%%${testInfo.outputPath('test-results', 'a-suite-afterAll-worker1')}`,
-    `%%${testInfo.outputPath('test-results', 'a-afterAll-worker1')}`,
-    `%%${testInfo.outputPath('test-results', 'a-afterAll1-worker1')}`,
   ]);
 });