From 43691c3468bb5bdcb1f9fe78d89ae88dcf5479fb Mon Sep 17 00:00:00 2001
From: Simon Knott <info@simonknott.de>
Date: Tue, 5 Nov 2024 08:24:36 +0100
Subject: [PATCH] fix(global setup): simplify ordering

---
 packages/playwright/src/runner/tasks.ts    | 46 ++++++++++++----------
 tests/playwright-test/global-setup.spec.ts | 14 +++----
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/packages/playwright/src/runner/tasks.ts b/packages/playwright/src/runner/tasks.ts
index 528cac47cd..e122b8ea7e 100644
--- a/packages/playwright/src/runner/tasks.ts
+++ b/packages/playwright/src/runner/tasks.ts
@@ -97,12 +97,11 @@ export function createGlobalSetupTasks(config: FullConfigInternal) {
   const tasks: Task<TestRun>[] = [];
   if (!config.configCLIOverrides.preserveOutputDir && !process.env.PW_TEST_NO_REMOVE_OUTPUT_DIRS)
     tasks.push(createRemoveOutputDirsTask());
-  tasks.push(...createPluginSetupTasks(config));
-  if (config.globalSetups.length || config.globalTeardowns.length) {
-    const length = Math.max(config.globalSetups.length, config.globalTeardowns.length);
-    for (let i = 0; i < length; i++)
-      tasks.push(createGlobalSetupTask(i, length));
-  }
+  tasks.push(
+      ...createPluginSetupTasks(config),
+      ...config.globalTeardowns.toReversed().map(file => createGlobalTeardownTask(file, config)),
+      ...config.globalSetups.map(file => createGlobalSetupTask(file, config)),
+  );
   return tasks;
 }
 
@@ -164,28 +163,35 @@ function createPluginBeginTask(plugin: TestRunnerPluginRegistration): Task<TestR
   };
 }
 
-function createGlobalSetupTask(index: number, length: number): Task<TestRun> {
-  let globalSetupResult: any;
-  let globalSetupFinished = false;
-  let teardownHook: any;
-
+function createGlobalSetupTask(file: string, config: FullConfigInternal): Task<TestRun> {
   let title = 'global setup';
-  if (length > 1)
-    title += ` #${index}`;
+  if (config.globalSetups.length > 1)
+    title += ` (${file})`;
 
+  let globalSetupResult: any;
   return {
     title,
     setup: async ({ config }) => {
-      const setupHook = config.globalSetups[index] ? await loadGlobalHook(config, config.globalSetups[index]) : undefined;
-      teardownHook = config.globalTeardowns[index] ? await loadGlobalHook(config, config.globalTeardowns[index]) : undefined;
-      globalSetupResult = setupHook ? await setupHook(config.config) : undefined;
-      globalSetupFinished = true;
+      const setupHook = await loadGlobalHook(config, file);
+      globalSetupResult = await setupHook(config.config);
     },
-    teardown: async ({ config }) => {
+    teardown: async () => {
       if (typeof globalSetupResult === 'function')
         await globalSetupResult();
-      if (globalSetupFinished)
-        await teardownHook?.(config.config);
+    },
+  };
+}
+
+function createGlobalTeardownTask(file: string, config: FullConfigInternal): Task<TestRun> {
+  let title = 'global teardown';
+  if (config.globalTeardowns.length > 1)
+    title += ` (${file})`;
+
+  return {
+    title,
+    teardown: async ({ config }) => {
+      const teardownHook = await loadGlobalHook(config, file);
+      await teardownHook(config.config);
     },
   };
 }
diff --git a/tests/playwright-test/global-setup.spec.ts b/tests/playwright-test/global-setup.spec.ts
index f1bd7b7458..47f44b8d5e 100644
--- a/tests/playwright-test/global-setup.spec.ts
+++ b/tests/playwright-test/global-setup.spec.ts
@@ -108,7 +108,7 @@ test('globalTeardown runs after failures', async ({ runInlineTest }) => {
   expect(output).toContain('teardown=42');
 });
 
-test('globalTeardown does not run when globalSetup times out', async ({ runInlineTest }) => {
+test('globalTeardown still runs when globalSetup times out', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'playwright.config.ts': `
       import * as path from 'path';
@@ -135,7 +135,7 @@ test('globalTeardown does not run when globalSetup times out', async ({ runInlin
     `,
   });
   expect(result.output).toContain('Timed out waiting 1s for the global setup to run');
-  expect(result.output).not.toContain('teardown=');
+  expect(result.output).toContain('teardown=');
 });
 
 test('globalSetup should work with sync function', async ({ runInlineTest }) => {
@@ -395,9 +395,9 @@ test('globalSetup should support multiple', async ({ runInlineTest }) => {
         globalTeardown: ['./globalTeardown1.ts', './globalTeardown2.ts'],
       };
     `,
-    'globalSetup1.ts': `module.exports = () => { console.log('%%globalSetup1'); return () => { console.log('%%globalSetup1Function'); throw new Error('kaboom'); } };`,
+    'globalSetup1.ts': `module.exports = () => { console.log('%%globalSetup1'); return () => { console.log('%%callback1'); throw new Error('kaboom'); } };`,
     'globalSetup2.ts': `module.exports = () => console.log('%%globalSetup2');`,
-    'globalSetup3.ts': `module.exports = () => { console.log('%%globalSetup3'); return () => console.log('%%globalSetup3Function'); }`,
+    'globalSetup3.ts': `module.exports = () => { console.log('%%globalSetup3'); return () => console.log('%%callback3'); }`,
     'globalSetup4.ts': `module.exports = () => console.log('%%globalSetup4');`,
     'globalTeardown1.ts': `module.exports = () => console.log('%%globalTeardown1')`,
     'globalTeardown2.ts': `module.exports = () => { console.log('%%globalTeardown2'); throw new Error('kaboom'); }`,
@@ -419,10 +419,10 @@ test('globalSetup should support multiple', async ({ runInlineTest }) => {
     'globalSetup4',
     'test a',
     'test b',
-    'globalSetup3Function',
+    'callback3',
+    'callback1',
+    'globalTeardown1',
     'globalTeardown2',
-    'globalSetup1Function',
-    // 'globalTeardown1' is missing, because globalSetup1Function errored out.
   ]);
   expect(result.output).toContain('Error: kaboom');
 });