From cfae7f755c38ee489979cae22da6b85854f3ec58 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 3 Sep 2024 22:38:02 -0700 Subject: [PATCH] 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()`. --- .../src/reporters/internalReporter.ts | 5 +- packages/playwright/src/runner/runner.ts | 24 ++-- packages/playwright/src/runner/taskRunner.ts | 23 ++-- packages/playwright/src/runner/tasks.ts | 22 ++-- packages/playwright/src/runner/testServer.ts | 108 +++++++++--------- 5 files changed, 87 insertions(+), 95 deletions(-) diff --git a/packages/playwright/src/reporters/internalReporter.ts b/packages/playwright/src/reporters/internalReporter.ts index 02959ab11b..392a8d9374 100644 --- a/packages/playwright/src/reporters/internalReporter.ts +++ b/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) { - this._reporter = reporter; + constructor(reporters: ReporterV2[]) { + this._reporter = new Multiplexer(reporters); } version(): 'v2' { diff --git a/packages/playwright/src/runner/runner.ts b/packages/playwright/src/runner/runner.ts index 05cf8a6aac..c37964ce2d 100644 --- a/packages/playwright/src/runner/runner.ts +++ b/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 }; } diff --git a/packages/playwright/src/runner/taskRunner.ts b/packages/playwright/src/runner/taskRunner.ts index 96505bef2a..52a06aa98c 100644 --- a/packages/playwright/src/runner/taskRunner.ts +++ b/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 { Multiplexer } from '../reporters/multiplexer'; +import type { InternalReporter } from '../reporters/internalReporter'; type TaskPhase = (reporter: ReporterV2, context: Context, errors: TestError[], softErrors: TestError[]) => Promise | void; export type Task = { setup?: TaskPhase, teardown?: TaskPhase }; export class TaskRunner { private _tasks: { name: string, task: Task }[] = []; - readonly reporter: InternalReporter; + private _reporter: InternalReporter; private _hasErrors = false; private _interrupted = false; private _isTearDown = false; private _globalTimeoutForError: number; - static create(reporters: ReporterV2[], globalTimeoutForError: number = 0) { - return new TaskRunner(createInternalReporter(reporters), globalTimeoutForError); + static create(reporter: InternalReporter, globalTimeoutForError: number = 0) { + return new TaskRunner(reporter, globalTimeoutForError); } private constructor(reporter: InternalReporter, globalTimeoutForError: number) { - this.reporter = reporter; + this._reporter = reporter; this._globalTimeoutForError = globalTimeoutForError; } @@ -56,7 +55,7 @@ export class TaskRunner { async runDeferCleanup(context: Context, deadline: number, cancelPromise = new ManualPromise()): Promise<{ status: FullResult['status'], cleanup: () => Promise }> { const sigintWatcher = new SigIntWatcher(); const timeoutWatcher = new TimeoutWatcher(deadline); - const teardownRunner = new TaskRunner(this.reporter, this._globalTimeoutForError); + const teardownRunner = new TaskRunner(this._reporter, this._globalTimeoutForError); teardownRunner._isTearDown = true; let currentTaskName: string | undefined; @@ -71,13 +70,13 @@ export class TaskRunner { 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 { 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)); -} diff --git a/packages/playwright/src/runner/tasks.ts b/packages/playwright/src/runner/tasks.ts index 09a8a1fdaf..8a74c0337a 100644 --- a/packages/playwright/src/runner/tasks.ts +++ b/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 { - const taskRunner = TaskRunner.create(reporters, config.config.globalTimeout); +export function createTaskRunner(config: FullConfigInternal, reporter: InternalReporter): TaskRunner { + const taskRunner = TaskRunner.create(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 { - const taskRunner = TaskRunner.create(reporters); +export function createTaskRunnerForWatchSetup(config: FullConfigInternal, reporter: InternalReporter): TaskRunner { + const taskRunner = TaskRunner.create(reporter); addGlobalSetupTasks(taskRunner, config); return taskRunner; } -export function createTaskRunnerForTestServer(config: FullConfigInternal, reporters: ReporterV2[]): TaskRunner { - const taskRunner = TaskRunner.create(reporters); +export function createTaskRunnerForTestServer(config: FullConfigInternal, reporter: InternalReporter): TaskRunner { + const taskRunner = TaskRunner.create(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, config: FullConfigInternal return taskRunner; } -export function createTaskRunnerForList(config: FullConfigInternal, reporters: ReporterV2[], mode: 'in-process' | 'out-of-process', options: { failOnLoadErrors: boolean }): TaskRunner { - const taskRunner = TaskRunner.create(reporters, config.config.globalTimeout); +export function createTaskRunnerForList(config: FullConfigInternal, reporter: InternalReporter, mode: 'in-process' | 'out-of-process', options: { failOnLoadErrors: boolean }): TaskRunner { + const taskRunner = TaskRunner.create(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 { - const taskRunner = TaskRunner.create(reporters, config.config.globalTimeout); +export function createTaskRunnerForListFiles(config: FullConfigInternal, reporter: InternalReporter): TaskRunner { + const taskRunner = TaskRunner.create(reporter, config.config.globalTimeout); taskRunner.addTask('load tests', createListFilesTask()); taskRunner.addTask('report begin', createReportBeginTask()); return taskRunner; diff --git a/packages/playwright/src/runner/testServer.ts b/packages/playwright/src/runner/testServer.ts index 1df2d72e8c..4765bf5089 100644 --- a/packages/playwright/src/runner/testServer.ts +++ b/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 }; - } - - private async _collectingInternalReporter() { - const { collectingReporter, report } = await this._collectingReporter(); - return { reporter: new InternalReporter(collectingReporter), report }; + return { reporter: new InternalReporter([collectingReporter, ...extraReporters]), report }; } async initialize(params: Parameters[0]): ReturnType { @@ -151,24 +147,17 @@ export class TestServerDispatcher implements TestServerInterface { const overrides: ConfigCLIOverrides = { outputDir: params.outputDir, }; - const { config, error } = await this._loadConfig(overrides); - if (!config) { - const { reporter, report } = await this._collectingInternalReporter(); - // Produce dummy config when it has an error. - reporter.onConfigure(baseFullConfig); - reporter.onError(error!); - await reporter.onExit(); + const { reporter, report } = await this._collectingInternalReporter(new ListReporter()); + const config = await this._loadConfigOrReportError(reporter, overrides); + if (!config) return { status: 'failed', report }; - } - const { collectingReporter, report } = await this._collectingReporter(); - const listReporter = new ListReporter(); - const taskRunner = createTaskRunnerForWatchSetup(config, [collectingReporter, listReporter]); - taskRunner.reporter.onConfigure(config.config); + const taskRunner = createTaskRunnerForWatchSetup(config, reporter); + reporter.onConfigure(config.config); const testRun = new TestRun(config); const { status, cleanup: globalCleanup } = await taskRunner.runDeferCleanup(testRun, 0); - await taskRunner.reporter.onEnd({ status }); - await taskRunner.reporter.onExit(); + await reporter.onEnd({ status }); + 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(); - if (!config) { - reporter.onError(error!); + const config = await this._loadConfigOrReportError(reporter); + if (!config) 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[0]): ReturnType { - const { config, error } = await this._loadConfig(); - if (!config) { - const { reporter, report } = await this._collectingInternalReporter(); - reporter.onError(error!); + const { reporter, report } = await this._collectingInternalReporter(); + const config = await this._loadConfigOrReportError(reporter); + if (!config) return { status: 'failed', report }; - } - const { collectingReporter, report } = await this._collectingReporter(); config.cliProjectFilter = params.projects?.length ? params.projects : undefined; - const taskRunner = createTaskRunnerForListFiles(config, [collectingReporter]); - taskRunner.reporter.onConfigure(config.config); + const taskRunner = createTaskRunnerForListFiles(config, reporter); + reporter.onConfigure(config.config); const testRun = new TestRun(config); const status = await taskRunner.run(testRun, 0); - await taskRunner.reporter.onEnd({ status }); - await taskRunner.reporter.onExit(); + await reporter.onEnd({ status }); + 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); - if (!config) { - const { reporter, report } = await this._collectingInternalReporter(); - reporter.onError(error!); + const { reporter, report } = await this._collectingInternalReporter(); + const config = await this._loadConfigOrReportError(reporter, overrides); + if (!config) 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, [collectingReporter], 'out-of-process', { failOnLoadErrors: false }); + const taskRunner = createTaskRunnerForList(config, reporter, '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 taskRunner.reporter.onExit(); + await reporter.onEnd({ status }); + 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); - if (!config) { - const wireReporter = await this._wireReporter(e => this._dispatchEvent('report', e)); - wireReporter.onError(error!); + const wireReporter = await this._wireReporter(e => this._dispatchEvent('report', e)); + const config = await this._loadConfigOrReportError(new InternalReporter([wireReporter]), overrides); + if (!config) return { status: 'failed' }; - } const testIdSet = params.testIds ? new Set(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 wireReporter = await this._wireReporter(e => this._dispatchEvent('report', e)); - reporters.push(wireReporter); - const taskRunner = createTaskRunnerForTestServer(config, reporters); + const configReporters = await createReporters(config, 'test', true); + const reporter = new InternalReporter([...configReporters, wireReporter]); + const taskRunner = createTaskRunnerForTestServer(config, reporter); 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 taskRunner.reporter.onExit(); + await reporter.onEnd({ status }); + 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 { + 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 {