fix(expect): account for timeout during the first locator handler check (#32095)

Also considered an alternative to not perform the locator handler check
during one-shot, but that would be somewhat against the promise of the
locator handler that is supposed to run **before** every expect check.

Fixes #32089.

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

@@ -1392,31 +1392,68 @@ export class Frame extends SdkObject {
   }
 
   private async _expectImpl(metadata: CallMetadata, selector: string, options: FrameExpectParams): Promise<{ matches: boolean, received?: any, log?: string[], timedOut?: boolean }> {
+    const lastIntermediateResult: { received?: any, isSet: boolean } = { isSet: false };
+    try {
       let timeout = this._page._timeoutSettings.timeout(options);
       const start = timeout > 0 ? monotonicTime() : 0;
-    const lastIntermediateResult: { received?: any, isSet: boolean } = { isSet: false };
+
-    const resultOneShot = await this._expectInternal(metadata, selector, options, true, timeout, lastIntermediateResult);
+      // Step 1: perform locator handlers checkpoint with a specified timeout.
+      await (new ProgressController(metadata, this)).run(async progress => {
+        progress.log(`${metadata.apiName}${timeout ? ` with timeout ${timeout}ms` : ''}`);
+        progress.log(`waiting for ${this._asLocator(selector)}`);
+        await this._page.performLocatorHandlersCheckpoint(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.
+      try {
+        const resultOneShot = await (new ProgressController(metadata, this)).run(async progress => {
+          return await this._expectInternal(progress, selector, options, lastIntermediateResult);
+        });
         if (resultOneShot.matches !== options.isNot)
           return resultOneShot;
+      } catch (e) {
+        if (js.isJavaScriptErrorInEvaluate(e) || isInvalidSelectorError(e))
+          throw e;
+        // Ignore any other errors from one-shot, we'll handle them during retries.
+      }
       if (timeout > 0) {
         const elapsed = monotonicTime() - start;
         timeout -= elapsed;
       }
       if (timeout < 0)
         return { matches: options.isNot, log: metadata.log, timedOut: true, received: lastIntermediateResult.received };
-    return await this._expectInternal(metadata, selector, options, false, timeout, lastIntermediateResult);
-  }
 
-  private async _expectInternal(metadata: CallMetadata, selector: string, options: FrameExpectParams, oneShot: boolean, timeout: number, lastIntermediateResult: { received?: any, isSet: boolean }): Promise<{ matches: boolean, received?: any, log?: string[], timedOut?: boolean }> {
+      // Step 3: auto-retry expect with increasing timeouts. Bounded by the total remaining time.
-    const controller = new ProgressController(metadata, this);
+      return await (new ProgressController(metadata, this)).run(async progress => {
-    return controller.run(async progress => {
-      if (oneShot) {
-        progress.log(`${metadata.apiName}${timeout ? ` with timeout ${timeout}ms` : ''}`);
-        progress.log(`waiting for ${this._asLocator(selector)}`);
-      }
         return await this.retryWithProgressAndTimeouts(progress, [100, 250, 500, 1000], async continuePolling => {
           await this._page.performLocatorHandlersCheckpoint(progress);
+          const { matches, received } = await this._expectInternal(progress, selector, options, lastIntermediateResult);
+          if (matches === options.isNot) {
+            // Keep waiting in these cases:
+            // expect(locator).conditionThatDoesNotMatch
+            // expect(locator).not.conditionThatDoesMatch
+            return continuePolling;
+          }
+          return { matches, received };
+        });
+      }, timeout);
+    } catch (e) {
+      // Q: Why not throw upon isSessionClosedError(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))
+        throw e;
+      const result: { matches: boolean, received?: any, log?: string[], timedOut?: boolean } = { matches: options.isNot, log: metadata.log };
+      if (lastIntermediateResult.isSet)
+        result.received = lastIntermediateResult.received;
+      if (e instanceof TimeoutError)
+        result.timedOut = true;
+      return result;
+    }
+  }
 
+  private async _expectInternal(progress: Progress, selector: string, options: FrameExpectParams, lastIntermediateResult: { received?: any, isSet: boolean }) {
     const selectorInFrame = await this.selectors.resolveFrameForSelector(selector, { strict: true });
     progress.throwIfAborted();
 
@@ -1439,7 +1476,7 @@ export class Frame extends SdkObject {
       if (callId)
         injected.markTargetElements(new Set(elements), callId);
       return { log, ...await injected.expect(elements[0], options, elements) };
-        }, { info, options, callId: metadata.id });
+    }, { info, options, callId: progress.metadata.id });
 
     if (log)
       progress.log(log);
@@ -1450,26 +1487,7 @@ export class Frame extends SdkObject {
       if (!missingReceived && !Array.isArray(received))
         progress.log(`  unexpected value "${renderUnexpectedValue(options.expression, received)}"`);
     }
-        if (!oneShot && matches === options.isNot) {
-          // Keep waiting in these cases:
-          // expect(locator).conditionThatDoesNotMatch
-          // expect(locator).not.conditionThatDoesMatch
-          return continuePolling;
-        }
     return { matches, received };
-      });
-    }, oneShot ? 0 : timeout).catch(e => {
-      // Q: Why not throw upon isSessionClosedError(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))
-        throw e;
-      const result: { matches: boolean, received?: any, log?: string[], timedOut?: boolean } = { matches: options.isNot, log: metadata.log };
-      if (lastIntermediateResult.isSet)
-        result.received = lastIntermediateResult.received;
-      if (e instanceof TimeoutError)
-        result.timedOut = true;
-      return result;
-    });
   }
 
   async _waitForFunctionExpression<R>(metadata: CallMetadata, expression: string, isFunction: boolean | undefined, arg: any, options: types.WaitForFunctionOptions, world: types.World = 'main'): Promise<js.SmartHandle<R>> {

tests/page/expect-timeout.spec.ts

@@ -55,3 +55,29 @@ test('should have timeout error name', async ({ page }) => {
   const error = await page.waitForSelector('#not-found', { timeout: 1 }).catch(e => e);
   expect(error.name).toBe('TimeoutError');
 });
+
+test('should not throw when navigating during one-shot check', async ({ page, server }) => {
+  await page.setContent(`<div>hello</div>`);
+  const promise = expect(page.locator('div')).toHaveText('bye');
+  await page.goto(server.EMPTY_PAGE);
+  await page.setContent(`<div>bye</div>`);
+  await promise;
+});
+
+test('should not throw when navigating during first locator handler check', async ({ page, server }) => {
+  await page.addLocatorHandler(page.locator('span'), async locator => {});
+  await page.setContent(`<div>hello</div>`);
+  const promise = expect(page.locator('div')).toHaveText('bye');
+  await page.goto(server.EMPTY_PAGE);
+  await page.setContent(`<div>bye</div>`);
+  await promise;
+});
+
+test('should timeout during first locator handler check', async ({ page, server }) => {
+  await page.addLocatorHandler(page.locator('div'), async locator => {});
+  await page.setContent(`<div>hello</div><span>bye</span>`);
+  const error = await expect(page.locator('span')).toHaveText('bye', { timeout: 3000 }).catch(e => e);
+  expect(error.message).toContain('Timed out 3000ms waiting for');
+  expect(error.message).toContain(`locator handler has finished, waiting for locator('div') to be hidden`);
+  expect(error.message).toContain(`locator resolved to visible <div>hello</div>`);
+});