fix(global setup): simplify ordering (#33444)

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.map(file => createGlobalTeardownTask(file, config)).reverse(),
+      ...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);
     },
   };
 }

tests/playwright-test/esm.spec.ts

@@ -676,9 +676,6 @@ test('should be able to use use execSync with a Node.js file inside a spec', asy
     'global-setup import level',
     'execSync: hello from hello.js',
     'spawnSync: hello from hello.js',
-    'global-teardown import level',
-    'execSync: hello from hello.js',
-    'spawnSync: hello from hello.js',
     'global-setup export level',
     'execSync: hello from hello.js',
     'spawnSync: hello from hello.js',
@@ -693,6 +690,9 @@ test('should be able to use use execSync with a Node.js file inside a spec', asy
     'execSync: hello from hello.js',
     'spawnSync: hello from hello.js',
     'fork: hello from hellofork.js',
+    'global-teardown import level',
+    'execSync: hello from hello.js',
+    'spawnSync: hello from hello.js',
     'global-teardown export level',
     'execSync: hello from hello.js',
     'spawnSync: hello from hello.js',

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'); }`,
@@ -410,8 +410,8 @@ test('globalSetup should support multiple', async ({ runInlineTest }) => {
   }, { reporter: 'line' });
   expect(result.passed).toBe(2);
 
-  // behaviour: setups in order, teardowns in reverse order.
-  // setup-returned functions inherit their position, and take precedence over `globalTeardown` scripts.
+  // first setups, then setup callbacks in reverse order.
+  // then teardowns in declared order.
   expect(result.outputLines).toEqual([
     'globalSetup1',
     'globalSetup2',
@@ -419,10 +419,38 @@ 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');
+});
+
+test('globalTeardown runs even if callback failed', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = {
+        globalSetup: './globalSetup.ts',
+        globalTeardown: './globalTeardown.ts',
+      };
+    `,
+    'globalSetup.ts': `module.exports = () => { console.log('%%globalSetup'); return () => { throw new Error('kaboom'); } };`,
+    'globalTeardown.ts': `module.exports = () => console.log('%%globalTeardown')`,
+
+    'a.test.js': `
+      import { test } from '@playwright/test';
+      test('a', () => console.log('%%test'));
+    `,
+  }, { reporter: 'line' });
+  expect(result.passed).toBe(1);
+
+  // first setups, then setup callbacks in reverse order.
+  // then teardowns in declared order.
+  expect(result.outputLines).toEqual([
+    'globalSetup',
+    'test',
+    'globalTeardown',
   ]);
   expect(result.output).toContain('Error: kaboom');
 });

tests/playwright-test/runner.spec.ts

@@ -442,7 +442,7 @@ test('sigint should stop global setup', async ({ interactWithTestRunner }) => {
   const result = parseTestRunnerOutput(testProcess.output);
   expect(result.passed).toBe(0);
   expect(result.output).toContain('Global setup');
-  expect(result.output).not.toContain('Global teardown');
+  expect(result.output).toContain('Global teardown');
 });
 
 test('sigint should stop plugins', async ({ interactWithTestRunner }) => {