chore: step timeout improvements (#34386)

microsoft · Jan 18, 2025 · 372d419 · 372d419
1 parent 9970446
commit 372d419
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 4 deletions.
diff --git a/docs/src/test-api/class-test.md b/docs/src/test-api/class-test.md
@@ -1822,7 +1822,7 @@ Maximum time in milliseconds for the step to finish. Defaults to `0` (no timeout
 * since: v1.50
 - `timeout` <[float]>
 
-Maximum time in milliseconds for the step to finish. Defaults to `0` (no timeout).
+The maximum time, in milliseconds, allowed for the step to complete. If the step does not complete within the specified timeout, the [`method: Test.step`] method will throw a [TimeoutError]. Defaults to `0` (no timeout).
 
 ## method: Test.use
 * since: v1.10

diff --git a/packages/playwright/src/common/testType.ts b/packages/playwright/src/common/testType.ts
@@ -270,9 +270,20 @@ export class TestTypeImpl {
     const step = testInfo._addStep({ category: 'test.step', title, location: options.location, box: options.box });
     return await zones.run('stepZone', step, async () => {
       try {
-        const result = await raceAgainstDeadline(async () => body(), options.timeout ? monotonicTime() + options.timeout : 0);
+        let result: Awaited<ReturnType<typeof raceAgainstDeadline<T>>> | undefined = undefined;
+        result = await raceAgainstDeadline(async () => {
+          try {
+            return await body();
+          } catch (e) {
+            // If the step timed out, the test fixtures will tear down, which in turn
+            // will abort unfinished actions in the step body. Record such errors here.
+            if (result?.timedOut)
+              testInfo._failWithError(e);
+            throw e;
+          }
+        }, options.timeout ? monotonicTime() + options.timeout : 0);
         if (result.timedOut)
-          throw new errors.TimeoutError(`Step timeout ${options.timeout}ms exceeded.`);
+          throw new errors.TimeoutError(`Step timeout of ${options.timeout}ms exceeded.`);
         step.complete({});
         return result.result;
       } catch (error) {

diff --git a/tests/playwright-test/test-step.spec.ts b/tests/playwright-test/test-step.spec.ts
@@ -399,7 +399,7 @@ test('step timeout option', async ({ runInlineTest }) => {
   }, { reporter: '', workers: 1 });
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
-  expect(result.output).toContain('Error: Step timeout 100ms exceeded.');
+  expect(result.output).toContain('Error: Step timeout of 100ms exceeded.');
 });
 
 test('step timeout longer than test timeout', async ({ runInlineTest }) => {
@@ -422,6 +422,27 @@ test('step timeout longer than test timeout', async ({ runInlineTest }) => {
   expect(result.output).toContain('Test timeout of 900ms exceeded.');
 });
 
+test('step timeout includes interrupted action errors', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      import { test, expect } from '@playwright/test';
+      test('step with timeout', async ({ page }) => {
+        await test.step('my step', async () => {
+          await page.waitForTimeout(100_000);
+        }, { timeout: 1000 });
+      });
+    `
+  }, { reporter: '', workers: 1 });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  // Should include 2 errors, one for the step timeout and one for the aborted action.
+  expect.soft(result.output).toContain('TimeoutError: Step timeout of 1000ms exceeded.');
+  expect.soft(result.output).toContain(`> 4 |         await test.step('my step', async () => {`);
+  expect.soft(result.output).toContain('Error: page.waitForTimeout: Test ended.');
+  expect.soft(result.output.split('Error: page.waitForTimeout: Test ended.').length).toBe(2);
+  expect.soft(result.output).toContain('> 5 |           await page.waitForTimeout(100_000);');
+});
+
 test('step timeout is errors.TimeoutError', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.ts': `