Skip to content

Commit

Permalink
chore(test runner): always go through internal reporter (#32426)
Browse files Browse the repository at this point in the history 
This way we guarantee the API contract and do not miss errors because we
forgot to call `onBegin()`.
  • Loading branch information
@dgozman
dgozman committed Sep 4, 2024
1 parent 446ed72 commit cfae7f7
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 95 deletions.
5 changes: 3 additions & 2 deletions packages/playwright/src/reporters/internalReporter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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' {
Expand Down
24 changes: 13 additions & 11 deletions packages/playwright/src/runner/runner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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 };
}

Expand Down
23 changes: 9 additions & 14 deletions packages/playwright/src/runner/taskRunner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<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) {
return new TaskRunner<Context>(createInternalReporter(reporters), globalTimeoutForError);
static create<Context>(reporter: InternalReporter, globalTimeoutForError: number = 0) {
return new TaskRunner<Context>(reporter, globalTimeoutForError);
}

private constructor(reporter: InternalReporter, globalTimeoutForError: number) {
this.reporter = reporter;
this._reporter = reporter;
this._globalTimeoutForError = globalTimeoutForError;
}

Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -146,7 +145,3 @@ class TimeoutWatcher {
clearTimeout(this._timer);
}
}

function createInternalReporter(reporters: ReporterV2[]): InternalReporter {
return new InternalReporter(new Multiplexer(reporters));
}
22 changes: 11 additions & 11 deletions packages/playwright/src/runner/tasks.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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);

Expand Down Expand Up @@ -60,22 +60,22 @@ export class TestRun {
}
}

export function createTaskRunner(config: FullConfigInternal, reporters: ReporterV2[]): TaskRunner<TestRun> {
const taskRunner = TaskRunner.create<TestRun>(reporters, config.config.globalTimeout);
export function createTaskRunner(config: FullConfigInternal, reporter: InternalReporter): TaskRunner<TestRun> {
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> {
const taskRunner = TaskRunner.create<TestRun>(reporters);
export function createTaskRunnerForWatchSetup(config: FullConfigInternal, reporter: InternalReporter): TaskRunner<TestRun> {
const taskRunner = TaskRunner.create<TestRun>(reporter);
addGlobalSetupTasks(taskRunner, config);
return taskRunner;
}

export function createTaskRunnerForTestServer(config: FullConfigInternal, reporters: ReporterV2[]): TaskRunner<TestRun> {
const taskRunner = TaskRunner.create<TestRun>(reporters);
export function createTaskRunnerForTestServer(config: FullConfigInternal, reporter: InternalReporter): TaskRunner<TestRun> {
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;
Expand All @@ -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> {
const taskRunner = TaskRunner.create<TestRun>(reporters, config.config.globalTimeout);
export function createTaskRunnerForList(config: FullConfigInternal, reporter: InternalReporter, mode: 'in-process' | 'out-of-process', options: { failOnLoadErrors: boolean }): TaskRunner<TestRun> {
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> {
const taskRunner = TaskRunner.create<TestRun>(reporters, config.config.globalTimeout);
export function createTaskRunnerForListFiles(config: FullConfigInternal, reporter: InternalReporter): TaskRunner<TestRun> {
const taskRunner = TaskRunner.create<TestRun>(reporter, config.config.globalTimeout);
taskRunner.addTask('load tests', createListFilesTask());
taskRunner.addTask('report begin', createReportBeginTask());
return taskRunner;
Expand Down
Loading

0 comments on commit cfae7f7

Please sign in to comment.