From 004a35fff7943a7a903b83034efe78fc18daa857 Mon Sep 17 00:00:00 2001
From: Yury Semikhatsky <yurys@chromium.org>
Date: Tue, 12 Nov 2024 15:35:45 -0800
Subject: [PATCH] address comments

---
 packages/playwright/src/common/testType.ts | 17 ++++------
 tests/playwright-test/test-step.spec.ts    | 39 ++++++++++++++++++++--
 2 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/packages/playwright/src/common/testType.ts b/packages/playwright/src/common/testType.ts
index 119095bfd1..afdbe5f448 100644
--- a/packages/playwright/src/common/testType.ts
+++ b/packages/playwright/src/common/testType.ts
@@ -21,7 +21,8 @@ import { wrapFunctionWithLocation } from '../transform/transform';
 import type { FixturesWithLocation } from './config';
 import type { Fixtures, TestType, TestDetails } from '../../types/test';
 import type { Location } from '../../types/testReporter';
-import { getPackageManagerExecCommand, ManualPromise, zones } from 'playwright-core/lib/utils';
+import { getPackageManagerExecCommand, monotonicTime, raceAgainstDeadline, zones } from 'playwright-core/lib/utils';
+import { errors } from 'playwright-core';
 
 const testTypeSymbol = Symbol('testType');
 
@@ -262,19 +263,15 @@ export class TestTypeImpl {
       throw new Error(`test.step() can only be called from a test`);
     const step = testInfo._addStep({ category: 'test.step', title, location: options.location, box: options.box });
     return await zones.run('stepZone', step, async () => {
-      const timeoutPromise = new ManualPromise<T>();
-      const timer = options.timeout
-        ? setTimeout(() => timeoutPromise.reject(new StepTimeoutError(`Step timeout ${options.timeout}ms exceeded.`)), options.timeout)
-        : undefined;
       try {
-        const result = await Promise.race([body(), timeoutPromise]);
+        const result = await raceAgainstDeadline(body, options.timeout ? monotonicTime() + options.timeout : 0);
+        if (result.timedOut)
+          throw new errors.TimeoutError(`Step timeout ${options.timeout}ms exceeded.`);
         step.complete({});
-        return result;
+        return result.result;
       } catch (error) {
         step.complete({ error });
         throw error;
-      } finally {
-        clearTimeout(timer);
       }
     });
   }
@@ -308,8 +305,6 @@ function validateTestDetails(details: TestDetails) {
   return { annotations, tags };
 }
 
-class StepTimeoutError extends Error {}
-
 export const rootTestType = new TestTypeImpl([]);
 
 export function mergeTests(...tests: TestType<any, any>[]) {
diff --git a/tests/playwright-test/test-step.spec.ts b/tests/playwright-test/test-step.spec.ts
index bd0c19693d..f0e3099540 100644
--- a/tests/playwright-test/test-step.spec.ts
+++ b/tests/playwright-test/test-step.spec.ts
@@ -398,11 +398,46 @@ test('step timeout option', async ({ runInlineTest }) => {
     `
   }, { reporter: '', workers: 1 });
   expect(result.exitCode).toBe(1);
-  expect(result.passed).toBe(0);
-  console.log(result.output);
+  expect(result.failed).toBe(1);
   expect(result.output).toContain('Error: Step timeout 100ms exceeded.');
 });
 
+test('step timeout longer than test timeout', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      import { defineConfig } from '@playwright/test';
+      export default defineConfig({ timeout: 900 });
+    `,
+    'a.test.ts': `
+      import { test, expect } from '@playwright/test';
+      test('step with timeout', async () => {
+        await test.step('my step', async () => {
+          await new Promise(() => {});
+        }, { timeout: 5000 });
+      });
+    `
+  }, { reporter: '', workers: 1 });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.output).toContain('Test timeout of 900ms exceeded.');
+});
+
+test('step timeout is errors.TimeoutError', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      import { test, expect, errors } from '@playwright/test';
+      test('step timeout error type', async () => {
+        const e = await test.step('my step', async () => {
+          await new Promise(() => {});
+        }, { timeout: 100 }).catch(e => e);
+        expect(e).toBeInstanceOf(errors.TimeoutError);
+      });
+    `
+  }, { reporter: '', workers: 1 });
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(1);
+});
+
 test('should mark step as failed when soft expect fails', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'reporter.ts': stepIndentReporter,