From f7ff2524557bb70d5b035b38230e937ffe48afe2 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 25 Jan 2023 17:26:30 -0800 Subject: [PATCH] chore: remove addFatalError (#20383) --- packages/playwright-test/src/fixtures.ts | 51 +++++++----- packages/playwright-test/src/globals.ts | 14 ---- packages/playwright-test/src/loaderMain.ts | 6 +- packages/playwright-test/src/poolBuilder.ts | 23 ++++-- .../src/reporters/multiplexer.ts | 3 - packages/playwright-test/src/runner.ts | 80 +++++++------------ packages/playwright-test/src/taskRunner.ts | 20 +++-- packages/playwright-test/src/testLoader.ts | 5 +- packages/playwright-test/src/testType.ts | 33 +++----- tests/playwright-test/exit-code.spec.ts | 4 +- tests/playwright-test/reporter-base.spec.ts | 2 +- tests/playwright-test/reporter.spec.ts | 2 +- 12 files changed, 111 insertions(+), 132 deletions(-) diff --git a/packages/playwright-test/src/fixtures.ts b/packages/playwright-test/src/fixtures.ts index 402d64f347..515b580344 100644 --- a/packages/playwright-test/src/fixtures.ts +++ b/packages/playwright-test/src/fixtures.ts @@ -20,7 +20,6 @@ import type { FixturesWithLocation, Location, WorkerInfo } from './types'; import { ManualPromise } from 'playwright-core/lib/utils'; import type { TestInfoImpl } from './testInfo'; import type { FixtureDescription, TimeoutManager } from './timeoutManager'; -import { addFatalError } from './globals'; type FixtureScope = 'test' | 'worker'; type FixtureAuto = boolean | 'all-hooks-included'; @@ -52,6 +51,10 @@ type FixtureRegistration = { // Whether this fixture is an option value set from the config. fromConfig?: boolean; }; +export type LoadError = { + message: string; + location: Location; +}; class Fixture { runner: FixtureRunner; @@ -187,9 +190,11 @@ export function isFixtureOption(value: any): value is FixtureTuple { export class FixturePool { readonly digest: string; readonly registrations: Map; + private _onLoadError: (error: LoadError) => void; - constructor(fixturesList: FixturesWithLocation[], parentPool?: FixturePool, disallowWorkerFixtures?: boolean) { + constructor(fixturesList: FixturesWithLocation[], onLoadError: (error: LoadError) => void, parentPool?: FixturePool, disallowWorkerFixtures?: boolean) { this.registrations = new Map(parentPool ? parentPool.registrations : []); + this._onLoadError = onLoadError; for (const { fixtures, location, fromConfig } of fixturesList) { for (const entry of Object.entries(fixtures)) { @@ -211,11 +216,11 @@ export class FixturePool { const previous = this.registrations.get(name); if (previous && options) { if (previous.scope !== options.scope) { - addFatalError(`Fixture "${name}" has already been registered as a { scope: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location); + this._addLoadError(`Fixture "${name}" has already been registered as a { scope: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location); continue; } if (previous.auto !== options.auto) { - addFatalError(`Fixture "${name}" has already been registered as a { auto: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location); + this._addLoadError(`Fixture "${name}" has already been registered as a { auto: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location); continue; } } else if (previous) { @@ -225,11 +230,11 @@ export class FixturePool { } if (!kScopeOrder.includes(options.scope)) { - addFatalError(`Fixture "${name}" has unknown { scope: '${options.scope}' }.`, location); + this._addLoadError(`Fixture "${name}" has unknown { scope: '${options.scope}' }.`, location); continue; } if (options.scope === 'worker' && disallowWorkerFixtures) { - addFatalError(`Cannot use({ ${name} }) in a describe group, because it forces a new worker.\nMake it top-level in the test file or put in the configuration file.`, location); + this._addLoadError(`Cannot use({ ${name} }) in a describe group, because it forces a new worker.\nMake it top-level in the test file or put in the configuration file.`, location); continue; } @@ -242,7 +247,7 @@ export class FixturePool { fn = original.fn; } - const deps = fixtureParameterNames(fn, location); + const deps = fixtureParameterNames(fn, location, e => this._onLoadError(e)); const registration: FixtureRegistration = { id: '', name, location, scope: options.scope, fn, auto: options.auto, option: options.option, timeout: options.timeout, customTitle: options.customTitle, deps, super: previous, fromConfig }; registrationId(registration); this.registrations.set(name, registration); @@ -262,13 +267,13 @@ export class FixturePool { const dep = this.resolveDependency(registration, name); if (!dep) { if (name === registration.name) - addFatalError(`Fixture "${registration.name}" references itself, but does not have a base implementation.`, registration.location); + this._addLoadError(`Fixture "${registration.name}" references itself, but does not have a base implementation.`, registration.location); else - addFatalError(`Fixture "${registration.name}" has unknown parameter "${name}".`, registration.location); + this._addLoadError(`Fixture "${registration.name}" has unknown parameter "${name}".`, registration.location); continue; } if (kScopeOrder.indexOf(registration.scope) > kScopeOrder.indexOf(dep.scope)) { - addFatalError(`${registration.scope} fixture "${registration.name}" cannot depend on a ${dep.scope} fixture "${name}" defined in ${formatLocation(dep.location)}.`, registration.location); + this._addLoadError(`${registration.scope} fixture "${registration.name}" cannot depend on a ${dep.scope} fixture "${name}" defined in ${formatLocation(dep.location)}.`, registration.location); continue; } if (!markers.has(dep)) { @@ -277,7 +282,7 @@ export class FixturePool { const index = stack.indexOf(dep); const regs = stack.slice(index, stack.length); const names = regs.map(r => `"${r.name}"`); - addFatalError(`Fixtures ${names.join(' -> ')} -> "${dep.name}" form a dependency cycle: ${regs.map(r => formatLocation(r.location)).join(' -> ')}`, dep.location); + this._addLoadError(`Fixtures ${names.join(' -> ')} -> "${dep.name}" form a dependency cycle: ${regs.map(r => formatLocation(r.location)).join(' -> ')}`, dep.location); continue; } } @@ -297,10 +302,10 @@ export class FixturePool { } validateFunction(fn: Function, prefix: string, location: Location) { - for (const name of fixtureParameterNames(fn, location)) { + for (const name of fixtureParameterNames(fn, location, e => this._onLoadError(e))) { const registration = this.registrations.get(name); if (!registration) - addFatalError(`${prefix} has unknown parameter "${name}".`, location); + this._addLoadError(`${prefix} has unknown parameter "${name}".`, location); } } @@ -309,6 +314,10 @@ export class FixturePool { return registration.super; return this.registrations.get(name); } + + private _addLoadError(message: string, location: Location) { + this._onLoadError({ message, location }); + } } export class FixtureRunner { @@ -368,7 +377,7 @@ export class FixtureRunner { } // Install used fixtures. - const names = fixtureParameterNames(fn, { file: '', line: 1, column: 1 }); + const names = fixtureParameterNames(fn, { file: '', line: 1, column: 1 }, serializeAndThrowError); const params: { [key: string]: any } = {}; for (const name of names) { const registration = this.pool!.registrations.get(name)!; @@ -404,7 +413,7 @@ export class FixtureRunner { } dependsOnWorkerFixturesOnly(fn: Function, location: Location): boolean { - const names = fixtureParameterNames(fn, location); + const names = fixtureParameterNames(fn, location, serializeAndThrowError); for (const name of names) { const registration = this.pool!.registrations.get(name)!; if (registration.scope !== 'worker') @@ -414,17 +423,21 @@ export class FixtureRunner { } } +function serializeAndThrowError(e: LoadError) { + throw new Error(`${formatLocation(e.location!)}: ${e.message}`); +} + const signatureSymbol = Symbol('signature'); -function fixtureParameterNames(fn: Function | any, location: Location): string[] { +function fixtureParameterNames(fn: Function | any, location: Location, onError: (error: LoadError) => void): string[] { if (typeof fn !== 'function') return []; if (!fn[signatureSymbol]) - fn[signatureSymbol] = innerFixtureParameterNames(fn, location); + fn[signatureSymbol] = innerFixtureParameterNames(fn, location, onError); return fn[signatureSymbol]; } -function innerFixtureParameterNames(fn: Function, location: Location): string[] { +function innerFixtureParameterNames(fn: Function, location: Location, onError: (error: LoadError) => void): string[] { const text = fn.toString(); const match = text.match(/(?:async)?(?:\s+function)?[^(]*\(([^)]*)/); if (!match) @@ -434,7 +447,7 @@ function innerFixtureParameterNames(fn: Function, location: Location): string[] return []; const [firstParam] = splitByComma(trimmedParams); if (firstParam[0] !== '{' || firstParam[firstParam.length - 1] !== '}') - addFatalError('First argument must use the object destructuring pattern: ' + firstParam, location); + onError({ message: 'First argument must use the object destructuring pattern: ' + firstParam, location }); const props = splitByComma(firstParam.substring(1, firstParam.length - 1)).map(prop => { const colon = prop.indexOf(':'); return colon === -1 ? prop : prop.substring(0, colon).trim(); diff --git a/packages/playwright-test/src/globals.ts b/packages/playwright-test/src/globals.ts index 9468be9673..fb5119d613 100644 --- a/packages/playwright-test/src/globals.ts +++ b/packages/playwright-test/src/globals.ts @@ -16,8 +16,6 @@ import type { TestInfoImpl } from './testInfo'; import type { Suite } from './test'; -import type { TestError, Location } from '../types/testReporter'; -import { formatLocation } from './util'; let currentTestInfoValue: TestInfoImpl | null = null; export function setCurrentTestInfo(testInfo: TestInfoImpl | null) { @@ -34,15 +32,3 @@ export function setCurrentlyLoadingFileSuite(suite: Suite | undefined) { export function currentlyLoadingFileSuite() { return currentFileSuite; } - -let _fatalErrorSink: ((fatalError: TestError) => void) | undefined; -export function setFatalErrorSink(fatalErrorSink: (fatalError: TestError) => void) { - _fatalErrorSink = fatalErrorSink; -} - -export function addFatalError(message: string, location: Location) { - if (_fatalErrorSink) - _fatalErrorSink({ message: `Error: ${message}`, location }); - else - throw new Error(`${formatLocation(location)}: ${message}`); -} diff --git a/packages/playwright-test/src/loaderMain.ts b/packages/playwright-test/src/loaderMain.ts index 4991bfaaa8..c8ab12d544 100644 --- a/packages/playwright-test/src/loaderMain.ts +++ b/packages/playwright-test/src/loaderMain.ts @@ -15,11 +15,10 @@ */ import type { SerializedConfig } from './ipc'; -import type { TestError } from '../reporter'; import { ConfigLoader } from './configLoader'; import { ProcessRunner } from './process'; import { loadTestFilesInProcess } from './testLoader'; -import { setFatalErrorSink } from './globals'; +import type { LoadError } from './fixtures'; export class LoaderMain extends ProcessRunner { private _config: SerializedConfig; @@ -37,8 +36,7 @@ export class LoaderMain extends ProcessRunner { } async loadTestFiles(params: { files: string[] }) { - const loadErrors: TestError[] = []; - setFatalErrorSink(error => loadErrors.push(error)); + const loadErrors: LoadError[] = []; const configLoader = await this._configLoader(); const rootSuite = await loadTestFilesInProcess(configLoader.fullConfig(), params.files, loadErrors); return { rootSuite: rootSuite._deepSerialize(), loadErrors }; diff --git a/packages/playwright-test/src/poolBuilder.ts b/packages/playwright-test/src/poolBuilder.ts index e4d1409515..796d7d4c45 100644 --- a/packages/playwright-test/src/poolBuilder.ts +++ b/packages/playwright-test/src/poolBuilder.ts @@ -15,25 +15,29 @@ */ import { FixturePool, isFixtureOption } from './fixtures'; +import type { LoadError } from './fixtures'; import type { Suite, TestCase } from './test'; import type { TestTypeImpl } from './testType'; import type { Fixtures, FixturesWithLocation, FullProjectInternal } from './types'; +import { formatLocation } from './util'; export class PoolBuilder { private _project: FullProjectInternal | undefined; private _testTypePools = new Map(); private _type: 'loader' | 'worker'; + private _loadErrors: LoadError[] | undefined; - static buildForLoader(suite: Suite) { - new PoolBuilder('loader').buildPools(suite); + static buildForLoader(suite: Suite, loadErrors: LoadError[]) { + new PoolBuilder('loader', loadErrors).buildPools(suite); } static createForWorker(project: FullProjectInternal) { - return new PoolBuilder('worker', project); + return new PoolBuilder('worker', undefined, project); } - private constructor(type: 'loader' | 'worker', project?: FullProjectInternal) { + private constructor(type: 'loader' | 'worker', loadErrors?: LoadError[], project?: FullProjectInternal) { this._type = type; + this._loadErrors = loadErrors; this._project = project; } @@ -57,7 +61,7 @@ export class PoolBuilder { for (const parent of parents) { if (parent._use.length) - pool = new FixturePool(parent._use, pool, parent._type === 'describe'); + pool = new FixturePool(parent._use, e => this._onLoadError(e), pool, parent._type === 'describe'); for (const hook of parent._hooks) pool.validateFunction(hook.fn, hook.type + ' hook', hook.location); for (const modifier of parent._modifiers) @@ -71,12 +75,19 @@ export class PoolBuilder { private _buildTestTypePool(testType: TestTypeImpl): FixturePool { if (!this._testTypePools.has(testType)) { const fixtures = this._project ? this._applyConfigUseOptions(this._project, testType) : testType.fixtures; - const pool = new FixturePool(fixtures); + const pool = new FixturePool(fixtures, e => this._onLoadError(e)); this._testTypePools.set(testType, pool); } return this._testTypePools.get(testType)!; } + private _onLoadError(e: LoadError): void { + if (this._loadErrors) + this._loadErrors.push(e); + else + throw new Error(`${formatLocation(e.location)}: ${e.message}`); + } + private _applyConfigUseOptions(project: FullProjectInternal, testType: TestTypeImpl): FixturesWithLocation[] { const projectUse = project.use || {}; const configKeys = new Set(Object.keys(projectUse)); diff --git a/packages/playwright-test/src/reporters/multiplexer.ts b/packages/playwright-test/src/reporters/multiplexer.ts index 4615c4fb18..ab6877719f 100644 --- a/packages/playwright-test/src/reporters/multiplexer.ts +++ b/packages/playwright-test/src/reporters/multiplexer.ts @@ -28,7 +28,6 @@ export class Multiplexer implements Reporter { private _reporters: Reporter[]; private _deferredErrors: TestError[] | null = []; private _deferredStdIO: StdIOChunk[] | null = []; - hasErrors = false; private _config!: FullConfig; constructor(reporters: Reporter[]) { @@ -107,8 +106,6 @@ export class Multiplexer implements Reporter { } onError(error: TestError) { - this.hasErrors = true; - if (this._deferredErrors) { this._deferredErrors.push(error); return; diff --git a/packages/playwright-test/src/runner.ts b/packages/playwright-test/src/runner.ts index 69a1cfa308..5a85c953b0 100644 --- a/packages/playwright-test/src/runner.ts +++ b/packages/playwright-test/src/runner.ts @@ -41,13 +41,13 @@ import { Multiplexer } from './reporters/multiplexer'; import type { TestCase } from './test'; import { Suite } from './test'; import type { Config, FullConfigInternal, FullProjectInternal } from './types'; -import { createFileMatcher, createFileMatcherFromFilters, createTitleMatcher, serializeError } from './util'; +import { createFileMatcher, createFileMatcherFromFilters, createTitleMatcher } from './util'; import type { Matcher, TestFileFilter } from './util'; -import { setFatalErrorSink } from './globals'; import { buildFileSuiteForProject, filterOnly, filterSuite, filterSuiteWithOnlySemantics, filterTestsRemoveEmptySuites } from './suiteUtils'; import { LoaderHost } from './loaderHost'; import { loadTestFilesInProcess } from './testLoader'; import { TaskRunner } from './taskRunner'; +import type { LoadError } from './fixtures'; const removeFolderAsync = promisify(rimraf); const readDirAsync = promisify(fs.readdir); @@ -240,7 +240,7 @@ export class Runner { return filesByProject; } - private async _loadAllTests(options: RunOptions): Promise<{ rootSuite: Suite, testGroups: TestGroup[] }> { + private async _loadAllTests(options: RunOptions, errors: TestError[]): Promise<{ rootSuite: Suite, testGroups: TestGroup[] }> { const config = this._configLoader.fullConfig(); const projects = this._collectProjects(options.projectFilter); const filesByProject = await this._collectFiles(projects, options.testFileFilters); @@ -249,10 +249,10 @@ export class Runner { files.forEach(file => allTestFiles.add(file)); // Load all tests. - const preprocessRoot = await this._loadTests(allTestFiles); + const preprocessRoot = await this._loadTests(allTestFiles, errors); // Complain about duplicate titles. - createDuplicateTitlesErrors(config, preprocessRoot).forEach(e => this._reporter.onError(e)); + errors.push(...createDuplicateTitlesErrors(config, preprocessRoot)); // Filter tests to respect line/column filter. filterByFocusedLine(preprocessRoot, options.testFileFilters); @@ -261,7 +261,7 @@ export class Runner { if (config.forbidOnly) { const onlyTestsAndSuites = preprocessRoot._getOnlyItems(); if (onlyTestsAndSuites.length > 0) - createForbidOnlyErrors(config, onlyTestsAndSuites).forEach(e => this._reporter.onError(e)); + errors.push(...createForbidOnlyErrors(config, onlyTestsAndSuites)); } // Filter only. @@ -316,7 +316,7 @@ export class Runner { return rootSuite; } - private async _loadTests(testFiles: Set): Promise { + private async _loadTests(testFiles: Set, errors: TestError[]): Promise { if (process.env.PW_TEST_OOP_LOADER) { const loaderHost = new LoaderHost(); await loaderHost.start(this._configLoader.serializedConfig()); @@ -326,11 +326,11 @@ export class Runner { await loaderHost.stop(); } } - const loadErrors: TestError[] = []; + const loadErrors: LoadError[] = []; try { return await loadTestFilesInProcess(this._configLoader.fullConfig(), [...testFiles], loadErrors); } finally { - loadErrors.forEach(e => this._reporter.onError(e)); + errors.push(...loadErrors); } } @@ -390,7 +390,6 @@ export class Runner { this._plugins.push(dockerPlugin); this._reporter = await this._createReporter(options.listOnly); - setFatalErrorSink(error => this._reporter.onError(error)); const taskRunner = new TaskRunner(this._reporter, config.globalTimeout); // Setup the plugins. @@ -419,21 +418,14 @@ export class Runner { // Load tests. let loadedTests!: { rootSuite: Suite, testGroups: TestGroup[] }; - taskRunner.addTask('load tests', async () => { - loadedTests = await this._loadAllTests(options); - if (this._reporter.hasErrors) { - status = 'failed'; - taskRunner.stop(); + taskRunner.addTask('load tests', async ({ errors }) => { + loadedTests = await this._loadAllTests(options, errors); + if (errors.length) return; - } // Fail when no tests. - if (!loadedTests.rootSuite.allTests().length && !options.passWithNoTests) { - this._reporter.onError(createNoTestsError()); - status = 'failed'; - taskRunner.stop(); - return; - } + if (!loadedTests.rootSuite.allTests().length && !options.passWithNoTests) + throw new Error(`No tests found`); if (!options.listOnly) { this._filterForCurrentShard(loadedTests.rootSuite, loadedTests.testGroups); @@ -444,11 +436,7 @@ export class Runner { if (!options.listOnly) { taskRunner.addTask('prepare to run', async () => { // Remove output directores. - if (!await this._removeOutputDirs(options)) { - status = 'failed'; - taskRunner.stop(); - return; - } + await this._removeOutputDirs(options); }); taskRunner.addTask('plugin begin', async () => { @@ -509,7 +497,7 @@ export class Runner { return status; } - private async _removeOutputDirs(options: RunOptions): Promise { + private async _removeOutputDirs(options: RunOptions) { const config = this._configLoader.fullConfig(); const outputDirs = new Set(); for (const p of config.projects) { @@ -517,23 +505,17 @@ export class Runner { outputDirs.add(p.outputDir); } - try { - await Promise.all(Array.from(outputDirs).map(outputDir => removeFolderAsync(outputDir).catch(async (error: any) => { - if ((error as any).code === 'EBUSY') { - // We failed to remove folder, might be due to the whole folder being mounted inside a container: - // https://github.com/microsoft/playwright/issues/12106 - // Do a best-effort to remove all files inside of it instead. - const entries = await readDirAsync(outputDir).catch(e => []); - await Promise.all(entries.map(entry => removeFolderAsync(path.join(outputDir, entry)))); - } else { - throw error; - } - }))); - } catch (e) { - this._reporter.onError(serializeError(e)); - return false; - } - return true; + await Promise.all(Array.from(outputDirs).map(outputDir => removeFolderAsync(outputDir).catch(async (error: any) => { + if ((error as any).code === 'EBUSY') { + // We failed to remove folder, might be due to the whole folder being mounted inside a container: + // https://github.com/microsoft/playwright/issues/12106 + // Do a best-effort to remove all files inside of it instead. + const entries = await readDirAsync(outputDir).catch(e => []); + await Promise.all(entries.map(entry => removeFolderAsync(path.join(outputDir, entry)))); + } else { + throw error; + } + }))); } } @@ -807,14 +789,6 @@ function createDuplicateTitlesErrors(config: FullConfigInternal, rootSuite: Suit return errors; } -function createNoTestsError(): TestError { - return createStacklessError(`=================\n no tests found.\n=================`); -} - -function createStacklessError(message: string, location?: TestError['location']): TestError { - return { message, location }; -} - function sanitizeConfigForJSON(object: any, visited: Set): any { const type = typeof object; if (type === 'function' || type === 'symbol') diff --git a/packages/playwright-test/src/taskRunner.ts b/packages/playwright-test/src/taskRunner.ts index e1f04095cb..c232261713 100644 --- a/packages/playwright-test/src/taskRunner.ts +++ b/packages/playwright-test/src/taskRunner.ts @@ -16,12 +16,12 @@ import { debug } from 'playwright-core/lib/utilsBundle'; import { ManualPromise, monotonicTime } from 'playwright-core/lib/utils'; -import type { FullResult, Reporter } from '../reporter'; +import type { FullResult, Reporter, TestError } from '../reporter'; import { SigIntWatcher } from './sigIntWatcher'; import { serializeError } from './util'; type TaskTeardown = () => Promise | undefined; -type Task = () => Promise | undefined; +type Task = (params: { errors: TestError[] }) => Promise | undefined; export class TaskRunner { private _tasks: { name: string, task: Task }[] = []; @@ -58,16 +58,22 @@ export class TaskRunner { if (this._interrupted) break; debug('pw:test:task')(`"${name}" started`); + const errors: TestError[] = []; try { - const teardown = await task(); + const teardown = await task({ errors }); if (teardown) teardownRunner._tasks.unshift({ name: `teardown for ${name}`, task: teardown }); } catch (e) { debug('pw:test:task')(`error in "${name}": `, e); - this._reporter.onError?.(serializeError(e)); - if (!this._isTearDown) - this._interrupted = true; - this._hasErrors = true; + errors.push(serializeError(e)); + } finally { + for (const error of errors) + this._reporter.onError?.(error); + if (errors.length) { + if (!this._isTearDown) + this._interrupted = true; + this._hasErrors = true; + } } debug('pw:test:task')(`"${name}" finished`); } diff --git a/packages/playwright-test/src/testLoader.ts b/packages/playwright-test/src/testLoader.ts index d9d181d800..bbd2f7da42 100644 --- a/packages/playwright-test/src/testLoader.ts +++ b/packages/playwright-test/src/testLoader.ts @@ -17,6 +17,7 @@ import path from 'path'; import type { TestError } from '../reporter'; import type { FullConfigInternal } from './types'; +import type { LoadError } from './fixtures'; import { setCurrentlyLoadingFileSuite } from './globals'; import { PoolBuilder } from './poolBuilder'; import { Suite } from './test'; @@ -79,7 +80,7 @@ export class TestLoader { } } -export async function loadTestFilesInProcess(config: FullConfigInternal, testFiles: string[], loadErrors: TestError[]): Promise { +export async function loadTestFilesInProcess(config: FullConfigInternal, testFiles: string[], loadErrors: LoadError[]): Promise { const testLoader = new TestLoader(config); const rootSuite = new Suite('', 'root'); for (const file of testFiles) { @@ -87,6 +88,6 @@ export async function loadTestFilesInProcess(config: FullConfigInternal, testFil rootSuite._addSuite(fileSuite); } // Generate hashes. - PoolBuilder.buildForLoader(rootSuite); + PoolBuilder.buildForLoader(rootSuite, loadErrors); return rootSuite; } diff --git a/packages/playwright-test/src/testType.ts b/packages/playwright-test/src/testType.ts index bf0c261c1f..c13ea04813 100644 --- a/packages/playwright-test/src/testType.ts +++ b/packages/playwright-test/src/testType.ts @@ -15,7 +15,7 @@ */ import { expect } from './expect'; -import { currentlyLoadingFileSuite, currentTestInfo, addFatalError, setCurrentlyLoadingFileSuite } from './globals'; +import { currentlyLoadingFileSuite, currentTestInfo, setCurrentlyLoadingFileSuite } from './globals'; import { TestCase, Suite } from './test'; import { wrapFunctionWithLocation } from './transform'; import type { Fixtures, FixturesWithLocation, Location, TestType } from './types'; @@ -68,15 +68,14 @@ export class TestTypeImpl { private _currentSuite(location: Location, title: string): Suite | undefined { const suite = currentlyLoadingFileSuite(); if (!suite) { - addFatalError([ + throw new Error([ `Playwright Test did not expect ${title} to be called here.`, `Most common reasons include:`, `- You are calling ${title} in a configuration file.`, `- You are calling ${title} in a file that is imported by the configuration file.`, `- You have two different versions of @playwright/test. This usually happens`, ` when one of the dependencies in your package.json depends on @playwright/test.`, - ].join('\n'), location); - return; + ].join('\n')); } return suite; } @@ -123,7 +122,7 @@ export class TestTypeImpl { for (let parent: Suite | undefined = suite; parent; parent = parent.parent) { if (parent._parallelMode === 'serial' && child._parallelMode === 'parallel') - addFatalError('describe.parallel cannot be nested inside describe.serial', location); + throw new Error('describe.parallel cannot be nested inside describe.serial'); } setCurrentlyLoadingFileSuite(child); @@ -152,11 +151,11 @@ export class TestTypeImpl { if (options.mode !== undefined) { if (suite._parallelMode !== 'default') - addFatalError('Parallel mode is already assigned for the enclosing scope.', location); + throw new Error('Parallel mode is already assigned for the enclosing scope.'); suite._parallelMode = options.mode; for (let parent: Suite | undefined = suite.parent; parent; parent = parent.parent) { if (parent._parallelMode === 'serial' && suite._parallelMode === 'parallel') - addFatalError('describe.parallel cannot be nested inside describe.serial', location); + throw new Error('describe.parallel cannot be nested inside describe.serial'); } } } @@ -182,12 +181,10 @@ export class TestTypeImpl { } const testInfo = currentTestInfo(); - if (!testInfo) { - addFatalError(`test.${type}() can only be called inside test, describe block or fixture`, location); - return; - } + if (!testInfo) + throw new Error(`test.${type}() can only be called inside test, describe block or fixture`); if (typeof modifierArgs[0] === 'function') - addFatalError(`test.${type}() with a function can only be called inside describe block`, location); + throw new Error(`test.${type}() with a function can only be called inside describe block`); testInfo[type](...modifierArgs as [any, any]); } @@ -199,10 +196,8 @@ export class TestTypeImpl { } const testInfo = currentTestInfo(); - if (!testInfo) { - addFatalError(`test.setTimeout() can only be called from a test`, location); - return; - } + if (!testInfo) + throw new Error(`test.setTimeout() can only be called from a test`); testInfo.setTimeout(timeout); } @@ -215,10 +210,8 @@ export class TestTypeImpl { private async _step(location: Location, title: string, body: () => Promise): Promise { const testInfo = currentTestInfo(); - if (!testInfo) { - addFatalError(`test.step() can only be called from a test`, location); - return undefined as any; - } + if (!testInfo) + throw new Error(`test.step() can only be called from a test`); const step = testInfo._addStep({ category: 'test.step', title, diff --git a/tests/playwright-test/exit-code.spec.ts b/tests/playwright-test/exit-code.spec.ts index d007d67540..b5474d6f58 100644 --- a/tests/playwright-test/exit-code.spec.ts +++ b/tests/playwright-test/exit-code.spec.ts @@ -141,7 +141,7 @@ test('should exit with code 1 if the specified folder does not exist', async ({ `, }); expect(result.exitCode).toBe(1); - expect(result.output).toContain(`no tests found.`); + expect(result.output).toContain(`No tests found`); }); test('should exit with code 1 if passed a file name', async ({ runInlineTest }) => { @@ -153,7 +153,7 @@ test('should exit with code 1 if passed a file name', async ({ runInlineTest }) `, }); expect(result.exitCode).toBe(1); - expect(result.output).toContain(`no tests found.`); + expect(result.output).toContain(`No tests found`); }); test('should exit with code 0 with --pass-with-no-tests', async ({ runInlineTest }) => { diff --git a/tests/playwright-test/reporter-base.spec.ts b/tests/playwright-test/reporter-base.spec.ts index 76e1d32821..6c043a2377 100644 --- a/tests/playwright-test/reporter-base.spec.ts +++ b/tests/playwright-test/reporter-base.spec.ts @@ -298,7 +298,7 @@ test('should print errors with inconsistent message/stack', async ({ runInlineTe test('should print "no tests found" error', async ({ runInlineTest }) => { const result = await runInlineTest({ }); expect(result.exitCode).toBe(1); - expect(result.output).toContain('no tests found.'); + expect(result.output).toContain('No tests found'); }); test('should not crash on undefined body with manual attachments', async ({ runInlineTest }) => { diff --git a/tests/playwright-test/reporter.spec.ts b/tests/playwright-test/reporter.spec.ts index 3495064f65..23e24f39ef 100644 --- a/tests/playwright-test/reporter.spec.ts +++ b/tests/playwright-test/reporter.spec.ts @@ -489,7 +489,7 @@ test('should report no-tests error to reporter', async ({ runInlineTest }) => { }, { 'reporter': '' }); expect(result.exitCode).toBe(1); - expect(result.output).toContain(`%%got error: =================\n no tests found.`); + expect(result.output).toContain(`%%got error: No tests found`); }); test('should report require error to reporter', async ({ runInlineTest }) => {