chore: make sure to call task's teardown if it has ever started (#24317)

This way things like WebServerPlugin can cleanup after themselves even
if they failed to start or were interrupted mid-way.

packages/playwright-ct-core/src/vitePlugin.ts

@@ -173,6 +173,7 @@ export function createPlugin(
     },
 
     end: async () => {
+      if (stoppableServer)
         await new Promise(f => stoppableServer.stop(f));
     },
   };

packages/playwright-test/src/runner/taskRunner.ts

@@ -21,8 +21,8 @@ import { SigIntWatcher } from './sigIntWatcher';
 import { serializeError } from '../util';
 import type { ReporterV2 } from '../reporters/reporterV2';
 
-type TaskTeardown = () => Promise<any> | undefined;
+type TaskPhase<Context> = (context: Context, errors: TestError[], softErrors: TestError[]) => Promise<void> | void;
-export type Task<Context> = (context: Context, errors: TestError[], softErrors: TestError[]) => Promise<TaskTeardown | void> | undefined;
+export type Task<Context> = { setup?: TaskPhase<Context>, teardown?: TaskPhase<Context> };
 
 export class TaskRunner<Context> {
   private _tasks: { name: string, task: Task<Context> }[] = [];
@@ -50,7 +50,7 @@ export class TaskRunner<Context> {
   async runDeferCleanup(context: Context, deadline: number, cancelPromise = new ManualPromise<void>()): Promise<{ status: FullResult['status'], cleanup: () => Promise<FullResult['status']> }> {
     const sigintWatcher = new SigIntWatcher();
     const timeoutWatcher = new TimeoutWatcher(deadline);
-    const teardownRunner = new TaskRunner(this._reporter, this._globalTimeoutForError);
+    const teardownRunner = new TaskRunner<Context>(this._reporter, this._globalTimeoutForError);
     teardownRunner._isTearDown = true;
 
     let currentTaskName: string | undefined;
@@ -64,9 +64,8 @@ export class TaskRunner<Context> {
         const errors: TestError[] = [];
         const softErrors: TestError[] = [];
         try {
-          const teardown = await task(context, errors, softErrors);
+          teardownRunner._tasks.unshift({ name: `teardown for ${name}`, task: { setup: task.teardown } });
-          if (teardown)
+          await task.setup?.(context, errors, softErrors);
-            teardownRunner._tasks.unshift({ name: `teardown for ${name}`, task: teardown });
         } catch (e) {
           debug('pw:test:task')(`error in "${name}": `, e);
           errors.push(serializeError(e));
@@ -106,12 +105,14 @@ export class TaskRunner<Context> {
       status = 'failed';
     }
     cancelPromise?.resolve();
+    // Note that upon hitting deadline, we "run cleanup", but it exits immediately
+    // because of the same deadline. Essentially, we're not perfomring any cleanup.
     const cleanup = () => teardownRunner.runDeferCleanup(context, deadline).then(r => r.status);
     return { status, cleanup };
   }
 }
 
-export class TimeoutWatcher {
+class TimeoutWatcher {
   private _timedOut = false;
   readonly promise = new ManualPromise();
   private _timer: NodeJS.Timeout | undefined;

packages/playwright-test/src/runner/tasks.ts

@@ -93,7 +93,6 @@ function addRunTasks(taskRunner: TaskRunner<TestRun>, config: FullConfigInternal
   taskRunner.addTask('report begin', createReportBeginTask());
   for (const plugin of config.plugins)
     taskRunner.addTask('plugin begin', createPluginBeginTask(plugin));
-  taskRunner.addTask('start workers', createWorkersTask());
   taskRunner.addTask('test suite', createRunTestsTask());
   return taskRunner;
 }
@@ -106,48 +105,67 @@ export function createTaskRunnerForList(config: FullConfigInternal, reporter: Re
 }
 
 function createReportBeginTask(): Task<TestRun> {
-  return async ({ config, reporter, rootSuite }) => {
+  let montonicStartTime = 0;
-    const montonicStartTime = monotonicTime();
+  return {
+    setup: async ({ config, reporter, rootSuite }) => {
+      montonicStartTime = monotonicTime();
       reporter.onBegin(rootSuite!);
-    return async () => {
+    },
+    teardown: async ({ config }) => {
       config.config.metadata.totalTime = monotonicTime() - montonicStartTime;
-    };
+    },
   };
 }
 
 function createPluginSetupTask(plugin: TestRunnerPluginRegistration): Task<TestRun> {
-  return async ({ config, reporter }) => {
+  return {
+    setup: async ({ config, reporter }) => {
       if (typeof plugin.factory === 'function')
         plugin.instance = await plugin.factory();
       else
         plugin.instance = plugin.factory;
       await plugin.instance?.setup?.(config.config, config.configDir, reporter);
-    return () => plugin.instance?.teardown?.();
+    },
+    teardown: async () => {
+      await plugin.instance?.teardown?.();
+    },
   };
 }
 
 function createPluginBeginTask(plugin: TestRunnerPluginRegistration): Task<TestRun> {
-  return async ({ rootSuite }) => {
+  return {
+    setup: async ({ rootSuite }) => {
       await plugin.instance?.begin?.(rootSuite!);
-    return () => plugin.instance?.end?.();
+    },
+    teardown: async () => {
+      await plugin.instance?.end?.();
+    },
   };
 }
 
 function createGlobalSetupTask(): Task<TestRun> {
-  return async ({ config }) => {
+  let globalSetupResult: any;
+  let globalSetupFinished = false;
+  let teardownHook: any;
+  return {
+    setup: async ({ config }) => {
       const setupHook = config.config.globalSetup ? await loadGlobalHook(config, config.config.globalSetup) : undefined;
-    const teardownHook = config.config.globalTeardown ? await loadGlobalHook(config, config.config.globalTeardown) : undefined;
+      teardownHook = config.config.globalTeardown ? await loadGlobalHook(config, config.config.globalTeardown) : undefined;
-    const globalSetupResult = setupHook ? await setupHook(config.config) : undefined;
+      globalSetupResult = setupHook ? await setupHook(config.config) : undefined;
-    return async () => {
+      globalSetupFinished = true;
+    },
+    teardown: async ({ config }) => {
       if (typeof globalSetupResult === 'function')
         await globalSetupResult();
+      if (globalSetupFinished)
         await teardownHook?.(config.config);
-    };
+    },
   };
 }
 
 function createRemoveOutputDirsTask(): Task<TestRun> {
-  return async ({ config }) => {
+  return {
+    setup: async ({ config }) => {
       if (process.env.PW_TEST_NO_REMOVE_OUTPUT_DIRS)
         return;
       const outputDirs = new Set<string>();
@@ -167,22 +185,26 @@ function createRemoveOutputDirsTask(): Task<TestRun> {
           throw error;
         }
       })));
+    },
   };
 }
 
 function createLoadTask(mode: 'out-of-process' | 'in-process', options: { filterOnly: boolean, failOnLoadErrors: boolean, doNotRunTestsOutsideProjectFilter?: boolean, additionalFileMatcher?: Matcher }): Task<TestRun> {
-  return async (testRun, errors, softErrors) => {
+  return {
+    setup: async (testRun, errors, softErrors) => {
       await collectProjectsAndTestFiles(testRun, !!options.doNotRunTestsOutsideProjectFilter, options.additionalFileMatcher);
       await loadFileSuites(testRun, mode, options.failOnLoadErrors ? errors : softErrors);
       testRun.rootSuite = await createRootSuite(testRun, options.failOnLoadErrors ? errors : softErrors, !!options.filterOnly);
       // Fail when no tests.
       if (options.failOnLoadErrors && !testRun.rootSuite.allTests().length && !testRun.config.cliPassWithNoTests && !testRun.config.config.shard)
         throw new Error(`No tests found`);
+    },
   };
 }
 
 function createPhasesTask(): Task<TestRun> {
-  return async testRun => {
+  return {
+    setup: async testRun => {
       let maxConcurrentTestGroups = 0;
 
       const processed = new Set<FullProjectInternal>();
@@ -227,21 +249,13 @@ function createPhasesTask(): Task<TestRun> {
       }
 
       testRun.config.config.metadata.actualWorkers = Math.min(testRun.config.config.workers, maxConcurrentTestGroups);
-  };
+    },
-}
-
-function createWorkersTask(): Task<TestRun> {
-  return async ({ phases }) => {
-    return async () => {
-      for (const { dispatcher } of phases.reverse())
-        await dispatcher.stop();
-    };
   };
 }
 
 function createRunTestsTask(): Task<TestRun> {
-  return async testRun => {
+  return {
-    const { phases } = testRun;
+    setup: async ({ phases }) => {
       const successfulProjects = new Set<FullProjectInternal>();
       const extraEnvByProjectId: EnvByProjectId = new Map();
       const teardownToSetups = buildTeardownToSetupsMap(phases.map(phase => phase.projects.map(p => p.project)).flat());
@@ -290,5 +304,10 @@ function createRunTestsTask(): Task<TestRun> {
           }
         }
       }
+    },
+    teardown: async ({ phases }) => {
+      for (const { dispatcher } of phases.reverse())
+        await dispatcher.stop();
+    },
   };
 }

tests/playwright-test/runner.spec.ts

@@ -399,16 +399,19 @@ test('sigint should stop plugins', async ({ runInlineTest }) => {
     `,
     'a.spec.js': `
       import { test, expect } from '@playwright/test';
-      test('test', async () => { });
+      test('test', async () => {
+        console.log('testing!');
+      });
     `,
   }, { 'workers': 1 }, {}, { sendSIGINTAfter: 1 });
   expect(result.exitCode).toBe(130);
   expect(result.passed).toBe(0);
   const output = result.output;
   expect(output).toContain('Plugin1 setup');
-  expect(output).not.toContain('Plugin1 teardown');
+  expect(output).toContain('Plugin1 teardown');
   expect(output).not.toContain('Plugin2 setup');
   expect(output).not.toContain('Plugin2 teardown');
+  expect(output).not.toContain('testing!');
 });
 
 test('sigint should stop plugins 2', async ({ runInlineTest }) => {
@@ -440,7 +443,9 @@ test('sigint should stop plugins 2', async ({ runInlineTest }) => {
     `,
     'a.spec.js': `
       import { test, expect } from '@playwright/test';
-      test('test', async () => { });
+      test('test', async () => {
+        console.log('testing!');
+      });
     `,
   }, { 'workers': 1 }, {}, { sendSIGINTAfter: 1 });
   expect(result.exitCode).toBe(130);
@@ -449,7 +454,8 @@ test('sigint should stop plugins 2', async ({ runInlineTest }) => {
   expect(output).toContain('Plugin1 setup');
   expect(output).toContain('Plugin2 setup');
   expect(output).toContain('Plugin1 teardown');
-  expect(output).not.toContain('Plugin2 teardown');
+  expect(output).toContain('Plugin2 teardown');
+  expect(output).not.toContain('testing!');
 });
 
 test('should not crash with duplicate titles and .only', async ({ runInlineTest }) => {