From 269a293ba1f0809ba5af5de46d62cb0ca60e0bed Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 16 Feb 2024 12:43:13 -0800 Subject: [PATCH] chore(test runner): allow TestInfoImpl without a TestCase (#29534) This will be useful to run `beforeAll`/`afterAll` hooks with a separate `TestInfo` instance, as well as run use helpers like `_runAndFailOnError()` during scope teardown. --- .../playwright/src/worker/fixtureRunner.ts | 24 ++++++------- packages/playwright/src/worker/testInfo.ts | 36 +++++++++---------- packages/playwright/src/worker/workerMain.ts | 26 +++++++------- tests/playwright-test/test-tag.spec.ts | 5 --- 4 files changed, 42 insertions(+), 49 deletions(-) diff --git a/packages/playwright/src/worker/fixtureRunner.ts b/packages/playwright/src/worker/fixtureRunner.ts index 971e72414f..2aed5c08f7 100644 --- a/packages/playwright/src/worker/fixtureRunner.ts +++ b/packages/playwright/src/worker/fixtureRunner.ts @@ -17,7 +17,7 @@ import { formatLocation, debugTest, filterStackFile } from '../util'; import { ManualPromise } from 'playwright-core/lib/utils'; import type { TestInfoImpl, TestStepInternal } from './testInfo'; -import type { FixtureDescription, TimeoutManager } from './timeoutManager'; +import type { FixtureDescription } from './timeoutManager'; import { fixtureParameterNames, type FixturePool, type FixtureRegistration, type FixtureScope } from '../common/fixtures'; import type { WorkerInfo } from '../../types/test'; import type { Location } from '../../types/testReporter'; @@ -136,21 +136,21 @@ class Fixture { testInfo._timeoutManager.setCurrentFixture(undefined); } - async teardown(timeoutManager: TimeoutManager) { + async teardown(testInfo: TestInfoImpl) { if (this._teardownWithDepsComplete) { // When we are waiting for the teardown for the second time, // most likely after the first time did timeout, annotate current fixture // for better error messages. - this._setTeardownDescription(timeoutManager); + this._setTeardownDescription(testInfo); await this._teardownWithDepsComplete; - timeoutManager.setCurrentFixture(undefined); + testInfo._timeoutManager.setCurrentFixture(undefined); return; } - this._teardownWithDepsComplete = this._teardownInternal(timeoutManager); + this._teardownWithDepsComplete = this._teardownInternal(testInfo); await this._teardownWithDepsComplete; } - private async _teardownInternal(timeoutManager: TimeoutManager) { + private async _teardownInternal(testInfo: TestInfoImpl) { if (typeof this.registration.fn !== 'function') return; try { @@ -161,10 +161,10 @@ class Fixture { } if (this._useFuncFinished) { debugTest(`teardown ${this.registration.name}`); - this._setTeardownDescription(timeoutManager); + this._setTeardownDescription(testInfo); this._useFuncFinished.resolve(); await this._selfTeardownComplete; - timeoutManager.setCurrentFixture(undefined); + testInfo._timeoutManager.setCurrentFixture(undefined); } } finally { for (const dep of this._deps) @@ -173,9 +173,9 @@ class Fixture { } } - private _setTeardownDescription(timeoutManager: TimeoutManager) { + private _setTeardownDescription(testInfo: TestInfoImpl) { this._runnableDescription.phase = 'teardown'; - timeoutManager.setCurrentFixture(this._runnableDescription); + testInfo._timeoutManager.setCurrentFixture(this._runnableDescription); } _collectFixturesInTeardownOrder(scope: FixtureScope, collector: Set) { @@ -206,14 +206,14 @@ export class FixtureRunner { this.pool = pool; } - async teardownScope(scope: FixtureScope, timeoutManager: TimeoutManager, onFixtureError: (error: Error) => void) { + async teardownScope(scope: FixtureScope, testInfo: TestInfoImpl, onFixtureError: (error: Error) => void) { // Teardown fixtures in the reverse order. const fixtures = Array.from(this.instanceForId.values()).reverse(); const collector = new Set(); for (const fixture of fixtures) fixture._collectFixturesInTeardownOrder(scope, collector); for (const fixture of collector) - await fixture.teardown(timeoutManager).catch(onFixtureError); + await fixture.teardown(testInfo).catch(onFixtureError); if (scope === 'test') this.testScopeClean = true; } diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index 74eacc128f..e79bd61529 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -52,7 +52,6 @@ export class TestInfoImpl implements TestInfo { private _onStepBegin: (payload: StepBeginPayload) => void; private _onStepEnd: (payload: StepEndPayload) => void; private _onAttach: (payload: AttachmentPayload) => void; - readonly _test: TestCase; readonly _timeoutManager: TimeoutManager; readonly _startTime: number; readonly _startWallTime: number; @@ -62,6 +61,7 @@ export class TestInfoImpl implements TestInfo { _didTimeout = false; _wasInterrupted = false; _lastStepId = 0; + private readonly _requireFile: string; readonly _projectInternal: FullProjectInternal; readonly _configInternal: FullConfigInternal; readonly _steps: TestStepInternal[] = []; @@ -129,19 +129,19 @@ export class TestInfoImpl implements TestInfo { configInternal: FullConfigInternal, projectInternal: FullProjectInternal, workerParams: WorkerInitParams, - test: TestCase, + test: TestCase | undefined, retry: number, onStepBegin: (payload: StepBeginPayload) => void, onStepEnd: (payload: StepEndPayload) => void, onAttach: (payload: AttachmentPayload) => void, ) { - this._test = test; - this.testId = test.id; + this.testId = test?.id ?? ''; this._onStepBegin = onStepBegin; this._onStepEnd = onStepEnd; this._onAttach = onAttach; this._startTime = monotonicTime(); this._startWallTime = Date.now(); + this._requireFile = test?._requireFile ?? ''; this.repeatEachIndex = workerParams.repeatEachIndex; this.retry = retry; @@ -151,20 +151,20 @@ export class TestInfoImpl implements TestInfo { this.project = projectInternal.project; this._configInternal = configInternal; this.config = configInternal.config; - this.title = test.title; - this.titlePath = test.titlePath(); - this.file = test.location.file; - this.line = test.location.line; - this.column = test.location.column; - this.fn = test.fn; - this.expectedStatus = test.expectedStatus; + this.title = test?.title ?? ''; + this.titlePath = test?.titlePath() ?? []; + this.file = test?.location.file ?? ''; + this.line = test?.location.line ?? 0; + this.column = test?.location.column ?? 0; + this.fn = test?.fn ?? (() => {}); + this.expectedStatus = test?.expectedStatus ?? 'skipped'; this._timeoutManager = new TimeoutManager(this.project.timeout); this.outputDir = (() => { - const relativeTestFilePath = path.relative(this.project.testDir, test._requireFile.replace(/\.(spec|test)\.(js|ts|mjs)$/, '')); + const relativeTestFilePath = path.relative(this.project.testDir, this._requireFile.replace(/\.(spec|test)\.(js|ts|mjs)$/, '')); const sanitizedRelativePath = relativeTestFilePath.replace(process.platform === 'win32' ? new RegExp('\\\\', 'g') : new RegExp('/', 'g'), '-'); - const fullTitleWithoutSpec = test.titlePath().slice(1).join(' '); + const fullTitleWithoutSpec = this.titlePath.slice(1).join(' '); let testOutputDir = trimLongString(sanitizedRelativePath + '-' + sanitizeForFilePath(fullTitleWithoutSpec)); if (projectInternal.id) @@ -177,7 +177,7 @@ export class TestInfoImpl implements TestInfo { })(); this.snapshotDir = (() => { - const relativeTestFilePath = path.relative(this.project.testDir, test._requireFile); + const relativeTestFilePath = path.relative(this.project.testDir, this._requireFile); return path.join(this.project.snapshotDir, relativeTestFilePath + '-snapshots'); })(); @@ -328,7 +328,7 @@ export class TestInfoImpl implements TestInfo { } const payload: StepEndPayload = { - testId: this._test.id, + testId: this.testId, stepId, wallTime: step.endWallTime, error: step.error, @@ -344,7 +344,7 @@ export class TestInfoImpl implements TestInfo { const parentStepList = parentStep ? parentStep.steps : this._steps; parentStepList.push(step); const payload: StepBeginPayload = { - testId: this._test.id, + testId: this.testId, stepId, parentStepId: parentStep ? parentStep.stepId : undefined, title: data.title, @@ -434,7 +434,7 @@ export class TestInfoImpl implements TestInfo { }); this._attachmentsPush(attachment); this._onAttach({ - testId: this._test.id, + testId: this.testId, name: attachment.name, contentType: attachment.contentType, path: attachment.path, @@ -465,7 +465,7 @@ export class TestInfoImpl implements TestInfo { snapshotPath(...pathSegments: string[]) { const subPath = path.join(...pathSegments); const parsedSubPath = path.parse(subPath); - const relativeTestFilePath = path.relative(this.project.testDir, this._test._requireFile); + const relativeTestFilePath = path.relative(this.project.testDir, this._requireFile); const parsedRelativeTestFilePath = path.parse(relativeTestFilePath); const projectNamePathSegment = sanitizeForFilePath(this.project.name); diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index 88a0507c24..3b4bd32b65 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -24,7 +24,6 @@ import type { Annotation, FullConfigInternal, FullProjectInternal } from '../com import { FixtureRunner } from './fixtureRunner'; import { ManualPromise, gracefullyCloseAll, removeFolders } from 'playwright-core/lib/utils'; import { TestInfoImpl } from './testInfo'; -import { TimeoutManager } from './timeoutManager'; import { ProcessRunner } from '../common/process'; import { loadTestFile } from '../common/testLoader'; import { applyRepeatEachIndex, bindFileSuiteToProject, filterTestsRemoveEmptySuites } from '../common/suiteUtils'; @@ -53,7 +52,7 @@ export class WorkerMain extends ProcessRunner { // This promise resolves once the single "run test group" call finishes. private _runFinished = new ManualPromise(); private _currentTest: TestInfoImpl | null = null; - private _lastRunningTests: TestInfoImpl[] = []; + private _lastRunningTests: TestCase[] = []; private _totalRunningTests = 0; // Suites that had their beforeAll hooks, but not afterAll hooks executed. // These suites still need afterAll hooks to be executed for the proper cleanup. @@ -129,7 +128,7 @@ export class WorkerMain extends ProcessRunner { '', '', colors.red(`Failed worker ran ${count}${lastMessage}:`), - ...this._lastRunningTests.map(testInfo => formatTestTitle(testInfo._test, testInfo.project.name)), + ...this._lastRunningTests.map(test => formatTestTitle(test, this._project.project.name)), ].join('\n'); if (error.message) { if (error.stack) { @@ -153,7 +152,7 @@ export class WorkerMain extends ProcessRunner { private async _teardownScopeAndReturnFirstError(scope: FixtureScope, testInfo: TestInfoImpl): Promise { let error: Error | undefined; - await this._fixtureRunner.teardownScope(scope, testInfo._timeoutManager, e => { + await this._fixtureRunner.teardownScope(scope, testInfo, e => { testInfo._failWithError(e, true, false); if (error === undefined) error = e; @@ -163,15 +162,14 @@ export class WorkerMain extends ProcessRunner { private async _teardownScopes() { // TODO: separate timeout for teardown? - const timeoutManager = new TimeoutManager(this._project.project.timeout); - await timeoutManager.withRunnable({ type: 'teardown' }, async () => { - const timeoutError = await timeoutManager.runWithTimeout(async () => { - await this._fixtureRunner.teardownScope('test', timeoutManager, e => this._fatalErrors.push(serializeError(e))); - await this._fixtureRunner.teardownScope('worker', timeoutManager, e => this._fatalErrors.push(serializeError(e))); + const fakeTestInfo = new TestInfoImpl(this._config, this._project, this._params, undefined, 0, () => {}, () => {}, () => {}); + await fakeTestInfo._timeoutManager.withRunnable({ type: 'teardown' }, async () => { + await fakeTestInfo._runWithTimeout(async () => { + await this._teardownScopeAndReturnFirstError('test', fakeTestInfo); + await this._teardownScopeAndReturnFirstError('worker', fakeTestInfo); }); - if (timeoutError) - this._fatalErrors.push(serializeError(timeoutError)); }); + this._fatalErrors.push(...fakeTestInfo.errors); } unhandledError(error: Error | any) { @@ -322,7 +320,7 @@ export class WorkerMain extends ProcessRunner { } this._totalRunningTests++; - this._lastRunningTests.push(testInfo); + this._lastRunningTests.push(test); if (this._lastRunningTests.length > 10) this._lastRunningTests.shift(); let didFailBeforeAllForSuite: Suite | undefined; @@ -603,14 +601,14 @@ export class WorkerMain extends ProcessRunner { function buildTestBeginPayload(testInfo: TestInfoImpl): TestBeginPayload { return { - testId: testInfo._test.id, + testId: testInfo.testId, startWallTime: testInfo._startWallTime, }; } function buildTestEndPayload(testInfo: TestInfoImpl): TestEndPayload { return { - testId: testInfo._test.id, + testId: testInfo.testId, duration: testInfo.duration, status: testInfo.status!, errors: testInfo.errors, diff --git a/tests/playwright-test/test-tag.spec.ts b/tests/playwright-test/test-tag.spec.ts index 5732c8ccd7..e442c2acaa 100644 --- a/tests/playwright-test/test-tag.spec.ts +++ b/tests/playwright-test/test-tag.spec.ts @@ -42,13 +42,10 @@ test('should have correct tags', async ({ runInlineTest }) => { 'stdio.spec.js': ` import { test, expect } from '@playwright/test'; test('no-tags', () => { - expect(test.info()._test.tags).toEqual([]); }); test('foo-tag @inline', { tag: '@foo' }, () => { - expect(test.info()._test.tags).toEqual(['@inline', '@foo']); }); test('foo-bar-tags', { tag: ['@foo', '@bar'] }, () => { - expect(test.info()._test.tags).toEqual(['@foo', '@bar']); }); test.skip('skip-foo-tag', { tag: '@foo' }, () => { }); @@ -59,11 +56,9 @@ test('should have correct tags', async ({ runInlineTest }) => { }); test.describe('suite @inline', { tag: '@foo' }, () => { test('foo-suite', () => { - expect(test.info()._test.tags).toEqual(['@inline', '@foo']); }); test.describe('inner', { tag: '@bar' }, () => { test('foo-bar-suite', () => { - expect(test.info()._test.tags).toEqual(['@inline', '@foo', '@bar']); }); }); });