Filip/playwright
1
0
Fork 0
Code Issues Pull requests Actions Packages Projects Releases Wiki Activity

chore(test runner): make FixturePool.registrations private (#29539)

Browse source 
This way everyone resolves through a public function instead of a map
lookup, which is sometimes wrong.
This commit is contained in:
Dmitry Gozman 2024-02-16 20:29:33 -08:00 committed by GitHub
parent ce55cdffb8
commit f414227ed8
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 21 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
packages/playwright/src/common/fixtures.ts
View file

@ -72,11 +72,11 @@ function isFixtureOption(value: any): value is FixtureTuple {
export class FixturePool { export class FixturePool {
readonly digest: string; readonly digest: string;
readonly registrations: Map<string, FixtureRegistration>; private readonly _registrations: Map<string, FixtureRegistration>;
private _onLoadError: LoadErrorSink; private _onLoadError: LoadErrorSink;
constructor(fixturesList: FixturesWithLocation[], onLoadError: LoadErrorSink, parentPool?: FixturePool, disallowWorkerFixtures?: boolean, optionOverrides?: OptionOverrides) { constructor(fixturesList: FixturesWithLocation[], onLoadError: LoadErrorSink, parentPool?: FixturePool, disallowWorkerFixtures?: boolean, optionOverrides?: OptionOverrides) {
this.registrations = new Map(parentPool ? parentPool.registrations : []); this._registrations = new Map(parentPool ? parentPool._registrations : []);
this._onLoadError = onLoadError; this._onLoadError = onLoadError;
const allOverrides = optionOverrides?.overrides ?? {}; const allOverrides = optionOverrides?.overrides ?? {};
@ -117,7 +117,7 @@ export class FixturePool {
} }
let fn = value as (Function | any); let fn = value as (Function | any);
const previous = this.registrations.get(name); const previous = this._registrations.get(name);
if (previous && options) { if (previous && options) {
if (previous.scope !== options.scope) { if (previous.scope !== options.scope) {
this._addLoadError(`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);
@ -154,7 +154,7 @@ export class FixturePool {
const deps = fixtureParameterNames(fn, location, e => this._onLoadError(e)); 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, hideStep: options.hideStep, deps, super: previous, optionOverride: isOptionsOverride }; const registration: FixtureRegistration = { id: '', name, location, scope: options.scope, fn, auto: options.auto, option: options.option, timeout: options.timeout, customTitle: options.customTitle, hideStep: options.hideStep, deps, super: previous, optionOverride: isOptionsOverride };
registrationId(registration); registrationId(registration);
this.registrations.set(name, registration); this._registrations.set(name, registration);
} }
} }
@ -165,7 +165,7 @@ export class FixturePool {
markers.set(registration, 'visiting'); markers.set(registration, 'visiting');
stack.push(registration); stack.push(registration);
for (const name of registration.deps) { for (const name of registration.deps) {
const dep = this.resolveDependency(registration, name); const dep = this.resolve(name, registration);
if (!dep) { if (!dep) {
if (name === registration.name) if (name === registration.name)
this._addLoadError(`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);
@ -192,9 +192,9 @@ export class FixturePool {
}; };
const hash = crypto.createHash('sha1'); const hash = crypto.createHash('sha1');
const names = Array.from(this.registrations.keys()).sort(); const names = Array.from(this._registrations.keys()).sort();
for (const name of names) { for (const name of names) {
const registration = this.registrations.get(name)!; const registration = this._registrations.get(name)!;
visit(registration); visit(registration);
if (registration.scope === 'worker') if (registration.scope === 'worker')
hash.update(registration.id + ';'); hash.update(registration.id + ';');
@ -204,16 +204,20 @@ export class FixturePool {
validateFunction(fn: Function, prefix: string, location: Location) { validateFunction(fn: Function, prefix: string, location: Location) {
for (const name of fixtureParameterNames(fn, location, e => this._onLoadError(e))) { for (const name of fixtureParameterNames(fn, location, e => this._onLoadError(e))) {
const registration = this.registrations.get(name); const registration = this._registrations.get(name);
if (!registration) if (!registration)
this._addLoadError(`${prefix} has unknown parameter "${name}".`, location); this._addLoadError(`${prefix} has unknown parameter "${name}".`, location);
} }
} }
resolveDependency(registration: FixtureRegistration, name: string): FixtureRegistration | undefined { resolve(name: string, forFixture?: FixtureRegistration): FixtureRegistration | undefined {
if (name === registration.name) if (name === forFixture?.name)
return registration.super; return forFixture.super;
return this.registrations.get(name); return this._registrations.get(name);
}
autoFixtures() {
return [...this._registrations.values()].filter(r => r.auto !== false);
} }
private _addLoadError(message: string, location: Location) { private _addLoadError(message: string, location: Location) {

10
packages/playwright/src/worker/fixtureRunner.ts
View file

@ -59,7 +59,7 @@ class Fixture {
const params: { [key: string]: any } = {}; const params: { [key: string]: any } = {};
for (const name of this.registration.deps) { for (const name of this.registration.deps) {
const registration = this.runner.pool!.resolveDependency(this.registration, name)!; const registration = this.runner.pool!.resolve(name, this.registration)!;
const dep = await this.runner.setupFixtureForRegistration(registration, testInfo); const dep = await this.runner.setupFixtureForRegistration(registration, testInfo);
// Fixture teardown is root => leafs, when we need to teardown a fixture, // Fixture teardown is root => leafs, when we need to teardown a fixture,
// it recursively tears down its usages first. // it recursively tears down its usages first.
@ -221,9 +221,7 @@ export class FixtureRunner {
async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise<object | null> { async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only'): Promise<object | null> {
// Install automatic fixtures. // Install automatic fixtures.
const auto: FixtureRegistration[] = []; const auto: FixtureRegistration[] = [];
for (const registration of this.pool!.registrations.values()) { for (const registration of this.pool!.autoFixtures()) {
if (registration.auto === false)
continue;
let shouldRun = true; let shouldRun = true;
if (autoFixtures === 'all-hooks-only') if (autoFixtures === 'all-hooks-only')
shouldRun = registration.scope === 'worker' || registration.auto === 'all-hooks-included'; shouldRun = registration.scope === 'worker' || registration.auto === 'all-hooks-included';
@ -243,7 +241,7 @@ export class FixtureRunner {
const names = getRequiredFixtureNames(fn); const names = getRequiredFixtureNames(fn);
const params: { [key: string]: any } = {}; const params: { [key: string]: any } = {};
for (const name of names) { for (const name of names) {
const registration = this.pool!.registrations.get(name)!; const registration = this.pool!.resolve(name)!;
const fixture = await this.setupFixtureForRegistration(registration, testInfo); const fixture = await this.setupFixtureForRegistration(registration, testInfo);
if (fixture.failed) if (fixture.failed)
return null; return null;
@ -278,7 +276,7 @@ export class FixtureRunner {
dependsOnWorkerFixturesOnly(fn: Function, location: Location): boolean { dependsOnWorkerFixturesOnly(fn: Function, location: Location): boolean {
const names = getRequiredFixtureNames(fn, location); const names = getRequiredFixtureNames(fn, location);
for (const name of names) { for (const name of names) {
const registration = this.pool!.registrations.get(name)!; const registration = this.pool!.resolve(name)!;
if (registration.scope !== 'worker') if (registration.scope !== 'worker')
return false; return false;
} }

2
packages/playwright/src/worker/workerMain.ts
View file

@ -332,7 +332,7 @@ export class WorkerMain extends ProcessRunner {
// test runner instead of a fixture belonging to Playwright. // test runner instead of a fixture belonging to Playwright.
// However, for backwards compatibility, we have to read it from a fixture today. // However, for backwards compatibility, we have to read it from a fixture today.
// We decided to not introduce the config-level option just yet. // We decided to not introduce the config-level option just yet.
const traceFixtureRegistration = test._pool!.registrations.get('trace'); const traceFixtureRegistration = test._pool!.resolve('trace');
if (!traceFixtureRegistration) if (!traceFixtureRegistration)
return; return;
if (typeof traceFixtureRegistration.fn === 'function') if (typeof traceFixtureRegistration.fn === 'function')