chore: use a single Progress instance for expect (#36407)

packages/playwright-core/src/server/frames.ts

@@ -27,7 +27,7 @@ import * as network from './network';
 import { Page } from './page';
 import { isAbortError, ProgressController } from './progress';
 import * as types from './types';
-import { LongStandingScope, asLocator, assert, constructURLBasedOnBaseURL, makeWaitForNextTask, monotonicTime, renderTitleForCall } from '../utils';
+import { LongStandingScope, asLocator, assert, constructURLBasedOnBaseURL, makeWaitForNextTask, renderTitleForCall } from '../utils';
 import { isSessionClosedError } from './protocolError';
 import { debugLogger } from './utils/debugLogger';
 import { eventsHelper } from './utils/eventsHelper';
@@ -1417,26 +1417,21 @@ export class Frame extends SdkObject {
   }
 
   private async _expectImpl(metadata: CallMetadata, selector: string | undefined, options: FrameExpectParams): Promise<{ matches: boolean, received?: any, log?: string[], timedOut?: boolean }> {
+    const controller = new ProgressController(metadata, this);
     const lastIntermediateResult: { received?: any, isSet: boolean } = { isSet: false };
-    try {
+    return await controller.run(async progress => {
-      let timeout = options.timeout;
-      const start = timeout > 0 ? monotonicTime() : 0;
-
       // Step 1: perform locator handlers checkpoint with a specified timeout.
-      await (new ProgressController(metadata, this)).run(async progress => {
+      progress.log(`${renderTitleForCall(metadata)}${options.timeout ? ` with timeout ${options.timeout}ms` : ''}`);
-        progress.log(`${renderTitleForCall(metadata)}${timeout ? ` with timeout ${timeout}ms` : ''}`);
+      if (selector)
-        if (selector)
+        progress.log(`waiting for ${this._asLocator(selector)}`);
-          progress.log(`waiting for ${this._asLocator(selector)}`);
+      await this._page.performActionPreChecks(progress);
-        await this._page.performActionPreChecks(progress);
-      }, timeout);
 
       // Step 2: perform one-shot expect check without a timeout.
       // Supports the case of `expect(locator).toBeVisible({ timeout: 1 })`
       // that should succeed when the locator is already visible.
+      progress.legacyDisableTimeout();
       try {
-        const resultOneShot = await (new ProgressController(metadata, this)).run(async progress => {
+        const resultOneShot = await this._expectInternal(progress, selector, options, lastIntermediateResult, true);
-          return await this._expectInternal(progress, selector, options, lastIntermediateResult);
-        });
         if (resultOneShot.matches !== options.isNot)
           return resultOneShot;
       } catch (e) {
@@ -1444,28 +1439,21 @@ export class Frame extends SdkObject {
           throw e;
         // Ignore any other errors from one-shot, we'll handle them during retries.
       }
-      if (timeout > 0) {
+      progress.legacyEnableTimeout();
-        const elapsed = monotonicTime() - start;
-        timeout -= elapsed;
-      }
-      if (timeout < 0)
-        return { matches: options.isNot, log: compressCallLog(metadata.log), timedOut: true, received: lastIntermediateResult.received };
 
       // Step 3: auto-retry expect with increasing timeouts. Bounded by the total remaining time.
-      return await (new ProgressController(metadata, this)).run(async progress => {
+      return await this.retryWithProgressAndTimeouts(progress, [100, 250, 500, 1000], async continuePolling => {
-        return await this.retryWithProgressAndTimeouts(progress, [100, 250, 500, 1000], async continuePolling => {
+        await this._page.performActionPreChecks(progress);
-          await this._page.performActionPreChecks(progress);
+        const { matches, received } = await this._expectInternal(progress, selector, options, lastIntermediateResult, false);
-          const { matches, received } = await this._expectInternal(progress, selector, options, lastIntermediateResult);
+        if (matches === options.isNot) {
-          if (matches === options.isNot) {
+          // Keep waiting in these cases:
-            // Keep waiting in these cases:
+          // expect(locator).conditionThatDoesNotMatch
-            // expect(locator).conditionThatDoesNotMatch
+          // expect(locator).not.conditionThatDoesMatch
-            // expect(locator).not.conditionThatDoesMatch
+          return continuePolling;
-            return continuePolling;
+        }
-          }
+        return { matches, received };
-          return { matches, received };
+      });
-        });
+    }, options.timeout).catch(e => {
-      }, timeout);
-    } catch (e) {
       // Q: Why not throw upon isNonRetriableError(e) as in other places?
       // A: We want user to receive a friendly message containing the last intermediate result.
       if (js.isJavaScriptErrorInEvaluate(e) || isInvalidSelectorError(e))
@@ -1476,18 +1464,20 @@ export class Frame extends SdkObject {
       if (e instanceof TimeoutError)
         result.timedOut = true;
       return result;
-    }
+    });
   }
 
-  private async _expectInternal(progress: Progress, selector: string | undefined, options: FrameExpectParams, lastIntermediateResult: { received?: any, isSet: boolean }) {
+  private async _expectInternal(progress: Progress, selector: string | undefined, options: FrameExpectParams, lastIntermediateResult: { received?: any, isSet: boolean }, noAbort: boolean) {
-    const selectorInFrame = selector ? await progress.race(this.selectors.resolveFrameForSelector(selector, { strict: true })) : undefined;
+    // The first expect check, a.k.a. one-shot, always finishes - even when progress is aborted.
+    const race = <T>(p: Promise<T>) => noAbort ? p : progress.race(p);
+    const selectorInFrame = selector ? await race(this.selectors.resolveFrameForSelector(selector, { strict: true })) : undefined;
 
     const { frame, info } = selectorInFrame || { frame: this, info: undefined };
     const world = options.expression === 'to.have.property' ? 'main' : (info?.world ?? 'utility');
-    const context = await progress.race(frame._context(world));
+    const context = await race(frame._context(world));
-    const injected = await progress.race(context.injectedScript());
+    const injected = await race(context.injectedScript());
 
-    const { log, matches, received, missingReceived } = await progress.race(injected.evaluate(async (injected, { info, options, callId }) => {
+    const { log, matches, received, missingReceived } = await race(injected.evaluate(async (injected, { info, options, callId }) => {
       const elements = info ? injected.querySelectorAll(info.parsed, document) : [];
       if (callId)
         injected.markTargetElements(new Set(elements), callId);

packages/playwright-core/src/server/progress.ts

@@ -15,7 +15,7 @@
  */
 
 import { TimeoutError } from './errors';
-import { assert } from '../utils';
+import { assert, monotonicTime } from '../utils';
 import { ManualPromise } from '../utils/isomorphic/manualPromise';
 
 import type { CallMetadata, Instrumentation, SdkObject } from './instrumentation';
@@ -43,6 +43,10 @@ export interface Progress {
   raceWithCleanup<T>(promise: Promise<T>, cleanup: (result: T) => any): Promise<T>;
   wait(timeout: number): Promise<void>;
   metadata: CallMetadata;
+
+  // Legacy lenient mode api only. To be removed.
+  legacyDisableTimeout(): void;
+  legacyEnableTimeout(): void;
 }
 
 export class ProgressController {
@@ -93,6 +97,26 @@ export class ProgressController {
     this._state = 'running';
     this.sdkObject.attribution.context?._activeProgressControllers.add(this);
 
+    const deadline = timeout ? Math.min(monotonicTime() + timeout, 2147483647) : 0; // 2^31-1 safe setTimeout in Node.
+    const timeoutError = new TimeoutError(`Timeout ${timeout}ms exceeded.`);
+
+    let timer: NodeJS.Timeout | undefined;
+    const startTimer = () => {
+      if (!deadline)
+        return;
+      const onTimeout = () => {
+        if (this._state === 'running') {
+          this._state = { error: timeoutError };
+          this._forceAbortPromise.reject(timeoutError);
+        }
+      };
+      const remaining = deadline - monotonicTime();
+      if (remaining <= 0)
+        onTimeout();
+      else
+        timer = setTimeout(onTimeout, remaining);
+    };
+
     const progress: Progress = {
       log: message => {
         if (this._state === 'running')
@@ -128,18 +152,19 @@ export class ProgressController {
         const promise = new Promise<void>(f => timer = setTimeout(f, timeout));
         return progress.race(promise).finally(() => clearTimeout(timer));
       },
+      legacyDisableTimeout: () => {
+        if (this._strictMode)
+          return;
+        clearTimeout(timer);
+      },
+      legacyEnableTimeout: () => {
+        if (this._strictMode)
+          return;
+        startTimer();
+      },
     };
 
-    let timer: NodeJS.Timeout | undefined;
+    startTimer();
-    if (timeout) {
-      const timeoutError = new TimeoutError(`Timeout ${timeout}ms exceeded.`);
-      timer = setTimeout(() => {
-        if (this._state === 'running') {
-          this._state = { error: timeoutError };
-          this._forceAbortPromise.reject(timeoutError);
-        }
-      }, Math.min(timeout, 2147483647)); // 2^31-1 safe setTimeout in Node.
-    }
 
     try {
       const promise = task(progress);