diff --git a/packages/playwright/src/index.ts b/packages/playwright/src/index.ts index 28c6cb48f9..04d95491ef 100644 --- a/packages/playwright/src/index.ts +++ b/packages/playwright/src/index.ts @@ -256,7 +256,6 @@ const playwrightFixtures: Fixtures = ({ apiName, params, wallTime, - laxParent: true, }); userData.userObject = step; }, diff --git a/packages/playwright/src/matchers/expect.ts b/packages/playwright/src/matchers/expect.ts index 8206579453..0b6b3da8ad 100644 --- a/packages/playwright/src/matchers/expect.ts +++ b/packages/playwright/src/matchers/expect.ts @@ -267,7 +267,6 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler { params: args[0] ? { expected: args[0] } : undefined, wallTime, infectParentStepsWithError: this._info.isSoft, - laxParent: true, isSoft: this._info.isSoft, }; diff --git a/packages/playwright/src/worker/fixtureRunner.ts b/packages/playwright/src/worker/fixtureRunner.ts index 7f320a2b5b..4343bdc9cc 100644 --- a/packages/playwright/src/worker/fixtureRunner.ts +++ b/packages/playwright/src/worker/fixtureRunner.ts @@ -99,7 +99,7 @@ class Fixture { title: `fixture: ${this.registration.name}`, category: 'fixture', location: isInternalFixture ? this.registration.location : undefined, - }, testInfo._afterHooksStep); + }); mutableStepOnStack!.stepId = afterStep.stepId; } }; diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index 0f00006e1e..3faf963114 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -33,12 +33,11 @@ export interface TestStepInternal { complete(result: { error?: Error, attachments?: Attachment[] }): void; stepId: string; title: string; - category: string; + category: 'hook' | 'fixture' | 'test.step' | string; wallTime: number; location?: Location; boxedStack?: StackFrame[]; steps: TestStepInternal[]; - laxParent?: boolean; endWallTime?: number; apiName?: string; params?: Record; @@ -46,6 +45,7 @@ export interface TestStepInternal { infectParentStepsWithError?: boolean; box?: boolean; isSoft?: boolean; + forceNoParent?: boolean; } export class TestInfoImpl implements TestInfo { @@ -65,8 +65,6 @@ export class TestInfoImpl implements TestInfo { readonly _projectInternal: FullProjectInternal; readonly _configInternal: FullConfigInternal; readonly _steps: TestStepInternal[] = []; - _beforeHooksStep: TestStepInternal | undefined; - _afterHooksStep: TestStepInternal | undefined; _onDidFinishTestFunction: (() => Promise) | undefined; _hasNonRetriableError = false; @@ -250,24 +248,40 @@ export class TestInfoImpl implements TestInfo { } } - _addStep(data: Omit, parentStep?: TestStepInternal): TestStepInternal { + private _findLastNonFinishedStep(filter: (step: TestStepInternal) => boolean) { + let result: TestStepInternal | undefined; + const visit = (step: TestStepInternal) => { + if (!step.endWallTime && filter(step)) + result = step; + step.steps.forEach(visit); + }; + this._steps.forEach(visit); + return result; + } + + _addStep(data: Omit): TestStepInternal { const stepId = `${data.category}@${++this._lastStepId}`; const rawStack = captureRawStack(); - if (!parentStep) - parentStep = zones.zoneData('stepZone', rawStack!) || undefined; - // For out-of-stack calls, locate the enclosing step. - let isLaxParent = false; - if (!parentStep && data.laxParent) { - const visit = (step: TestStepInternal) => { - // Do not nest chains of route.continue. - const shouldNest = step.title !== data.title; - if (!step.endWallTime && shouldNest) - parentStep = step; - step.steps.forEach(visit); - }; - this._steps.forEach(visit); - isLaxParent = !!parentStep; + let parentStep: TestStepInternal | undefined; + if (data.category === 'hook' || data.category === 'fixture') { + // Predefined steps form a fixed hierarchy - find the last non-finished one. + parentStep = this._findLastNonFinishedStep(step => step.category === 'fixture' || step.category === 'hook'); + } else { + parentStep = zones.zoneData('stepZone', rawStack!) || undefined; + if (!parentStep) { + if (data.category === 'test.step') { + // Nest test.step without a good stack in the last non-finished predefined step like a hook. + parentStep = this._findLastNonFinishedStep(step => step.category === 'fixture' || step.category === 'hook'); + } else { + // Do not nest chains of route.continue. + parentStep = this._findLastNonFinishedStep(step => step.title !== data.title); + } + } + } + if (data.forceNoParent) { + // This is used to reset step hierarchy after test timeout. + parentStep = undefined; } const filteredStack = filteredStackTrace(rawStack); @@ -281,7 +295,6 @@ export class TestInfoImpl implements TestInfo { const step: TestStepInternal = { stepId, ...data, - laxParent: isLaxParent, steps: [], complete: result => { if (step.endWallTime) @@ -414,7 +427,6 @@ export class TestInfoImpl implements TestInfo { title: `attach "${name}"`, category: 'attach', wallTime: Date.now(), - laxParent: true, }); this._attachmentsPush(attachment); this._onAttach({ diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index f080d08e0c..88a0507c24 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -358,8 +358,6 @@ export class WorkerMain extends ProcessRunner { let testFunctionParams: object | null = null; await testInfo._runAsStep({ category: 'hook', title: 'Before Hooks' }, async step => { - testInfo._beforeHooksStep = step; - // Run "beforeAll" hooks, unless already run during previous tests. for (const suite of suites) { didFailBeforeAllForSuite = suite; // Assume failure, unless reset below. @@ -416,8 +414,7 @@ export class WorkerMain extends ProcessRunner { // A timed-out test gets a full additional timeout to run after hooks. const afterHooksSlot = testInfo._didTimeout ? { timeout: this._project.project.timeout, elapsed: 0 } : undefined; - await testInfo._runAsStepWithRunnable({ category: 'hook', title: 'After Hooks', runnableType: 'afterHooks', runnableSlot: afterHooksSlot }, async step => { - testInfo._afterHooksStep = step; + await testInfo._runAsStepWithRunnable({ category: 'hook', title: 'After Hooks', runnableType: 'afterHooks', runnableSlot: afterHooksSlot, forceNoParent: true }, async step => { let firstAfterHooksError: Error | undefined; await testInfo._runWithTimeout(async () => { // Note: do not wrap all teardown steps together, because failure in any of them diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index 9587315e65..32b27b3751 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/tests/playwright-test/playwright.trace.spec.ts @@ -330,8 +330,8 @@ test('should not override trace file in afterAll', async ({ runInlineTest, serve ' fixture: request', ' apiRequest.newContext', ' apiRequestContext.get', - ' fixture: request', - ' apiRequestContext.dispose', + ' fixture: request', + ' apiRequestContext.dispose', ' fixture: browser', ]); expect(trace1.errors).toEqual([`'oh no!'`]); @@ -836,3 +836,155 @@ test('should not throw when merging traces multiple times', async ({ runInlineTe expect(result.passed).toBe(1); expect(fs.existsSync(testInfo.outputPath('test-results', 'a-foo', 'trace.zip'))).toBe(true); }); + +test('should record nested steps, even after timeout', async ({ runInlineTest }, testInfo) => { + const result = await runInlineTest({ + 'playwright.config.ts': ` + module.exports = { + use: { trace: { mode: 'on' } }, + timeout: 5000, + }; + `, + 'a.spec.ts': ` + import { test as base, expect } from '@playwright/test'; + const test = base.extend({ + fooPage: async ({ page }, use) => { + expect(1, 'fooPage setup').toBe(1); + await new Promise(f => setTimeout(f, 1)); // To avoid same-wall-time sorting issues. + await page.setContent('hello'); + await test.step('step in fooPage setup', async () => { + await page.setContent('bar'); + }); + await use(page); + expect(1, 'fooPage teardown').toBe(1); + await new Promise(f => setTimeout(f, 1)); // To avoid same-wall-time sorting issues. + await page.setContent('hi'); + await test.step('step in fooPage teardown', async () => { + await page.setContent('bar'); + }); + }, + barPage: async ({ browser }, use) => { + expect(1, 'barPage setup').toBe(1); + await new Promise(f => setTimeout(f, 1)); // To avoid same-wall-time sorting issues. + const page = await browser.newPage(); + await test.step('step in barPage setup', async () => { + await page.setContent('bar'); + }); + await use(page); + expect(1, 'barPage teardown').toBe(1); + await new Promise(f => setTimeout(f, 1)); // To avoid same-wall-time sorting issues. + await test.step('step in barPage teardown', async () => { + await page.close(); + }); + }, + }); + + test.beforeAll(async ({ barPage }) => { + expect(1, 'beforeAll start').toBe(1); + await new Promise(f => setTimeout(f, 1)); // To avoid same-wall-time sorting issues. + await barPage.setContent('hello'); + await test.step('step in beforeAll', async () => { + await barPage.setContent('bar'); + }); + }); + + test.beforeEach(async ({ fooPage }) => { + expect(1, 'beforeEach start').toBe(1); + await new Promise(f => setTimeout(f, 1)); // To avoid same-wall-time sorting issues. + await fooPage.setContent('hello'); + await test.step('step in beforeEach', async () => { + await fooPage.setContent('hi'); + // Next line times out. We make sure that after hooks steps + // form the expected step tree even when some previous steps have not finished. + await new Promise(() => {}); + }); + }); + + test('example', async ({ fooPage }) => { + }); + + test.afterEach(async ({ fooPage }) => { + expect(1, 'afterEach start').toBe(1); + await new Promise(f => setTimeout(f, 1)); // To avoid same-wall-time sorting issues. + await fooPage.setContent('hello'); + await test.step('step in afterEach', async () => { + await fooPage.setContent('bar'); + }); + }); + + test.afterAll(async ({ barPage }) => { + expect(1, 'afterAll start').toBe(1); + await new Promise(f => setTimeout(f, 1)); // To avoid same-wall-time sorting issues. + await barPage.setContent('hello'); + await test.step('step in afterAll', async () => { + await barPage.setContent('bar'); + }); + }); + `, + }, { workers: 1 }); + + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + const trace = await parseTrace(testInfo.outputPath('test-results', 'a-example', 'trace.zip')); + expect(trace.actionTree).toEqual([ + 'Before Hooks', + ' beforeAll hook', + ' fixture: browser', + ' browserType.launch', + ' fixture: barPage', + ' barPage setup', + ' browser.newPage', + ' step in barPage setup', + ' page.setContent', + ' beforeAll start', + ' page.setContent', + ' step in beforeAll', + ' page.setContent', + ' fixture: barPage', + ' barPage teardown', + ' step in barPage teardown', + ' page.close', + ' beforeEach hook', + ' fixture: context', + ' browser.newContext', + ' fixture: page', + ' browserContext.newPage', + ' fixture: fooPage', + ' fooPage setup', + ' page.setContent', + ' step in fooPage setup', + ' page.setContent', + ' beforeEach start', + ' page.setContent', + ' step in beforeEach', + ' page.setContent', + 'After Hooks', + ' afterEach hook', + ' afterEach start', + ' page.setContent', + ' step in afterEach', + ' page.setContent', + ' fixture: fooPage', + ' fooPage teardown', + ' page.setContent', + ' step in fooPage teardown', + ' page.setContent', + ' fixture: page', + ' fixture: context', + ' afterAll hook', + ' fixture: barPage', + ' barPage setup', + ' browser.newPage', + ' step in barPage setup', + ' page.setContent', + ' afterAll start', + ' page.setContent', + ' step in afterAll', + ' page.setContent', + ' fixture: barPage', + ' barPage teardown', + ' step in barPage teardown', + ' page.close', + ' fixture: browser', + ]); +});