feat(breaking): always report onBegin/onEnd, report file errors (#11758)

docs/src/test-reporter-api/class-suite.md

@@ -24,6 +24,11 @@ Reporter is given a root suite in the [`method: Reporter.onBegin`] method.
 
 Returns the list of all test cases in this suite and its descendants, as opposite to [`property: Suite.tests`].
 
+## property: Suite.loadError
+- type: <[void]|[TestError]>
+
+For file suites, contains errors that occurred while loading this file.
+
 ## property: Suite.location
 - type: <[void]|[Location]>
 

packages/playwright-test/src/fixtures.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { formatLocation, wrapInPromise, debugTest } from './util';
+import { formatLocation, debugTest } from './util';
 import * as crypto from 'crypto';
 import { FixturesWithLocation, Location, WorkerInfo, TestInfo } from './types';
 import { ManualPromise } from 'playwright-core/lib/utils/async';
@@ -78,7 +78,7 @@ class Fixture {
       await this._useFuncFinished;
     };
     const info = this.registration.scope === 'worker' ? workerInfo : testInfo;
-    this._selfTeardownComplete = wrapInPromise(this.registration.fn(params, useFunc, info)).catch((e: any) => {
+    this._selfTeardownComplete = Promise.resolve().then(() => this.registration.fn(params, useFunc, info)).catch((e: any) => {
       if (!useFuncStarted.isDone())
         useFuncStarted.reject(e);
       else

packages/playwright-test/src/loader.ts

@@ -27,6 +27,7 @@ import { ProjectImpl } from './project';
 import { Reporter } from '../types/testReporter';
 import { BuiltInReporter, builtInReporters } from './runner';
 import { isRegExp } from 'playwright-core/lib/utils/utils';
+import { serializeError } from './util';
 
 // To allow multiple loaders in the same process without clearing require cache,
 // we make these maps global.
@@ -113,20 +114,25 @@ export class Loader {
     this._fullConfig.projects = this._projects.map(p => p.config);
   }
 
-  async loadTestFile(file: string) {
+  async loadTestFile(file: string, environment: 'runner' | 'worker') {
     if (cachedFileSuites.has(file))
       return cachedFileSuites.get(file)!;
-    try {
     const suite = new Suite(path.relative(this._fullConfig.rootDir, file) || path.basename(file));
     suite._requireFile = file;
     suite.location = { file, line: 0, column: 0 };
+
     setCurrentlyLoadingFileSuite(suite);
+    try {
       await this._requireOrImport(file);
       cachedFileSuites.set(file, suite);
-      return suite;
+    } catch (e) {
+      if (environment === 'worker')
+        throw e;
+      suite.loadError = serializeError(e);
     } finally {
       setCurrentlyLoadingFileSuite(undefined);
     }
+    return suite;
   }
 
   async loadGlobalHook(file: string, name: string): Promise<(config: FullConfig) => any> {

packages/playwright-test/src/reporters/multiplexer.ts

@@ -34,37 +34,37 @@ export class Multiplexer implements Reporter {
 
   onTestBegin(test: TestCase, result: TestResult) {
     for (const reporter of this._reporters)
-      reporter.onTestBegin?.(test, result);
+      wrap(() => reporter.onTestBegin?.(test, result));
   }
 
   onStdOut(chunk: string | Buffer, test?: TestCase, result?: TestResult) {
     for (const reporter of this._reporters)
-      reporter.onStdOut?.(chunk, test, result);
+      wrap(() => reporter.onStdOut?.(chunk, test, result));
   }
 
   onStdErr(chunk: string | Buffer, test?: TestCase, result?: TestResult) {
     for (const reporter of this._reporters)
-      reporter.onStdErr?.(chunk, test, result);
+      wrap(() => reporter.onStdErr?.(chunk, test, result));
   }
 
   onTestEnd(test: TestCase, result: TestResult) {
     for (const reporter of this._reporters)
-      reporter.onTestEnd?.(test, result);
+      wrap(() => reporter.onTestEnd?.(test, result));
   }
 
   async onEnd(result: FullResult) {
     for (const reporter of this._reporters)
-      await reporter.onEnd?.(result);
+      await Promise.resolve().then(() => reporter.onEnd?.(result)).catch(e => console.error('Error in reporter', e));
   }
 
   onError(error: TestError) {
     for (const reporter of this._reporters)
-      reporter.onError?.(error);
+      wrap(() => reporter.onError?.(error));
   }
 
   onStepBegin(test: TestCase, result: TestResult, step: TestStep) {
     for (const reporter of this._reporters)
-      (reporter as any).onStepBegin?.(test, result, step);
+      wrap(() => (reporter as any).onStepBegin?.(test, result, step));
   }
 
   onStepEnd(test: TestCase, result: TestResult, step: TestStep) {
@@ -72,3 +72,11 @@ export class Multiplexer implements Reporter {
       (reporter as any).onStepEnd?.(test, result, step);
   }
 }
+
+function wrap(callback: () => void) {
+  try {
+    callback();
+  } catch (e) {
+    console.error('Error in reporter', e);
+  }
+}

packages/playwright-test/src/runner.ts

@@ -20,7 +20,7 @@ import * as fs from 'fs';
 import * as path from 'path';
 import { promisify } from 'util';
 import { Dispatcher, TestGroup } from './dispatcher';
-import { createFileMatcher, createTitleMatcher, FilePatternFilter, serializeError } from './util';
+import { createFileMatcher, createTitleMatcher, FilePatternFilter } from './util';
 import { TestCase, Suite } from './test';
 import { Loader } from './loader';
 import { FullResult, Reporter, TestError } from '../types/testReporter';
@@ -39,6 +39,7 @@ import { Config, FullConfig } from './types';
 import { WebServer } from './webServer';
 import { raceAgainstTimeout } from 'playwright-core/lib/utils/async';
 import { SigIntWatcher } from 'playwright-core/lib/utils/utils';
+import { serializeError } from './util';
 
 const removeFolderAsync = promisify(rimraf);
 const readDirAsync = promisify(fs.readdir);
@@ -56,7 +57,6 @@ type RunOptions = {
 export class Runner {
   private _loader: Loader;
   private _reporter!: Reporter;
-  private _didBegin = false;
   private _internalGlobalSetups: Array<InternalGlobalSetupFunction> = [];
 
   constructor(configOverrides: Config, options: { defaultConfig?: Config } = {}) {
@@ -135,34 +135,24 @@ export class Runner {
 
   async runAllTests(options: RunOptions = {}): Promise<FullResult> {
     this._reporter = await this._createReporter(!!options.listOnly);
-    try {
     const config = this._loader.fullConfig();
     const result = await raceAgainstTimeout(() => this._run(!!options.listOnly, options.filePatternFilter || [], options.projectFilter), config.globalTimeout);
+    let fullResult: FullResult;
     if (result.timedOut) {
-        const actualResult: FullResult = { status: 'timedout' };
-        if (this._didBegin)
-          await this._reporter.onEnd?.(actualResult);
-        else
       this._reporter.onError?.(createStacklessError(`Timed out waiting ${config.globalTimeout / 1000}s for the entire test run`));
-        return actualResult;
+      fullResult = { status: 'timedout' };
-      } else if (this._didBegin) {
+    } else {
-        await this._reporter.onEnd?.(result.result);
+      fullResult = result.result;
     }
-      return result.result;
+    await this._reporter.onEnd?.(fullResult);
-    } catch (e) {
+
-      const result: FullResult = { status: 'failed' };
-      try {
-        this._reporter.onError?.(serializeError(e));
-      } catch (ignored) {
-      }
-      return result;
-    } finally {
     // Calling process.exit() might truncate large stdout/stderr output.
     // See https://github.com/nodejs/node/issues/6456.
     // See https://github.com/nodejs/node/issues/12921
     await new Promise<void>(resolve => process.stdout.write('', () => resolve()));
     await new Promise<void>(resolve => process.stderr.write('', () => resolve()));
-    }
+
+    return fullResult;
   }
 
   async listAllTestFiles(config: Config, projectNames: string[] | undefined): Promise<any> {
@@ -237,20 +227,17 @@ export class Runner {
       files.forEach(file => allTestFiles.add(file));
 
     const config = this._loader.fullConfig();
-    const internalGlobalTeardowns: (() => Promise<void>)[] = [];
+
-    if (!list) {
+    const fatalErrors: TestError[] = [];
-      for (const internalGlobalSetup of this._internalGlobalSetups)
+
-        internalGlobalTeardowns.push(await internalGlobalSetup());
-    }
-    const webServer = (!list && config.webServer) ? await WebServer.create(config.webServer) : undefined;
-    let globalSetupResult: any;
-    if (config.globalSetup && !list)
-      globalSetupResult = await (await this._loader.loadGlobalHook(config.globalSetup, 'globalSetup'))(this._loader.fullConfig());
-    try {
     // 1. Add all tests.
     const preprocessRoot = new Suite('');
-      for (const file of allTestFiles)
+    for (const file of allTestFiles) {
-        preprocessRoot._addSuite(await this._loader.loadTestFile(file));
+      const fileSuite = await this._loader.loadTestFile(file, 'runner');
+      if (fileSuite.loadError)
+        fatalErrors.push(fileSuite.loadError);
+      preprocessRoot._addSuite(fileSuite);
+    }
 
     // 2. Filter tests to respect column filter.
     filterByFocusedLine(preprocessRoot, testFileReFilters);
@@ -258,10 +245,8 @@ export class Runner {
     // 3. Complain about only.
     if (config.forbidOnly) {
       const onlyTestsAndSuites = preprocessRoot._getOnlyItems();
-        if (onlyTestsAndSuites.length > 0) {
+      if (onlyTestsAndSuites.length > 0)
-          this._reporter.onError?.(createForbidOnlyError(config, onlyTestsAndSuites));
+        fatalErrors.push(createForbidOnlyError(config, onlyTestsAndSuites));
-          return { status: 'failed' };
-        }
     }
 
     // 4. Filter only
@@ -270,11 +255,10 @@ export class Runner {
 
     // 5. Complain about clashing.
     const clashingTests = getClashingTestsPerSuite(preprocessRoot);
-      if (clashingTests.size > 0) {
+    if (clashingTests.size > 0)
-        this._reporter.onError?.(createDuplicateTitlesError(config, clashingTests));
+      fatalErrors.push(createDuplicateTitlesError(config, clashingTests));
-        return { status: 'failed' };
-      }
 
+    // 6. Generate projects.
     const fileSuites = new Map<string, Suite>();
     for (const fileSuite of preprocessRoot.suites)
       fileSuites.set(fileSuite._requireFile, fileSuite);
@@ -305,14 +289,17 @@ export class Runner {
       outputDirs.add(project.config.outputDir);
     }
 
+    // 7. Fail when no tests.
     let total = rootSuite.allTests().length;
-      if (!total) {
+    if (!total)
-        this._reporter.onError?.(createNoTestsError());
+      fatalErrors.push(createNoTestsError());
-        return { status: 'failed' };
-      }
 
-      await Promise.all(Array.from(outputDirs).map(outputDir => removeFolderAsync(outputDir).catch(e => {})));
+    // 8. Fail when output fails.
+    await Promise.all(Array.from(outputDirs).map(outputDir => removeFolderAsync(outputDir).catch(e => {
+      fatalErrors.push(serializeError(e));
+    })));
 
+    // 9. Compute shards.
     let testGroups = createTestGroups(rootSuite);
 
     const shard = config.shard;
@@ -346,12 +333,38 @@ export class Runner {
     }
     (config as any).__testGroupsCount = testGroups.length;
 
+    // 10. Report begin
+    this._reporter.onBegin?.(config, rootSuite);
+
+    // 11. Bail out on errors prior to running global setup.
+    if (fatalErrors.length) {
+      for (const error of fatalErrors)
+        this._reporter.onError?.(error);
+      return { status: 'failed' };
+    }
+
+    // 12. Bail out if list mode only, don't do any work.
+    if (list)
+      return { status: 'passed' };
+
+    // 13. Declare global setup to tear down in finally.
+    const internalGlobalTeardowns: (() => Promise<void>)[] = [];
+    let webServer: WebServer | undefined;
+    let globalSetupResult: any;
+
+    const result: FullResult = { status: 'passed' };
+
+    try {
+      // 14. Perform global setup.
+      for (const internalGlobalSetup of this._internalGlobalSetups)
+        internalGlobalTeardowns.push(await internalGlobalSetup());
+      webServer = config.webServer ? await WebServer.create(config.webServer) : undefined;
+      if (config.globalSetup)
+        globalSetupResult = await (await this._loader.loadGlobalHook(config.globalSetup, 'globalSetup'))(this._loader.fullConfig());
+
       const sigintWatcher = new SigIntWatcher();
 
-      this._reporter.onBegin?.(config, rootSuite);
-      this._didBegin = true;
       let hasWorkerErrors = false;
-      if (!list) {
       const dispatcher = new Dispatcher(this._loader, testGroups, this._reporter);
       await Promise.race([dispatcher.run(), sigintWatcher.promise()]);
       if (!sigintWatcher.hadSignal()) {
@@ -361,24 +374,47 @@ export class Runner {
       }
       await dispatcher.stop();
       hasWorkerErrors = dispatcher.hasWorkerErrors();
-      }
-
-      if (sigintWatcher.hadSignal()) {
-        const result: FullResult = { status: 'interrupted' };
-        return result;
-      }
 
+      if (!sigintWatcher.hadSignal()) {
         const failed = hasWorkerErrors || rootSuite.allTests().some(test => !test.ok());
-      const result: FullResult = { status: failed ? 'failed' : 'passed' };
+        result.status = failed ? 'failed' : 'passed';
-      return result;
+      } else {
+        result.status = 'interrupted';
+      }
+    } catch (e) {
+      this._reporter.onError?.(serializeError(e));
+      return { status: 'failed' };
     } finally {
+
+      await this._runAndAssignError(async () => {
         if (globalSetupResult && typeof globalSetupResult === 'function')
           await globalSetupResult(this._loader.fullConfig());
-      if (config.globalTeardown && !list)
+      }, result);
+
+      await this._runAndAssignError(async () => {
+        if (config.globalTeardown)
           await (await this._loader.loadGlobalHook(config.globalTeardown, 'globalTeardown'))(this._loader.fullConfig());
+      }, result);
+
+      await this._runAndAssignError(async () => {
         await webServer?.kill();
+      }, result);
+
+      await this._runAndAssignError(async () => {
         for (const internalGlobalTeardown of internalGlobalTeardowns)
           await internalGlobalTeardown();
+      }, result);
+    }
+    return result;
+  }
+
+  private async _runAndAssignError(callback: () => Promise<void>, result: FullResult) {
+    try {
+      await callback();
+    } catch (e) {
+      if (result.status === 'passed')
+        result.status = 'failed';
+      this._reporter.onError?.(serializeError(e));
     }
   }
 }

packages/playwright-test/src/test.ts

@@ -40,6 +40,7 @@ export type Modifier = {
 export class Suite extends Base implements reporterTypes.Suite {
   suites: Suite[] = [];
   tests: TestCase[] = [];
+  loadError?: reporterTypes.TestError;
   location?: Location;
   parent?: Suite;
   _use: FixturesWithLocation[] = [];

packages/playwright-test/src/util.ts

@@ -106,10 +106,6 @@ export function mergeObjects<A extends object, B extends object>(a: A | undefine
   return result as any;
 }
 
-export async function wrapInPromise(value: any) {
-  return value;
-}
-
 export function forceRegExp(pattern: string): RegExp {
   const match = pattern.match(/^\/(.*)\/([gi]*)$/);
   if (match)

packages/playwright-test/src/workerRunner.ts

@@ -125,7 +125,7 @@ export class WorkerRunner extends EventEmitter {
     try {
       this._entries = new Map(runPayload.entries.map(e => [ e.testId, e ]));
       await this._loadIfNeeded();
-      const fileSuite = await this._loader.loadTestFile(runPayload.file);
+      const fileSuite = await this._loader.loadTestFile(runPayload.file, 'worker');
       const suite = this._project.cloneFileSuite(fileSuite, this._params.repeatEachIndex, test => {
         if (!this._entries.has(test._id))
           return false;

packages/playwright-test/types/testReporter.d.ts

@@ -70,6 +70,10 @@ export interface Suite {
    *   group suite.
    */
   title: string;
+  /**
+   * For file suites, contains errors that occurred while loading this file.
+   */
+  loadError?: TestError;
   /**
    * Location in the source where the suite is defined. Missing for root and project suites.
    */

tests/playwright-test/global-setup.spec.ts

@@ -106,38 +106,14 @@ test('globalTeardown does not run when globalSetup times out', async ({ runInlin
       });
     `,
   });
-  // We did not collect tests, so everything should be zero.
+  // We did not run tests, so we should only have 1 skipped test.
-  expect(result.skipped).toBe(0);
+  expect(result.skipped).toBe(1);
   expect(result.passed).toBe(0);
   expect(result.failed).toBe(0);
   expect(result.exitCode).toBe(1);
   expect(result.output).not.toContain('teardown=');
 });
 
-test('globalSetup should be run before requiring tests', async ({ runInlineTest }) => {
-  const { passed } = await runInlineTest({
-    'playwright.config.ts': `
-      import * as path from 'path';
-      module.exports = {
-        globalSetup: './globalSetup.ts',
-      };
-    `,
-    'globalSetup.ts': `
-      module.exports = async () => {
-        process.env.FOO = JSON.stringify({ foo: 'bar' });
-      };
-    `,
-    'a.test.js': `
-      const { test } = pwt;
-      let value = JSON.parse(process.env.FOO);
-      test('should work', async ({}) => {
-        expect(value).toEqual({ foo: 'bar' });
-      });
-    `,
-  });
-  expect(passed).toBe(1);
-});
-
 test('globalSetup should work with sync function', async ({ runInlineTest }) => {
   const { passed } = await runInlineTest({
     'playwright.config.ts': `
@@ -153,8 +129,8 @@ test('globalSetup should work with sync function', async ({ runInlineTest }) =>
     `,
     'a.test.js': `
       const { test } = pwt;
-      let value = JSON.parse(process.env.FOO);
       test('should work', async ({}) => {
+        const value = JSON.parse(process.env.FOO);
         expect(value).toEqual({ foo: 'bar' });
       });
     `,

utils/generate_types/overrides-testReporter.d.ts

@@ -26,6 +26,7 @@ export interface Location {
 export interface Suite {
   parent?: Suite;
   title: string;
+  loadError?: TestError;
   location?: Location;
   suites: Suite[];
   tests: TestCase[];