fix(steps): only propagate soft errors up the hierarchy (#24054)

Fixes #23979
microsoft · Jul 5, 2023 · 566b277 · 566b277
1 parent ea3a29e
commit 566b277
Show file tree

Hide file tree

Showing 4 changed files with 179 additions and 7 deletions.
diff --git a/packages/playwright-test/src/matchers/expect.ts b/packages/playwright-test/src/matchers/expect.ts
@@ -255,7 +255,8 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler<any> {
         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
@@ -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
@@ -39,7 +39,7 @@ export interface TestStepInternal {
   apiName?: string;
   params?: Record<string, any>;
   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
@@ -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: '<error>',
+      location: { column: 'number', file: 'a.test.ts', line: 'number' },
+      steps: [
+        {
+          category: 'test.step',
+          title: 'first inner',
+          error: '<error>',
+          location: { column: 'number', file: 'a.test.ts', line: 'number' },
+          steps: [
+            {
+              category: 'expect',
+              title: 'expect.soft.toBe',
+              error: '<error>',
+              location: { column: 'number', file: 'a.test.ts', line: 'number' },
+            },
+          ],
+        },
+      ],
+    },
+    {
+      category: 'test.step',
+      title: 'second outer',
+      error: '<error>',
+      location: { column: 'number', file: 'a.test.ts', line: 'number' },
+      steps: [
+        {
+          category: 'test.step',
+          title: 'second inner',
+          error: '<error>',
+          location: { column: 'number', file: 'a.test.ts', line: 'number' },
+          steps: [
+            {
+              category: 'expect',
+              title: 'expect.toBe',
+              error: '<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: '<error>',
+              location: { column: 'number', file: 'a.test.ts', line: 'number' },
+            },
+          ],
+        },
+      ],
+    },
+    {
+      category: 'test.step',
+      title: 'second outer',
+      error: '<error>',
+      location: { column: 'number', file: 'a.test.ts', line: 'number' },
+      steps: [
+        {
+          category: 'test.step',
+          title: 'second inner',
+          error: '<error>',
+          location: { column: 'number', file: 'a.test.ts', line: 'number' },
+          steps: [
+            {
+              category: 'expect',
+              title: 'expect.toBe',
+              error: '<error>',
+              location: { column: 'number', file: 'a.test.ts', line: 'number' },
+            },
+          ],
+        },
+      ],
+    },
+    {
+      category: 'hook',
+      title: 'After Hooks',
+    },
+  ]);
+});