From 388a3e1f37809faf0a3c9ae8232377f949c86d70 Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Wed, 4 Jan 2023 14:13:49 -0800
Subject: [PATCH] fix(test runner): make sure to run afterAll after skipped
 tests (#19878)

Fixes #19745.
---
 packages/playwright-test/src/workerRunner.ts |  9 ++++---
 tests/playwright-test/hooks.spec.ts          | 28 ++++++++++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerRunner.ts
index 0e7e6fbcb1..aab51c4bf4 100644
--- a/packages/playwright-test/src/workerRunner.ts
+++ b/packages/playwright-test/src/workerRunner.ts
@@ -289,6 +289,7 @@ export class WorkerRunner extends EventEmitter {
 
     const suites = getSuites(test);
     const reversedSuites = suites.slice().reverse();
+    const nextSuites = new Set(getSuites(nextTest));
 
     // Inherit test.setTimeout() from parent suites, deepest has the priority.
     for (const suite of reversedSuites) {
@@ -312,8 +313,11 @@ export class WorkerRunner extends EventEmitter {
     this.emit('testBegin', buildTestBeginPayload(testInfo));
 
     const isSkipped = testInfo.expectedStatus === 'skipped';
-    if (isSkipped && nextTest) {
-      // Fast path - this test and skipped, and there are more tests that will handle cleanup.
+    const hasAfterAllToRunBeforeNextTest = reversedSuites.some(suite => {
+      return this._activeSuites.has(suite) && !nextSuites.has(suite) && suite._hooks.some(hook => hook.type === 'afterAll');
+    });
+    if (isSkipped && nextTest && !hasAfterAllToRunBeforeNextTest) {
+      // Fast path - this test is skipped, and there are more tests that will handle cleanup.
       testInfo.status = 'skipped';
       this.emit('testEnd', buildTestEndPayload(testInfo));
       return;
@@ -445,7 +449,6 @@ export class WorkerRunner extends EventEmitter {
       }
 
       // Run "afterAll" hooks for suites that are not shared with the next test.
-      const nextSuites = new Set(getSuites(nextTest));
       // In case of failure the worker will be stopped and we have to make sure that afterAll
       // hooks run before test fixtures teardown.
       for (const suite of reversedSuites) {
diff --git a/tests/playwright-test/hooks.spec.ts b/tests/playwright-test/hooks.spec.ts
index 31dbfcec7f..3c774e90ae 100644
--- a/tests/playwright-test/hooks.spec.ts
+++ b/tests/playwright-test/hooks.spec.ts
@@ -844,3 +844,31 @@ test('afterEach exception after skipped test should be reported', async ({ runIn
   expect(result.failed).toBe(1);
   expect(result.output).toContain('Error: oh my!');
 });
+
+test('afterAll should be run for test.skip', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.js': `
+      const { test } = pwt;
+      test.describe('suite1', () => {
+        test.beforeAll(() => console.log('\\n%%beforeAll1'));
+        test.afterAll(() => console.log('\\n%%afterAll1'));
+        test('test1', () => console.log('\\n%%test1'));
+        test.skip('test2', () => {});
+        test.skip('test2.5', () => {});
+      });
+      test.describe('suite2', () => {
+        test.beforeAll(() => console.log('\\n%%beforeAll2'));
+        test.afterAll(() => console.log('\\n%%afterAll2'));
+        test('test3', () => console.log('\\n%%test3'));
+      });
+    `,
+  });
+  expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%beforeAll1',
+    '%%test1',
+    '%%afterAll1',
+    '%%beforeAll2',
+    '%%test3',
+    '%%afterAll2',
+  ]);
+});