From 28577afde4293c9514c190f578e07bcf2f68969a Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 9 Jan 2023 16:17:06 -0800 Subject: [PATCH] feat(soft expect): mark steps with failed soft expects as failed (#19973) Fixes #19673. --- packages/playwright-test/src/testInfo.ts | 58 ++++++++++++++++++-- packages/playwright-test/src/workerRunner.ts | 41 ++------------ tests/playwright-test/reporter-html.spec.ts | 15 ++++- tests/playwright-test/test-step.spec.ts | 55 +++++++++++++++++++ 4 files changed, 126 insertions(+), 43 deletions(-) diff --git a/packages/playwright-test/src/testInfo.ts b/packages/playwright-test/src/testInfo.ts index 1848f18dd1..f892d119b1 100644 --- a/packages/playwright-test/src/testInfo.ts +++ b/packages/playwright-test/src/testInfo.ts @@ -18,7 +18,7 @@ import fs from 'fs'; import path from 'path'; import { monotonicTime } from 'playwright-core/lib/utils'; import type { TestInfoError, TestInfo, TestStatus } from '../types/test'; -import type { WorkerInitParams } from './ipc'; +import type { StepBeginPayload, StepEndPayload, WorkerInitParams } from './ipc'; import type { Loader } from './loader'; import type { TestCase } from './test'; import { TimeoutManager } from './timeoutManager'; @@ -26,7 +26,8 @@ import type { Annotation, FullConfigInternal, FullProjectInternal, TestStepInter import { getContainedPath, normalizeAndSaveAttachment, sanitizeForFilePath, serializeError, trimLongString } from './util'; export class TestInfoImpl implements TestInfo { - private _addStepImpl: (data: Omit) => TestStepInternal; + private _onStepBegin: (payload: StepBeginPayload) => void; + private _onStepEnd: (payload: StepEndPayload) => void; readonly _test: TestCase; readonly _timeoutManager: TimeoutManager; readonly _startTime: number; @@ -34,6 +35,7 @@ export class TestInfoImpl implements TestInfo { private _hasHardError: boolean = false; readonly _onTestFailureImmediateCallbacks = new Map<() => Promise, string>(); // fn -> title _didTimeout = false; + _lastStepId = 0; // ------------ TestInfo fields ------------ readonly repeatEachIndex: number; @@ -85,10 +87,12 @@ export class TestInfoImpl implements TestInfo { workerParams: WorkerInitParams, test: TestCase, retry: number, - addStepImpl: (data: Omit) => TestStepInternal, + onStepBegin: (payload: StepBeginPayload) => void, + onStepEnd: (payload: StepEndPayload) => void, ) { this._test = test; - this._addStepImpl = addStepImpl; + this._onStepBegin = onStepBegin; + this._onStepEnd = onStepEnd; this._startTime = monotonicTime(); this._startWallTime = Date.now(); @@ -183,8 +187,50 @@ export class TestInfoImpl implements TestInfo { } } - _addStep(data: Omit) { - return this._addStepImpl(data); + _addStep(data: Omit): TestStepInternal { + const stepId = `${data.category}@${data.title}@${++this._lastStepId}`; + let callbackHandled = false; + const firstErrorIndex = this.errors.length; + const step: TestStepInternal = { + ...data, + complete: result => { + if (callbackHandled) + return; + callbackHandled = true; + let error: TestInfoError | undefined; + if (result.error instanceof Error) { + // Step function threw an error. + error = serializeError(result.error); + } else if (result.error) { + // Internal API step reported an error. + error = result.error; + } else { + // There was some other error (porbably soft expect) during step execution. + // Report step as failed to make it easier to spot. + error = this.errors[firstErrorIndex]; + } + const payload: StepEndPayload = { + testId: this._test.id, + refinedTitle: step.refinedTitle, + stepId, + wallTime: Date.now(), + error, + }; + this._onStepEnd(payload); + } + }; + const hasLocation = data.location && !data.location.file.includes('@playwright'); + // Sanitize location that comes from user land, it might have extra properties. + const location = data.location && hasLocation ? { file: data.location.file, line: data.location.line, column: data.location.column } : undefined; + const payload: StepBeginPayload = { + testId: this._test.id, + stepId, + ...data, + location, + wallTime: Date.now(), + }; + this._onStepBegin(payload); + return step; } _failWithError(error: TestInfoError, isHardError: boolean) { diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerRunner.ts index aab51c4bf4..e50e832ba9 100644 --- a/packages/playwright-test/src/workerRunner.ts +++ b/packages/playwright-test/src/workerRunner.ts @@ -18,11 +18,11 @@ import { colors, rimraf } from 'playwright-core/lib/utilsBundle'; import util from 'util'; import { EventEmitter } from 'events'; import { debugTest, formatLocation, relativeFilePath, serializeError } from './util'; -import type { TestBeginPayload, TestEndPayload, RunPayload, DonePayload, WorkerInitParams, StepBeginPayload, StepEndPayload, TeardownErrorsPayload, WatchTestResolvedPayload } from './ipc'; +import type { TestBeginPayload, TestEndPayload, RunPayload, DonePayload, WorkerInitParams, TeardownErrorsPayload, WatchTestResolvedPayload } from './ipc'; import { setCurrentTestInfo } from './globals'; import { Loader } from './loader'; import type { Suite, TestCase } from './test'; -import type { Annotation, FullProjectInternal, TestInfoError, TestStepInternal } from './types'; +import type { Annotation, FullProjectInternal, TestInfoError } from './types'; import { FixtureRunner } from './fixtures'; import { ManualPromise } from 'playwright-core/lib/utils/manualPromise'; import { TestInfoImpl } from './testInfo'; @@ -226,40 +226,9 @@ export class WorkerRunner extends EventEmitter { } private async _runTest(test: TestCase, retry: number, nextTest: TestCase | undefined) { - let lastStepId = 0; - const testInfo = new TestInfoImpl(this._loader, this._project, this._params, test, retry, data => { - const stepId = `${data.category}@${data.title}@${++lastStepId}`; - let callbackHandled = false; - const step: TestStepInternal = { - ...data, - complete: result => { - if (callbackHandled) - return; - callbackHandled = true; - const error = result.error instanceof Error ? serializeError(result.error) : result.error; - const payload: StepEndPayload = { - testId: test.id, - refinedTitle: step.refinedTitle, - stepId, - wallTime: Date.now(), - error, - }; - this.emit('stepEnd', payload); - } - }; - const hasLocation = data.location && !data.location.file.includes('@playwright'); - // Sanitize location that comes from user land, it might have extra properties. - const location = data.location && hasLocation ? { file: data.location.file, line: data.location.line, column: data.location.column } : undefined; - const payload: StepBeginPayload = { - testId: test.id, - stepId, - ...data, - location, - wallTime: Date.now(), - }; - this.emit('stepBegin', payload); - return step; - }); + const testInfo = new TestInfoImpl(this._loader, this._project, this._params, test, retry, + stepBeginPayload => this.emit('stepBegin', stepBeginPayload), + stepEndPayload => this.emit('stepEnd', stepEndPayload)); const processAnnotation = (annotation: Annotation) => { testInfo.annotations.push(annotation); diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts index a758aabf19..6f826fbb00 100644 --- a/tests/playwright-test/reporter-html.spec.ts +++ b/tests/playwright-test/reporter-html.spec.ts @@ -482,7 +482,7 @@ test('should warn user when viewing via file:// protocol', async ({ runInlineTes }); }); -test('should show timed out steps and hooks', async ({ runInlineTest, page, showReport }) => { +test('should show failed and timed out steps and hooks', async ({ runInlineTest, page, showReport }) => { const result = await runInlineTest({ 'playwright.config.js': ` module.exports = { timeout: 3000 }; @@ -508,6 +508,12 @@ test('should show timed out steps and hooks', async ({ runInlineTest, page, show console.log('afterAll 1'); }); test('fails', async ({ page }) => { + await test.step('outer error', async () => { + await test.step('inner error', async () => { + expect.soft(1).toBe(2); + }); + }); + await test.step('outer step', async () => { await test.step('inner step', async () => { await new Promise(() => {}); @@ -521,9 +527,16 @@ test('should show timed out steps and hooks', async ({ runInlineTest, page, show await showReport(); await page.click('text=fails'); + + await page.click('.tree-item:has-text("outer error") >> text=outer error'); + await page.click('.tree-item:has-text("outer error") >> .tree-item >> text=inner error'); + await expect(page.locator('.tree-item:has-text("outer error") svg.color-text-danger')).toHaveCount(3); + await expect(page.locator('.tree-item:has-text("expect.soft.toBe"):not(:has-text("inner"))')).toBeVisible(); + await page.click('text=outer step'); await expect(page.locator('.tree-item:has-text("outer step") svg.color-text-danger')).toHaveCount(2); await expect(page.locator('.tree-item:has-text("inner step") svg.color-text-danger')).toHaveCount(2); + await page.click('text=Before Hooks'); await expect(page.locator('.tree-item:has-text("Before Hooks") .tree-item')).toContainText([ /beforeAll hook/, diff --git a/tests/playwright-test/test-step.spec.ts b/tests/playwright-test/test-step.spec.ts index 9dc58e32c1..09fb44c0d1 100644 --- a/tests/playwright-test/test-step.spec.ts +++ b/tests/playwright-test/test-step.spec.ts @@ -35,6 +35,7 @@ class Reporter { column: step.location.column ? typeof step.location.column : 0 } : undefined, steps: step.steps.length ? step.steps.map(s => this.distillStep(s)) : undefined, + error: step.error ? '' : undefined, }; } @@ -401,3 +402,57 @@ test('should return value from step', async ({ runInlineTest }) => { expect(result.output).toContain('v1 = 10'); expect(result.output).toContain('v2 = 20'); }); + +test('should mark step as failed when soft expect fails', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'reporter.ts': stepHierarchyReporter, + 'playwright.config.ts': ` + module.exports = { + reporter: './reporter', + }; + `, + 'a.test.ts': ` + const { test } = pwt; + test('pass', async ({}) => { + await test.step('outer', async () => { + await test.step('inner', async () => { + expect.soft(1).toBe(2); + }); + }); + await test.step('passing', () => {}); + }); + ` + }, { reporter: '', workers: 1 }); + + expect(result.exitCode).toBe(1); + const objects = result.output.split('\n').filter(line => line.startsWith('%% ')).map(line => line.substring(3).trim()).filter(Boolean).map(line => JSON.parse(line)); + expect(objects).toEqual([ + { title: 'Before Hooks', category: 'hook' }, + { + title: 'outer', + category: 'test.step', + error: '', + steps: [{ + title: 'inner', + category: 'test.step', + error: '', + steps: [ + { + title: 'expect.soft.toBe', + category: 'expect', + location: { file: 'a.test.ts', line: 'number', column: 'number' }, + error: '' + } + ], + location: { file: 'a.test.ts', line: 'number', column: 'number' } + }], + location: { file: 'a.test.ts', line: 'number', column: 'number' } + }, + { + title: 'passing', + category: 'test.step', + location: { file: 'a.test.ts', line: 'number', column: 'number' } + }, + { title: 'After Hooks', category: 'hook' } + ]); +});