support tags in worker hooks

src/generate/formatter.ts

@@ -74,13 +74,14 @@ export class Formatter {
   }
 
   workerHooksCall(type: WorkerHookType, fixturesNames: Set<string>, bddDataVar: string) {
-    const runWorkerHooksFixture = '$runWorkerHooks';
+    const runWorkerHooksFixture =
+      type === 'beforeAll' ? '$runBeforeAllHooks' : '$registerAfterAllHooks';
     const fixturesStr = [...fixturesNames].join(', ');
     const allFixturesStr = [runWorkerHooksFixture, ...fixturesNames].join(', ');
     const title = type === 'beforeAll' ? 'BeforeAll Hooks' : 'AfterAll Hooks';
     return [
       // eslint-disable-next-line max-len
-      `test.${type}(${this.quoted(title)}, ({ ${allFixturesStr} }) => ${runWorkerHooksFixture}(test, ${this.quoted(type)}, { ${fixturesStr} }, ${bddDataVar}));`,
+      `test.${type}(${this.quoted(title)}, ({ ${allFixturesStr} }) => ${runWorkerHooksFixture}(test, { ${fixturesStr} }, ${bddDataVar}));`,
     ];
   }
 

src/generate/hooks.ts

@@ -27,7 +27,8 @@ import {
   WorkerHook,
   WorkerHookType,
 } from '../hooks/worker';
-import { toBoolean } from '../utils';
+import { areSetsEqual, toBoolean } from '../utils';
+import { exit } from '../utils/exit';
 import { Formatter } from './formatter';
 import { TestGen } from './test';
 
@@ -41,10 +42,11 @@ export class TestFileHooks {
 
   fillFromTests(tests: TestGen[]) {
     tests.forEach((test) => {
-      this.beforeAll.registerHooksForTest(test.tags);
-      this.afterAll.registerHooksForTest(test.tags);
-      this.before.registerHooksForTest(test.tags);
-      this.after.registerHooksForTest(test.tags);
+      // todo: filter skipped tests
+      this.beforeAll.registerHooksForTest(test);
+      this.afterAll.registerHooksForTest(test);
+      this.before.registerHooksForTest(test);
+      this.after.registerHooksForTest(test);
     });
   }
 
@@ -77,8 +79,8 @@ class TestFileScenarioHooks<T extends ScenarioHookType> {
     private formatter: Formatter,
   ) {}
 
-  registerHooksForTest(testTags: string[]) {
-    getScenarioHooksToRun(this.type, testTags).forEach((hook) => this.hooks.add(hook));
+  registerHooksForTest(test: TestGen) {
+    getScenarioHooksToRun(this.type, test.tags).forEach((hook) => this.hooks.add(hook));
   }
 
   getCustomTestInstances() {
@@ -97,14 +99,27 @@ class TestFileScenarioHooks<T extends ScenarioHookType> {
 
 class TestFileWorkerHooks<T extends WorkerHookType> {
   private hooks = new Set<WorkerHook>();
+  private tests: TestGen[] = [];
 
   constructor(
     private type: T,
     private formatter: Formatter,
   ) {}
 
-  registerHooksForTest(testTags: string[]) {
-    getWorkerHooksToRun(this.type, testTags).forEach((hook) => this.hooks.add(hook));
+  registerHooksForTest(test: TestGen) {
+    /**
+     * For worker hooks (beforeAll, afterAll) we require
+     * that each test match exactly the same set of hooks.
+     * Otherwise, in fully-parallel mode, we will run all worker hooks
+     * in each worker for each test, that actually makes test-level tags useless.
+     */
+    const hooksForTest = new Set(getWorkerHooksToRun(this.type, test.tags));
+    if (this.tests.length === 0) {
+      this.hooks = hooksForTest;
+    } else {
+      this.ensureHooksEqual(test, hooksForTest);
+    }
+    this.tests.push(test);
   }
 
   getCustomTestInstances() {
@@ -120,4 +135,17 @@ class TestFileWorkerHooks<T extends WorkerHookType> {
       BddDataRenderer.varName,
     );
   }
+
+  private ensureHooksEqual(test: TestGen, hooksForTest: Set<WorkerHook>) {
+    if (areSetsEqual(this.hooks, hooksForTest)) return;
+    const prevTest = this.tests.at(-1)!;
+    exit(
+      [
+        `Tagged ${this.type} hooks should be the same for all scenarios in a feature.`,
+        `Feature: ${test.featureUri}`,
+        `  - ${this.hooks.size} hook(s): ${prevTest.testTitle} ${prevTest.tags.join(' ')}`,
+        `  - ${hooksForTest.size} hook(s): ${test.testTitle} ${test.tags.join(' ')}`,
+      ].join('\n'),
+    );
+  }
 }

src/generate/test/index.ts

@@ -49,13 +49,13 @@ export class TestGen {
   // eslint-disable-next-line max-params
   constructor(
     private config: BDDConfig,
-    private featureUri: string,
+    public featureUri: string,
     private i18nKeywordsMap: KeywordsMap | undefined,
     private stepFinder: StepFinder,
     private formatter: Formatter,
     private backgrounds: BackgroundGen[],
     public pickle: PickleWithLocation,
-    private testTitle: string,
+    public testTitle: string,
     private scenarioSteps: readonly Step[],
     ownTestTags: string[],
   ) {

src/hooks/scenario.ts

@@ -79,15 +79,16 @@ export function createBeforeAfter<
 }
 
 // eslint-disable-next-line visual/complexity
-export async function runScenarioHooks(type: ScenarioHookType, fixtures: ScenarioHookFixtures) {
-  const hooksToRun = getScenarioHooksToRun(type, fixtures.$bddContext.tags);
-
+export async function runScenarioHooks(
+  hooks: GeneralScenarioHook[],
+  fixtures: ScenarioHookFixtures,
+) {
   let error;
-  for (const hook of hooksToRun) {
+  for (const hook of hooks) {
     try {
       await runScenarioHook(hook, fixtures);
     } catch (e) {
-      if (type === 'before') throw e;
+      if (hook.type === 'before') throw e;
       if (!error) error = e;
     }
   }
@@ -155,12 +156,7 @@ function setTagsExpression(hook: GeneralScenarioHook) {
 
 function addHook(hook: GeneralScenarioHook) {
   setTagsExpression(hook);
-  if (hook.type === 'before') {
-    scenarioHooks.push(hook);
-  } else {
-    // 'after' hooks run in reverse order
-    scenarioHooks.unshift(hook);
-  }
+  scenarioHooks.push(hook);
 }
 
 function getTimeoutMessage(hook: GeneralScenarioHook) {

src/hooks/worker.ts

@@ -11,7 +11,6 @@ import { KeyValue, PlaywrightLocation, TestTypeCommon } from '../playwright/type
 import { callWithTimeout } from '../utils';
 import { getLocationByOffset } from '../playwright/getLocationInFile';
 import { runStepWithLocation } from '../playwright/runStepWithLocation';
-import { BddFileData } from '../bddData/types';
 
 export type WorkerHookType = 'beforeAll' | 'afterAll';
 
@@ -40,6 +39,12 @@ export type WorkerHook<Fixtures = object> = {
   executed?: boolean;
 };
 
+export type WorkerHookRunInfo = {
+  test: TestTypeCommon;
+  hook: WorkerHook;
+  fixtures: WorkerHookFixtures;
+};
+
 /**
  * When calling BeforeAll() / AfterAll() you can pass:
  * 1. hook fn
@@ -73,37 +78,32 @@ export function createBeforeAllAfterAll<Fixtures extends KeyValue>(
   };
 }
 
-// eslint-disable-next-line visual/complexity, max-params
-export async function runWorkerHooks(
-  test: TestTypeCommon,
-  type: WorkerHookType,
-  fixtures: WorkerHookFixtures,
-  bddFileData: BddFileData,
-) {
-  // collect worker hooks for all tests in the file
-  const hooksToRun = new Set<WorkerHook>();
-  bddFileData.forEach((bddTestData) => {
-    getWorkerHooksToRun(type, bddTestData.tags).forEach((hook) => hooksToRun.add(hook));
-  });
-
+// eslint-disable-next-line visual/complexity
+export async function runWorkerHooks(hooksRunInfo: Map<WorkerHook, WorkerHookRunInfo>) {
   let error;
-  for (const hook of hooksToRun) {
+  for (const runInfo of hooksRunInfo.values()) {
     try {
-      await runWorkerHook(test, hook, fixtures);
+      await runWorkerHook(runInfo);
     } catch (e) {
-      if (type === 'beforeAll') throw e;
+      if (runInfo.hook.type === 'beforeAll') throw e;
       if (!error) error = e;
     }
   }
   if (error) throw error;
 }
 
-async function runWorkerHook(test: TestTypeCommon, hook: WorkerHook, fixtures: WorkerHookFixtures) {
-  if (!hook.executed) {
-    hook.executed = true;
-    const hookFn = wrapHookFnWithTimeout(hook, fixtures);
-    const stepTitle = getHookStepTitle(hook);
+async function runWorkerHook({ test, hook, fixtures }: WorkerHookRunInfo) {
+  if (hook.executed) return;
+  hook.executed = true;
+  const hookFn = wrapHookFnWithTimeout(hook, fixtures);
+  const stepTitle = getHookStepTitle(hook);
+  // test.step() is not available for afterAll hooks.
+  // See: https://github.com/microsoft/playwright/issues/33750
+  // So all afterAll hooks are called under AfterAll step (with type = 'hook' in reporter)
+  if (hook.type === 'beforeAll') {
     await runStepWithLocation(test, stepTitle, hook.location, hookFn);
+  } else {
+    await hookFn();
   }
 }
 
@@ -150,12 +150,7 @@ function getFnFromArgs(args: unknown[]) {
 
 function addHook(hook: WorkerHook) {
   setTagsExpression(hook);
-  if (hook.type === 'beforeAll') {
-    workerHooks.push(hook);
-  } else {
-    // 'afterAll' hooks run in reverse order
-    workerHooks.unshift(hook);
-  }
+  workerHooks.push(hook);
 }
 
 function getTimeoutMessage(hook: WorkerHook) {

src/run/bddFixtures/test.ts

@@ -6,7 +6,7 @@
 
 import { test as base } from './worker';
 import { BDDConfig } from '../../config/types';
-import { runScenarioHooks } from '../../hooks/scenario';
+import { getScenarioHooksToRun, runScenarioHooks } from '../../hooks/scenario';
 import { createStepInvoker, StepKeywordFixture } from '../StepInvoker';
 import { TestTypeCommon } from '../../playwright/types';
 import { TestInfo } from '@playwright/test';
@@ -145,17 +145,20 @@ export const test = base.extend<BddFixturesTest>({
   // unused dependency '$afterEach' is important to run afterEach
   // in case of error in beforeEach.
   $beforeEach: [
-    async ({ $bddContext, $beforeEachFixtures, $afterEach }, use) => {
-      await runScenarioHooks('before', { $bddContext, ...$beforeEachFixtures });
+    async ({ $bddContext, $beforeEachFixtures, $tags, $afterEach }, use) => {
+      const hooksToRun = getScenarioHooksToRun('before', $tags);
+      await runScenarioHooks(hooksToRun, { $bddContext, ...$beforeEachFixtures });
       await use();
     },
     fixtureOptions,
   ],
   // runs afterEach hooks
   $afterEach: [
-    async ({ $bddContext, $afterEachFixtures }, use) => {
+    async ({ $bddContext, $afterEachFixtures, $tags }, use) => {
       await use();
-      await runScenarioHooks('after', { $bddContext, ...$afterEachFixtures });
+      const hooksToRun = getScenarioHooksToRun('after', $tags);
+      hooksToRun.reverse();
+      await runScenarioHooks(hooksToRun, { $bddContext, ...$afterEachFixtures });
     },
     fixtureOptions,
   ],

src/run/bddFixtures/worker.ts

@@ -5,8 +5,15 @@
 import { BDDConfig } from '../../config/types';
 import { test as base, WorkerInfo } from '@playwright/test';
 import { getConfigFromEnv } from '../../config/env';
-import { runWorkerHooks } from '../../hooks/worker';
+import {
+  getWorkerHooksToRun,
+  runWorkerHooks,
+  WorkerHook,
+  WorkerHookRunInfo,
+} from '../../hooks/worker';
 import { loadSteps, resolveStepFiles } from '../../steps/loader';
+import { BddFileData } from '../../bddData/types';
+import { TestTypeCommon } from '../../playwright/types';
 
 // BDD fixtures prefixed with '$' to avoid collision with user's fixtures.
 
@@ -15,10 +22,17 @@ import { loadSteps, resolveStepFiles } from '../../steps/loader';
 // make type coercion to satisfy TS in early PW versions
 const fixtureOptions = { scope: 'worker', box: true } as { scope: 'worker' };
 
+type WorkerHooksFixture = (
+  test: TestTypeCommon,
+  fixtures: Record<string, unknown>,
+  bddFileData: BddFileData,
+) => unknown;
+
 export type BddFixturesWorker = {
   $workerInfo: WorkerInfo;
   $bddConfig: BDDConfig;
-  $runWorkerHooks: typeof runWorkerHooks;
+  $runBeforeAllHooks: WorkerHooksFixture;
+  $registerAfterAllHooks: WorkerHooksFixture;
 };
 
 export const test = base.extend<NonNullable<unknown>, BddFixturesWorker>({
@@ -32,12 +46,57 @@ export const test = base.extend<NonNullable<unknown>, BddFixturesWorker>({
     },
     fixtureOptions,
   ],
-  $runWorkerHooks: [
+  $runBeforeAllHooks: [
+    // Important unused dependency:
+    // - $bddConfig: to load bdd config before hooks
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    async ({ $bddConfig }, use, $workerInfo) => {
+      await use(async (test, customFixtures, bddFileData) => {
+        // Collect worker hooks for all tests in the file
+        // In fact, we could use only first test from bddFileData to get hooks,
+        // b/c we require that all tests in the file have the same beforeAll hooks.
+        // todo: filter out skipped tests
+        const hooksToRun = new Map<WorkerHook, WorkerHookRunInfo>();
+        const fixtures = { $workerInfo, ...customFixtures };
+        bddFileData.forEach((bddTestData) => {
+          // eslint-disable-next-line max-nested-callbacks
+          getWorkerHooksToRun('beforeAll', bddTestData.tags).forEach((hook) => {
+            hooksToRun.set(hook, { test, hook, fixtures });
+          });
+        });
+        await runWorkerHooks(hooksToRun);
+      });
+    },
+    fixtureOptions,
+  ],
+  // Execution of AfterAll hooks is different from BeforeAll hooks.
+  // The main challenge is with tagged AfterAll hooks:
+  // We can't detect when the last test for tagged AfterAll hook is executed,
+  // especially when there are several test files in a worker or in fullyParallel mode.
+  // The solution: collect and run all AfterAll hooks in worker teardown phase.
+  // A list of afterAll hooks is populated from test.afterAll() call in each test file.
+
+  $registerAfterAllHooks: [
     // Important unused dependency:
     // - $bddConfig: to load bdd config before hooks
     // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    async ({ $bddConfig }, use) => {
-      await use(runWorkerHooks);
+    async ({ $bddConfig }, use, $workerInfo) => {
+      const hooksToRun = new Map<WorkerHook, WorkerHookRunInfo>();
+
+      await use((test, customFixtures, bddFileData) => {
+        const fixtures = { $workerInfo, ...customFixtures };
+        bddFileData.forEach((bddTestData) => {
+          getWorkerHooksToRun('afterAll', bddTestData.tags)
+            // eslint-disable-next-line max-nested-callbacks
+            .filter((hook) => !hooksToRun.has(hook))
+            // eslint-disable-next-line max-nested-callbacks
+            .forEach((hook) => hooksToRun.set(hook, { test, hook, fixtures }));
+        });
+      });
+
+      // run AfterAll hooks in FILO order
+      const reversedHooksToRun = new Map([...hooksToRun].reverse());
+      await runWorkerHooks(reversedHooksToRun);
     },
     fixtureOptions,
   ],

src/utils/index.ts

@@ -107,3 +107,7 @@ export function saveFileSync(filePath: string, content: string) {
 export function toBoolean<T>(value: T): value is NonNullable<T> {
   return Boolean(value);
 }
+
+export function areSetsEqual<T>(set1: Set<T>, set2: Set<T>) {
+  return set1.size === set2.size && [...set1].every((val) => set2.has(val));
+}

test/_helpers/index.mjs

@@ -9,6 +9,9 @@ import { expect } from '@playwright/test';
 export * from './runPlaywright.mjs';
 export * from './TestDir.mjs';
 
+// At some point we will be able to run test.only()
+// See: https://github.com/nodejs/node/issues/47945
+
 export { test, expect, normalize };
 export const playwrightVersion = getPackageVersion('@playwright/test');