chore(test runner): always go through internal reporter (#32426)

This way we guarantee the API contract and do not miss errors because we
forgot to call `onBegin()`.

packages/playwright/src/reporters/internalReporter.ts

@@ -21,6 +21,7 @@ import { Suite } from '../common/test';
 import { colors, prepareErrorStack, relativeFilePath } from './base';
 import type { ReporterV2 } from './reporterV2';
 import { monotonicTime } from 'playwright-core/lib/utils';
+import { Multiplexer } from './multiplexer';
 
 export class InternalReporter {
   private _reporter: ReporterV2;
@@ -29,8 +30,8 @@ export class InternalReporter {
   private _startTime: Date | undefined;
   private _monotonicStartTime: number | undefined;
 
-  constructor(reporter: ReporterV2) {
+  constructor(reporters: ReporterV2[]) {
-    this._reporter = reporter;
+    this._reporter = new Multiplexer(reporters);
   }
 
   version(): 'v2' {

packages/playwright/src/runner/runner.ts

@@ -27,6 +27,7 @@ import type { FullConfigInternal } from '../common/config';
 import type { Suite } from '../common/test';
 import { wrapReporterAsV2 } from '../reporters/reporterV2';
 import { affectedTestFiles } from '../transform/compilationCache';
+import { InternalReporter } from '../reporters/internalReporter';
 
 type ProjectConfigWithFiles = {
   name: string;
@@ -77,27 +78,28 @@ export class Runner {
     webServerPluginsForConfig(config).forEach(p => config.plugins.push({ factory: p }));
 
     const reporters = await createReporters(config, listOnly ? 'list' : 'test', false);
+    const reporter = new InternalReporter(reporters);
     const taskRunner = listOnly ? createTaskRunnerForList(
         config,
-        reporters,
+        reporter,
         'in-process',
-        { failOnLoadErrors: true }) : createTaskRunner(config, reporters);
+        { failOnLoadErrors: true }) : createTaskRunner(config, reporter);
 
     const testRun = new TestRun(config);
-    taskRunner.reporter.onConfigure(config.config);
+    reporter.onConfigure(config.config);
 
     const taskStatus = await taskRunner.run(testRun, deadline);
     let status: FullResult['status'] = testRun.failureTracker.result();
     if (status === 'passed' && taskStatus !== 'passed')
       status = taskStatus;
-    const modifiedResult = await taskRunner.reporter.onEnd({ status });
+    const modifiedResult = await reporter.onEnd({ status });
     if (modifiedResult && modifiedResult.status)
       status = modifiedResult.status;
 
     if (!listOnly)
       await writeLastRunInfo(testRun, status);
 
-    await taskRunner.reporter.onExit();
+    await reporter.onExit();
 
     // Calling process.exit() might truncate large stdout/stderr output.
     // See https://github.com/nodejs/node/issues/6456.
@@ -110,23 +112,23 @@ export class Runner {
   async loadAllTests(mode: 'in-process' | 'out-of-process' = 'in-process'): Promise<{ status: FullResult['status'], suite?: Suite, errors: TestError[] }> {
     const config = this._config;
     const errors: TestError[] = [];
-    const reporters = [wrapReporterAsV2({
+    const reporter = new InternalReporter([wrapReporterAsV2({
       onError(error: TestError) {
         errors.push(error);
       }
-    })];
+    })]);
-    const taskRunner = createTaskRunnerForList(config, reporters, mode, { failOnLoadErrors: true });
+    const taskRunner = createTaskRunnerForList(config, reporter, mode, { failOnLoadErrors: true });
     const testRun = new TestRun(config);
-    taskRunner.reporter.onConfigure(config.config);
+    reporter.onConfigure(config.config);
 
     const taskStatus = await taskRunner.run(testRun, 0);
     let status: FullResult['status'] = testRun.failureTracker.result();
     if (status === 'passed' && taskStatus !== 'passed')
       status = taskStatus;
-    const modifiedResult = await taskRunner.reporter.onEnd({ status });
+    const modifiedResult = await reporter.onEnd({ status });
     if (modifiedResult && modifiedResult.status)
       status = modifiedResult.status;
-    await taskRunner.reporter.onExit();
+    await reporter.onExit();
     return { status, suite: testRun.rootSuite, errors };
   }
 

packages/playwright/src/runner/taskRunner.ts

@@ -20,26 +20,25 @@ import type { FullResult, TestError } from '../../types/testReporter';
 import { SigIntWatcher } from './sigIntWatcher';
 import { serializeError } from '../util';
 import type { ReporterV2 } from '../reporters/reporterV2';
-import { InternalReporter } from '../reporters/internalReporter';
+import type { InternalReporter } from '../reporters/internalReporter';
-import { Multiplexer } from '../reporters/multiplexer';
 
 type TaskPhase<Context> = (reporter: ReporterV2, context: Context, errors: TestError[], softErrors: TestError[]) => Promise<void> | void;
 export type Task<Context> = { setup?: TaskPhase<Context>, teardown?: TaskPhase<Context> };
 
 export class TaskRunner<Context> {
   private _tasks: { name: string, task: Task<Context> }[] = [];
-  readonly reporter: InternalReporter;
+  private _reporter: InternalReporter;
   private _hasErrors = false;
   private _interrupted = false;
   private _isTearDown = false;
   private _globalTimeoutForError: number;
 
-  static create<Context>(reporters: ReporterV2[], globalTimeoutForError: number = 0) {
+  static create<Context>(reporter: InternalReporter, globalTimeoutForError: number = 0) {
-    return new TaskRunner<Context>(createInternalReporter(reporters), globalTimeoutForError);
+    return new TaskRunner<Context>(reporter, globalTimeoutForError);
   }
 
   private constructor(reporter: InternalReporter, globalTimeoutForError: number) {
-    this.reporter = reporter;
+    this._reporter = reporter;
     this._globalTimeoutForError = globalTimeoutForError;
   }
 
@@ -56,7 +55,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<Context>(this.reporter, this._globalTimeoutForError);
+    const teardownRunner = new TaskRunner<Context>(this._reporter, this._globalTimeoutForError);
     teardownRunner._isTearDown = true;
 
     let currentTaskName: string | undefined;
@@ -71,13 +70,13 @@ export class TaskRunner<Context> {
         const softErrors: TestError[] = [];
         try {
           teardownRunner._tasks.unshift({ name: `teardown for ${name}`, task: { setup: task.teardown } });
-          await task.setup?.(this.reporter, context, errors, softErrors);
+          await task.setup?.(this._reporter, context, errors, softErrors);
         } catch (e) {
           debug('pw:test:task')(`error in "${name}": `, e);
           errors.push(serializeError(e));
         } finally {
           for (const error of [...softErrors, ...errors])
-            this.reporter.onError?.(error);
+            this._reporter.onError?.(error);
           if (errors.length) {
             if (!this._isTearDown)
               this._interrupted = true;
@@ -105,7 +104,7 @@ export class TaskRunner<Context> {
     if (sigintWatcher.hadSignal() || cancelPromise?.isDone()) {
       status = 'interrupted';
     } else if (timeoutWatcher.timedOut()) {
-      this.reporter.onError?.({ message: colors.red(`Timed out waiting ${this._globalTimeoutForError / 1000}s for the ${currentTaskName} to run`) });
+      this._reporter.onError?.({ message: colors.red(`Timed out waiting ${this._globalTimeoutForError / 1000}s for the ${currentTaskName} to run`) });
       status = 'timedout';
     } else if (this._hasErrors) {
       status = 'failed';
@@ -146,7 +145,3 @@ class TimeoutWatcher {
     clearTimeout(this._timer);
   }
 }
-
-function createInternalReporter(reporters: ReporterV2[]): InternalReporter {
-  return new InternalReporter(new Multiplexer(reporters));
-}

packages/playwright/src/runner/tasks.ts

@@ -21,7 +21,6 @@ import { debug } from 'playwright-core/lib/utilsBundle';
 import { removeFolders } from 'playwright-core/lib/utils';
 import { Dispatcher, type EnvByProjectId } from './dispatcher';
 import type { TestRunnerPluginRegistration } from '../plugins';
-import type { ReporterV2 } from '../reporters/reporterV2';
 import { createTestGroups, type TestGroup } from '../runner/testGroups';
 import type { Task } from './taskRunner';
 import { TaskRunner } from './taskRunner';
@@ -32,6 +31,7 @@ import { Suite } from '../common/test';
 import { buildDependentProjects, buildTeardownToSetupsMap, filterProjects } from './projectUtils';
 import { FailureTracker } from './failureTracker';
 import { detectChangedTestFiles } from './vcs';
+import type { InternalReporter } from '../reporters/internalReporter';
 
 const readDirAsync = promisify(fs.readdir);
 
@@ -60,22 +60,22 @@ export class TestRun {
   }
 }
 
-export function createTaskRunner(config: FullConfigInternal, reporters: ReporterV2[]): TaskRunner<TestRun> {
+export function createTaskRunner(config: FullConfigInternal, reporter: InternalReporter): TaskRunner<TestRun> {
-  const taskRunner = TaskRunner.create<TestRun>(reporters, config.config.globalTimeout);
+  const taskRunner = TaskRunner.create<TestRun>(reporter, config.config.globalTimeout);
   addGlobalSetupTasks(taskRunner, config);
   taskRunner.addTask('load tests', createLoadTask('in-process', { filterOnly: true, failOnLoadErrors: true }));
   addRunTasks(taskRunner, config);
   return taskRunner;
 }
 
-export function createTaskRunnerForWatchSetup(config: FullConfigInternal, reporters: ReporterV2[]): TaskRunner<TestRun> {
+export function createTaskRunnerForWatchSetup(config: FullConfigInternal, reporter: InternalReporter): TaskRunner<TestRun> {
-  const taskRunner = TaskRunner.create<TestRun>(reporters);
+  const taskRunner = TaskRunner.create<TestRun>(reporter);
   addGlobalSetupTasks(taskRunner, config);
   return taskRunner;
 }
 
-export function createTaskRunnerForTestServer(config: FullConfigInternal, reporters: ReporterV2[]): TaskRunner<TestRun> {
+export function createTaskRunnerForTestServer(config: FullConfigInternal, reporter: InternalReporter): TaskRunner<TestRun> {
-  const taskRunner = TaskRunner.create<TestRun>(reporters);
+  const taskRunner = TaskRunner.create<TestRun>(reporter);
   taskRunner.addTask('load tests', createLoadTask('out-of-process', { filterOnly: true, failOnLoadErrors: false, doNotRunDepsOutsideProjectFilter: true }));
   addRunTasks(taskRunner, config);
   return taskRunner;
@@ -99,15 +99,15 @@ function addRunTasks(taskRunner: TaskRunner<TestRun>, config: FullConfigInternal
   return taskRunner;
 }
 
-export function createTaskRunnerForList(config: FullConfigInternal, reporters: ReporterV2[], mode: 'in-process' | 'out-of-process', options: { failOnLoadErrors: boolean }): TaskRunner<TestRun> {
+export function createTaskRunnerForList(config: FullConfigInternal, reporter: InternalReporter, mode: 'in-process' | 'out-of-process', options: { failOnLoadErrors: boolean }): TaskRunner<TestRun> {
-  const taskRunner = TaskRunner.create<TestRun>(reporters, config.config.globalTimeout);
+  const taskRunner = TaskRunner.create<TestRun>(reporter, config.config.globalTimeout);
   taskRunner.addTask('load tests', createLoadTask(mode, { ...options, filterOnly: false }));
   taskRunner.addTask('report begin', createReportBeginTask());
   return taskRunner;
 }
 
-export function createTaskRunnerForListFiles(config: FullConfigInternal, reporters: ReporterV2[]): TaskRunner<TestRun> {
+export function createTaskRunnerForListFiles(config: FullConfigInternal, reporter: InternalReporter): TaskRunner<TestRun> {
-  const taskRunner = TaskRunner.create<TestRun>(reporters, config.config.globalTimeout);
+  const taskRunner = TaskRunner.create<TestRun>(reporter, config.config.globalTimeout);
   taskRunner.addTask('load tests', createListFilesTask());
   taskRunner.addTask('report begin', createReportBeginTask());
   return taskRunner;

packages/playwright/src/runner/testServer.ts

@@ -39,6 +39,7 @@ import { serializeError } from '../util';
 import { cacheDir } from '../transform/compilationCache';
 import { baseFullConfig } from '../isomorphic/teleReceiver';
 import { InternalReporter } from '../reporters/internalReporter';
+import type { ReporterV2 } from '../reporters/reporterV2';
 
 const originalStdoutWrite = process.stdout.write;
 const originalStderrWrite = process.stderr.write;
@@ -102,15 +103,10 @@ export class TestServerDispatcher implements TestServerInterface {
     return await createReporterForTestServer(this._serializer, messageSink);
   }
 
-  private async _collectingReporter() {
+  private async _collectingInternalReporter(...extraReporters: ReporterV2[]) {
     const report: ReportEntry[] = [];
     const collectingReporter = await createReporterForTestServer(this._serializer, e => report.push(e));
-    return { collectingReporter, report };
+    return { reporter: new InternalReporter([collectingReporter, ...extraReporters]), report };
-  }
-
-  private async _collectingInternalReporter() {
-    const { collectingReporter, report } = await this._collectingReporter();
-    return { reporter: new InternalReporter(collectingReporter), report };
   }
 
   async initialize(params: Parameters<TestServerInterface['initialize']>[0]): ReturnType<TestServerInterface['initialize']> {
@@ -151,24 +147,17 @@ export class TestServerDispatcher implements TestServerInterface {
     const overrides: ConfigCLIOverrides = {
       outputDir: params.outputDir,
     };
-    const { config, error } = await this._loadConfig(overrides);
+    const { reporter, report } = await this._collectingInternalReporter(new ListReporter());
-    if (!config) {
+    const config = await this._loadConfigOrReportError(reporter, overrides);
-      const { reporter, report } = await this._collectingInternalReporter();
+    if (!config)
-      // Produce dummy config when it has an error.
-      reporter.onConfigure(baseFullConfig);
-      reporter.onError(error!);
-      await reporter.onExit();
       return { status: 'failed', report };
-    }
 
-    const { collectingReporter, report } = await this._collectingReporter();
+    const taskRunner = createTaskRunnerForWatchSetup(config, reporter);
-    const listReporter = new ListReporter();
+    reporter.onConfigure(config.config);
-    const taskRunner = createTaskRunnerForWatchSetup(config, [collectingReporter, listReporter]);
-    taskRunner.reporter.onConfigure(config.config);
     const testRun = new TestRun(config);
     const { status, cleanup: globalCleanup } = await taskRunner.runDeferCleanup(testRun, 0);
-    await taskRunner.reporter.onEnd({ status });
+    await reporter.onEnd({ status });
-    await taskRunner.reporter.onExit();
+    await reporter.onExit();
     if (status !== 'passed') {
       await globalCleanup();
       return { report, status };
@@ -188,11 +177,9 @@ export class TestServerDispatcher implements TestServerInterface {
     if (this._devServerHandle)
       return { status: 'failed', report: [] };
     const { reporter, report } = await this._collectingInternalReporter();
-    const { config, error } = await this._loadConfig();
+    const config = await this._loadConfigOrReportError(reporter);
-    if (!config) {
+    if (!config)
-      reporter.onError(error!);
       return { status: 'failed', report };
-    }
     const devServerCommand = (config.config as any)['@playwright/test']?.['cli']?.['dev-server'];
     if (!devServerCommand) {
       reporter.onError({ message: 'No dev-server command found in the configuration' });
@@ -216,7 +203,11 @@ export class TestServerDispatcher implements TestServerInterface {
       return { status: 'passed', report: [] };
     } catch (e) {
       const { reporter, report } = await this._collectingInternalReporter();
+      // Produce dummy config when it has an error.
+      reporter.onConfigure(baseFullConfig);
       reporter.onError(serializeError(e));
+      await reporter.onEnd({ status: 'failed' });
+      await reporter.onExit();
       return { status: 'failed', report };
     }
   }
@@ -228,21 +219,18 @@ export class TestServerDispatcher implements TestServerInterface {
   }
 
   async listFiles(params: Parameters<TestServerInterface['listFiles']>[0]): ReturnType<TestServerInterface['listFiles']> {
-    const { config, error } = await this._loadConfig();
+    const { reporter, report } = await this._collectingInternalReporter();
-    if (!config) {
+    const config = await this._loadConfigOrReportError(reporter);
-      const { reporter, report } = await this._collectingInternalReporter();
+    if (!config)
-      reporter.onError(error!);
       return { status: 'failed', report };
-    }
 
-    const { collectingReporter, report } = await this._collectingReporter();
     config.cliProjectFilter = params.projects?.length ? params.projects : undefined;
-    const taskRunner = createTaskRunnerForListFiles(config, [collectingReporter]);
+    const taskRunner = createTaskRunnerForListFiles(config, reporter);
-    taskRunner.reporter.onConfigure(config.config);
+    reporter.onConfigure(config.config);
     const testRun = new TestRun(config);
     const status = await taskRunner.run(testRun, 0);
-    await taskRunner.reporter.onEnd({ status });
+    await reporter.onEnd({ status });
-    await taskRunner.reporter.onExit();
+    await reporter.onExit();
     return { report, status };
   }
 
@@ -261,12 +249,10 @@ export class TestServerDispatcher implements TestServerInterface {
       retries: 0,
       outputDir: params.outputDir,
     };
-    const { config, error } = await this._loadConfig(overrides);
+    const { reporter, report } = await this._collectingInternalReporter();
-    if (!config) {
+    const config = await this._loadConfigOrReportError(reporter, overrides);
-      const { reporter, report } = await this._collectingInternalReporter();
+    if (!config)
-      reporter.onError(error!);
       return { report, status: 'failed' };
-    }
 
     config.cliArgs = params.locations || [];
     config.cliGrep = params.grep;
@@ -274,13 +260,12 @@ export class TestServerDispatcher implements TestServerInterface {
     config.cliProjectFilter = params.projects?.length ? params.projects : undefined;
     config.cliListOnly = true;
 
-    const { collectingReporter, report } = await this._collectingReporter();
+    const taskRunner = createTaskRunnerForList(config, reporter, 'out-of-process', { failOnLoadErrors: false });
-    const taskRunner = createTaskRunnerForList(config, [collectingReporter], 'out-of-process', { failOnLoadErrors: false });
     const testRun = new TestRun(config);
-    taskRunner.reporter.onConfigure(config.config);
+    reporter.onConfigure(config.config);
     const status = await taskRunner.run(testRun, 0);
-    await taskRunner.reporter.onEnd({ status });
+    await reporter.onEnd({ status });
-    await taskRunner.reporter.onExit();
+    await reporter.onExit();
 
     this._watchedProjectDirs = new Set();
     this._ignoredProjectOutputs = new Set();
@@ -337,12 +322,10 @@ export class TestServerDispatcher implements TestServerInterface {
     else
       process.env.PW_LIVE_TRACE_STACKS = undefined;
 
-    const { config, error } = await this._loadConfig(overrides);
+    const wireReporter = await this._wireReporter(e => this._dispatchEvent('report', e));
-    if (!config) {
+    const config = await this._loadConfigOrReportError(new InternalReporter([wireReporter]), overrides);
-      const wireReporter = await this._wireReporter(e => this._dispatchEvent('report', e));
+    if (!config)
-      wireReporter.onError(error!);
       return { status: 'failed' };
-    }
 
     const testIdSet = params.testIds ? new Set<string>(params.testIds) : null;
     config.cliListOnly = false;
@@ -353,16 +336,15 @@ export class TestServerDispatcher implements TestServerInterface {
     config.cliProjectFilter = params.projects?.length ? params.projects : undefined;
     config.testIdMatcher = testIdSet ? id => testIdSet.has(id) : undefined;
 
-    const reporters = await createReporters(config, 'test', true);
+    const configReporters = await createReporters(config, 'test', true);
-    const wireReporter = await this._wireReporter(e => this._dispatchEvent('report', e));
+    const reporter = new InternalReporter([...configReporters, wireReporter]);
-    reporters.push(wireReporter);
+    const taskRunner = createTaskRunnerForTestServer(config, reporter);
-    const taskRunner = createTaskRunnerForTestServer(config, reporters);
     const testRun = new TestRun(config);
-    taskRunner.reporter.onConfigure(config.config);
+    reporter.onConfigure(config.config);
     const stop = new ManualPromise();
     const run = taskRunner.run(testRun, 0, stop).then(async status => {
-      await taskRunner.reporter.onEnd({ status });
+      await reporter.onEnd({ status });
-      await taskRunner.reporter.onExit();
+      await reporter.onExit();
       this._testRun = undefined;
       return status;
     });
@@ -429,6 +411,18 @@ export class TestServerDispatcher implements TestServerInterface {
       return { config: null, error: serializeError(e) };
     }
   }
+
+  private async _loadConfigOrReportError(reporter: InternalReporter, overrides?: ConfigCLIOverrides): Promise<FullConfigInternal | null> {
+    const { config, error } = await this._loadConfig(overrides);
+    if (config)
+      return config;
+    // Produce dummy config when it has an error.
+    reporter.onConfigure(baseFullConfig);
+    reporter.onError(error!);
+    await reporter.onEnd({ status: 'failed' });
+    await reporter.onExit();
+    return null;
+  }
 }
 
 export async function runUIMode(configFile: string | undefined, options: TraceViewerServerOptions & TraceViewerRedirectOptions): Promise<reporterTypes.FullResult['status'] | 'restarted'> {