From 566b277ce879c7eaa38e4cc50aa086436bde8a89 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 5 Jul 2023 15:30:53 -0700 Subject: [PATCH] fix(steps): only propagate soft errors up the hierarchy (#24054) Fixes https://github.com/microsoft/playwright/issues/23979 --- .../playwright-test/src/matchers/expect.ts | 3 +- .../playwright-test/src/matchers/matchers.ts | 1 - .../playwright-test/src/worker/testInfo.ts | 21 ++- tests/playwright-test/test-step.spec.ts | 161 ++++++++++++++++++ 4 files changed, 179 insertions(+), 7 deletions(-) diff --git a/packages/playwright-test/src/matchers/expect.ts b/packages/playwright-test/src/matchers/expect.ts index 8f4f034e26753..cdf96f3e7b53d 100644 --- a/packages/playwright-test/src/matchers/expect.ts +++ b/packages/playwright-test/src/matchers/expect.ts @@ -255,7 +255,8 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler { category: 'expect', title: trimLongString(customMessage || defaultTitle, 1024), params: args[0] ? { expected: args[0] } : undefined, - wallTime + wallTime, + infectParentStepsWithError: this._info.isSoft, }) : undefined; const reportStepError = (jestError: Error) => { diff --git a/packages/playwright-test/src/matchers/matchers.ts b/packages/playwright-test/src/matchers/matchers.ts index a673f1a0c74c1..fa9678cc3d3f6 100644 --- a/packages/playwright-test/src/matchers/matchers.ts +++ b/packages/playwright-test/src/matchers/matchers.ts @@ -352,7 +352,6 @@ export async function toPass( title: 'expect.toPass', category: 'expect', location: stackFrames[0], - insulateChildErrors: true, }, callback); }; diff --git a/packages/playwright-test/src/worker/testInfo.ts b/packages/playwright-test/src/worker/testInfo.ts index 39a5ba3906b0d..c4b04bd5b92cb 100644 --- a/packages/playwright-test/src/worker/testInfo.ts +++ b/packages/playwright-test/src/worker/testInfo.ts @@ -39,7 +39,7 @@ export interface TestStepInternal { apiName?: string; params?: Record; error?: TestInfoError; - insulateChildErrors?: boolean; + infectParentStepsWithError?: boolean; } export class TestInfoImpl implements TestInfo { @@ -265,12 +265,23 @@ export class TestInfoImpl implements TestInfo { } else if (result.error) { // Internal API step reported an error. error = result.error; - } else if (!data.insulateChildErrors) { - // One of the child steps failed (probably soft expect). - // Report this step as failed to make it easier to spot. - error = step.steps.map(s => s.error).find(e => !!e); } step.error = error; + + if (!error) { + // Soft errors inside try/catch will make the test fail. + // In order to locate the failing step, we are marking all the parent + // steps as failing unconditionally. + for (const childStep of step.steps) { + if (childStep.error && childStep.infectParentStepsWithError) { + step.error = childStep.error; + step.infectParentStepsWithError = true; + break; + } + } + error = step.error; + } + const payload: StepEndPayload = { testId: this._test.id, stepId, diff --git a/tests/playwright-test/test-step.spec.ts b/tests/playwright-test/test-step.spec.ts index 000cd65aa6efd..ff3ca04f42594 100644 --- a/tests/playwright-test/test-step.spec.ts +++ b/tests/playwright-test/test-step.spec.ts @@ -1128,3 +1128,164 @@ test('should show final toPass error', async ({ runInlineTest }) => { }, ]); }); + +test('should propagate nested soft errors', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'reporter.ts': stepHierarchyReporter, + 'playwright.config.ts': `module.exports = { reporter: './reporter', };`, + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('fail', async () => { + await test.step('first outer', async () => { + await test.step('first inner', async () => { + expect.soft(1).toBe(2); + }); + }); + + await test.step('second outer', async () => { + await test.step('second inner', async () => { + expect(1).toBe(2); + }); + }); + }); + ` + }, { reporter: '' }); + + expect(result.exitCode).toBe(1); + const objects = result.outputLines.map(line => JSON.parse(line)); + expect(objects).toEqual([ + { + category: 'hook', + title: 'Before Hooks', + }, + { + category: 'test.step', + title: 'first outer', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + steps: [ + { + category: 'test.step', + title: 'first inner', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + steps: [ + { + category: 'expect', + title: 'expect.soft.toBe', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + }, + ], + }, + ], + }, + { + category: 'test.step', + title: 'second outer', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + steps: [ + { + category: 'test.step', + title: 'second inner', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + steps: [ + { + category: 'expect', + title: 'expect.toBe', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + }, + ], + }, + ], + }, + { + category: 'hook', + title: 'After Hooks', + }, + ]); +}); + +test('should not propagate nested hard errors', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'reporter.ts': stepHierarchyReporter, + 'playwright.config.ts': `module.exports = { reporter: './reporter', };`, + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('fail', async () => { + await test.step('first outer', async () => { + await test.step('first inner', async () => { + try { + expect(1).toBe(2); + } catch (e) { + } + }); + }); + + await test.step('second outer', async () => { + await test.step('second inner', async () => { + expect(1).toBe(2); + }); + }); + }); + ` + }, { reporter: '' }); + + expect(result.exitCode).toBe(1); + const objects = result.outputLines.map(line => JSON.parse(line)); + expect(objects).toEqual([ + { + category: 'hook', + title: 'Before Hooks', + }, + { + category: 'test.step', + title: 'first outer', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + steps: [ + { + category: 'test.step', + title: 'first inner', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + steps: [ + { + category: 'expect', + title: 'expect.toBe', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + }, + ], + }, + ], + }, + { + category: 'test.step', + title: 'second outer', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + steps: [ + { + category: 'test.step', + title: 'second inner', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + steps: [ + { + category: 'expect', + title: 'expect.toBe', + error: '', + location: { column: 'number', file: 'a.test.ts', line: 'number' }, + }, + ], + }, + ], + }, + { + category: 'hook', + title: 'After Hooks', + }, + ]); +});