chore: migrate wait tasks to Progress (#2422)

Author: dgozman

src/debug/stackTrace.ts

@@ -18,6 +18,7 @@ import * as path from 'path';
 
 // NOTE: update this to point to playwright/lib when moving this file.
 const PLAYWRIGHT_LIB_PATH = path.normalize(path.join(__dirname, '..'));
+const APICOVERAGE = path.normalize(path.join(__dirname, '..', '..', 'test', 'apicoverage'));
 
 type ParsedStackFrame = { filePath: string, functionName: string };
 
@@ -67,7 +68,7 @@ export function getCurrentApiCall(prefix = PLAYWRIGHT_LIB_PATH): string {
   let apiName: string = '';
   for (const frame of stackFrames) {
     const parsed = parseStackFrame(frame);
-    if (!parsed || (!parsed.filePath.startsWith(prefix) && parsed.filePath !== __filename))
+    if (!parsed || (!parsed.filePath.startsWith(prefix) && !parsed.filePath.startsWith(APICOVERAGE) && parsed.filePath !== __filename))
       break;
     apiName = parsed.functionName;
   }

src/frames.ts

@@ -30,6 +30,7 @@ import * as types from './types';
 import { waitForTimeoutWasUsed } from './hints';
 import { BrowserContext } from './browserContext';
 import { rewriteErrorMessage } from './debug/stackTrace';
+import { Progress } from './progress';
 
 type ContextType = 'main' | 'utility';
 type ContextData = {
@@ -441,37 +442,40 @@ export class Frame {
     return selectors._query(this, selector);
   }
 
-  async waitForSelector(selector: string, options?: types.WaitForElementOptions): Promise<dom.ElementHandle<Element> | null> {
-    if (options && (options as any).visibility)
+  async waitForSelector(selector: string, options: types.WaitForElementOptions = {}): Promise<dom.ElementHandle<Element> | null> {
+    if ((options as any).visibility)
       throw new Error('options.visibility is not supported, did you mean options.state?');
-    if (options && (options as any).waitFor && (options as any).waitFor !== 'visible')
+    if ((options as any).waitFor && (options as any).waitFor !== 'visible')
       throw new Error('options.waitFor is not supported, did you mean options.state?');
-    const { state = 'visible' } = (options || {});
+    const { state = 'visible' } = options;
     if (!['attached', 'detached', 'visible', 'hidden'].includes(state))
       throw new Error(`Unsupported waitFor option "${state}"`);
-
-    const deadline = this._page._timeoutSettings.computeDeadline(options);
     const { world, task } = selectors._waitForSelectorTask(selector, state);
-    const result = await this._scheduleRerunnableTask(task, world, deadline, `selector "${selector}"${state === 'attached' ? '' : ' to be ' + state}`);
-    if (!result.asElement()) {
-      result.dispose();
-      return null;
-    }
-    const handle = result.asElement() as dom.ElementHandle<Element>;
-    const mainContext = await this._mainContext();
-    if (handle && handle._context !== mainContext) {
-      const adopted = await this._page._delegate.adoptElementHandle(handle, mainContext);
-      handle.dispose();
-      return adopted;
-    }
-    return handle;
+    return Progress.runCancelableTask(async progress => {
+      progress.log(dom.inputLog, `Waiting for selector "${selector}"${state === 'attached' ? '' : ' to be ' + state}...`);
+      const result = await this._scheduleRerunnableTask(progress, world, task);
+      if (!result.asElement()) {
+        result.dispose();
+        return null;
+      }
+      const handle = result.asElement() as dom.ElementHandle<Element>;
+      const mainContext = await this._mainContext();
+      if (handle && handle._context !== mainContext) {
+        const adopted = await this._page._delegate.adoptElementHandle(handle, mainContext);
+        handle.dispose();
+        return adopted;
+      }
+      return handle;
+    }, options, this._page, this._page._timeoutSettings);
   }
 
   async dispatchEvent(selector: string, type: string, eventInit?: Object, options?: types.TimeoutOptions): Promise<void> {
-    const deadline = this._page._timeoutSettings.computeDeadline(options);
     const task = selectors._dispatchEventTask(selector, type, eventInit || {});
-    const result = await this._scheduleRerunnableTask(task, 'main', deadline, `selector "${selector}"`);
-    result.dispose();
+    return Progress.runCancelableTask(async progress => {
+      progress.log(dom.inputLog, `Dispatching "${type}" event on selector "${selector}"...`);
+      const result = await this._scheduleRerunnableTask(progress, 'main', task);
+      result.dispose();
+    }, options || {}, this._page, this._page._timeoutSettings);
   }
 
   async $eval<R, Arg>(selector: string, pageFunction: types.FuncOn<Element, Arg, R>, arg: Arg): Promise<R>;
@@ -702,7 +706,9 @@ export class Frame {
       try {
         const { world, task } = selectors._waitForSelectorTask(selector, 'attached');
         this._page._log(dom.inputLog, `waiting for the selector "${selector}"`);
-        const handle = await this._scheduleRerunnableTask(task, world, deadline, `selector "${selector}"`);
+        const handle = await Progress.runCancelableTask(
+            progress => this._scheduleRerunnableTask(progress, world, task),
+            options, this._page, this._page._timeoutSettings);
         this._page._log(dom.inputLog, `...got element for the selector`);
         const element = handle.asElement() as dom.ElementHandle<Element>;
         try {
@@ -803,20 +809,23 @@ export class Frame {
   async waitForFunction<R>(pageFunction: types.Func1<void, R>, arg?: any, options?: types.WaitForFunctionOptions): Promise<types.SmartHandle<R>>;
   async waitForFunction<R, Arg>(pageFunction: types.Func1<Arg, R>, arg: Arg, options: types.WaitForFunctionOptions = {}): Promise<types.SmartHandle<R>> {
     const { polling = 'raf' } = options;
-    const deadline = this._page._timeoutSettings.computeDeadline(options);
     if (helper.isString(polling))
       assert(polling === 'raf', 'Unknown polling option: ' + polling);
     else if (helper.isNumber(polling))
       assert(polling > 0, 'Cannot poll with non-positive interval: ' + polling);
     else
       throw new Error('Unknown polling options: ' + polling);
     const predicateBody = helper.isString(pageFunction) ? 'return (' + pageFunction + ')' : 'return (' + pageFunction + ')(arg)';
-
-    const task = async (context: dom.FrameExecutionContext) => context.evaluateHandleInternal(({ injected, predicateBody, polling, arg }) => {
-      const innerPredicate = new Function('arg', predicateBody) as (arg: any) => R;
-      return injected.poll(polling, () => innerPredicate(arg));
-    }, { injected: await context.injectedScript(), predicateBody, polling, arg });
-    return this._scheduleRerunnableTask(task, 'main', deadline);
+    const task = async (context: dom.FrameExecutionContext) => {
+      const injectedScript = await context.injectedScript();
+      return context.evaluateHandleInternal(({ injectedScript, predicateBody, polling, arg }) => {
+        const innerPredicate = new Function('arg', predicateBody) as (arg: any) => R;
+        return injectedScript.poll(polling, () => innerPredicate(arg));
+      }, { injectedScript, predicateBody, polling, arg });
+    };
+    return Progress.runCancelableTask(
+        progress => this._scheduleRerunnableTask(progress, 'main', task),
+        options, this._page, this._page._timeoutSettings);
   }
 
   async title(): Promise<string> {
@@ -836,9 +845,9 @@ export class Frame {
     this._parentFrame = null;
   }
 
-  private _scheduleRerunnableTask<T>(task: SchedulableTask<T>, contextType: ContextType, deadline: number, title?: string): Promise<types.SmartHandle<T>> {
+  private _scheduleRerunnableTask<T>(progress: Progress, contextType: ContextType, task: SchedulableTask<T>): Promise<types.SmartHandle<T>> {
     const data = this._contextData.get(contextType)!;
-    const rerunnableTask = new RerunnableTask(data, task, deadline, title);
+    const rerunnableTask = new RerunnableTask(data, progress, task);
     if (data.context)
       rerunnableTask.rerun(data.context);
     return rerunnableTask.promise;
@@ -893,38 +902,24 @@ export type SchedulableTask<T> = (context: dom.FrameExecutionContext) => Promise
 
 class RerunnableTask<T> {
   readonly promise: Promise<types.SmartHandle<T>>;
-  terminate: (reason: Error) => void = () => {};
   private _task: SchedulableTask<T>;
   private _resolve: (result: types.SmartHandle<T>) => void = () => {};
   private _reject: (reason: Error) => void = () => {};
-  private _terminatedPromise: Promise<Error>;
+  private _progress: Progress;
 
-  constructor(data: ContextData, task: SchedulableTask<T>, deadline: number, title?: string) {
+  constructor(data: ContextData, progress: Progress, task: SchedulableTask<T>) {
     this._task = task;
+    this._progress = progress;
     data.rerunnableTasks.add(this);
-
-    // Since page navigation requires us to re-install the pageScript, we should track
-    // timeout on our end.
-    const timeoutError = new TimeoutError(`waiting for ${title || 'function'} failed: timeout exceeded. Re-run with the DEBUG=pw:input env variable to see the debug log.`);
-    let timeoutTimer: NodeJS.Timer | undefined;
-    this._terminatedPromise = new Promise(resolve => {
-      timeoutTimer = setTimeout(() => resolve(timeoutError), helper.timeUntilDeadline(deadline));
-      this.terminate = resolve;
-    });
-
-    // This promise is either resolved with the task result, or rejected with a meaningful
-    // evaluation error.
-    const resultPromise = new Promise<types.SmartHandle<T>>((resolve, reject) => {
+    this.promise = progress.race(new Promise<types.SmartHandle<T>>((resolve, reject) => {
+      // The task is either resolved with a value, or rejected with a meaningful evaluation error.
       this._resolve = resolve;
       this._reject = reject;
-    });
-    const failPromise = this._terminatedPromise.then(error => Promise.reject(error));
+    }));
+  }
 
-    this.promise = Promise.race([resultPromise, failPromise]).finally(() => {
-      if (timeoutTimer)
-        clearTimeout(timeoutTimer);
-      data.rerunnableTasks.delete(this);
-    });
+  terminate(error: Error) {
+    this._progress.cancel(error);
   }
 
   async rerun(context: dom.FrameExecutionContext) {
@@ -938,7 +933,7 @@ class RerunnableTask<T> {
       poll = null;
       copy.evaluate(p => p.cancel()).catch(e => {}).then(() => copy.dispose());
     };
-    this._terminatedPromise.then(cancelPoll);
+    this._progress.cleanupWhenCanceled(cancelPoll);
 
     try {
       poll = await this._task(context);

src/helper.ts

@@ -70,7 +70,7 @@ class Helper {
       const isAsync = method.constructor.name === 'AsyncFunction';
       if (!isAsync)
         continue;
-      Reflect.set(classType.prototype, methodName, function(this: any, ...args: any[]) {
+      const override = function(this: any, ...args: any[]) {
         const syncStack: any = {};
         Error.captureStackTrace(syncStack);
         return method.call(this, ...args).catch((e: any) => {
@@ -80,7 +80,9 @@ class Helper {
             e.stack += '\n  -- ASYNC --\n' + stack;
           throw e;
         });
-      });
+      };
+      Object.defineProperty(override, 'name', { writable: false, value: methodName });
+      Reflect.set(classType.prototype, methodName, override);
     }
   }
 

src/progress.ts

@@ -24,27 +24,31 @@ import { getCurrentApiCall, rewriteErrorMessage } from './debug/stackTrace';
 class AbortError extends Error {}
 
 export class Progress {
-  static async runCancelableTask<T>(task: (progress: Progress) => Promise<T>, timeoutOptions: types.TimeoutOptions, logger: InnerLogger, apiName?: string): Promise<T> {
-    let resolveCancelation = () => {};
-    const progress = new Progress(timeoutOptions, logger, new Promise(resolve => resolveCancelation = resolve), apiName);
+  static async runCancelableTask<T>(task: (progress: Progress) => Promise<T>, timeoutOptions: types.TimeoutOptions, logger: InnerLogger, timeoutSettings?: TimeoutSettings, apiName?: string): Promise<T> {
+    apiName = apiName || getCurrentApiCall();
+
+    const defaultTimeout = timeoutSettings ? timeoutSettings.timeout() : DEFAULT_TIMEOUT;
+    const { timeout = defaultTimeout } = timeoutOptions;
+    const deadline = TimeoutSettings.computeDeadline(timeout);
 
-    const { timeout = DEFAULT_TIMEOUT } = timeoutOptions;
-    const timeoutError = new TimeoutError(`Timeout ${timeout}ms exceeded during ${progress.apiName}.`);
-    let rejectWithTimeout: (error: Error) => void;
-    const timeoutPromise = new Promise<T>((resolve, x) => rejectWithTimeout = x);
-    const timeoutTimer = setTimeout(() => rejectWithTimeout(timeoutError), helper.timeUntilDeadline(progress.deadline));
+    let rejectCancelPromise: (error: Error) => void = () => {};
+    const cancelPromise = new Promise<T>((resolve, x) => rejectCancelPromise = x);
+    const timeoutError = new TimeoutError(`Timeout ${timeout}ms exceeded during ${apiName}.`);
+    const timer = setTimeout(() => rejectCancelPromise(timeoutError), helper.timeUntilDeadline(deadline));
 
+    let resolveCancelation = () => {};
+    const progress = new Progress(deadline, logger, new Promise(resolve => resolveCancelation = resolve), rejectCancelPromise, apiName);
     try {
       const promise = task(progress);
-      const result = await Promise.race([promise, timeoutPromise]);
-      clearTimeout(timeoutTimer);
+      const result = await Promise.race([promise, cancelPromise]);
+      clearTimeout(timer);
       progress._running = false;
       progress._logRecording = [];
       return result;
     } catch (e) {
       resolveCancelation();
-      rewriteErrorMessage(e, e.message + formatLogRecording(progress._logRecording, progress.apiName));
-      clearTimeout(timeoutTimer);
+      rewriteErrorMessage(e, e.message + formatLogRecording(progress._logRecording, apiName));
+      clearTimeout(timer);
       progress._running = false;
       progress._logRecording = [];
       await Promise.all(progress._cleanups.splice(0).map(cleanup => runCleanup(cleanup)));
@@ -54,16 +58,18 @@ export class Progress {
 
   readonly apiName: string;
   readonly deadline: number;  // To be removed?
+  readonly cancel: (error: Error) => void;
   readonly _canceled: Promise<any>;
 
   private _logger: InnerLogger;
   private _logRecording: string[] = [];
   private _cleanups: (() => any)[] = [];
   private _running = true;
 
-  constructor(options: types.TimeoutOptions, logger: InnerLogger, canceled: Promise<any>, apiName?: string) {
-    this.apiName = apiName || getCurrentApiCall();
-    this.deadline = TimeoutSettings.computeDeadline(options.timeout);
+  constructor(deadline: number, logger: InnerLogger, canceled: Promise<any>, cancel: (error: Error) => void, apiName: string) {
+    this.deadline = deadline;
+    this.apiName = apiName;
+    this.cancel = cancel;
     this._canceled = canceled;
     this._logger = logger;
   }
@@ -108,6 +114,8 @@ async function runCleanup(cleanup: () => any) {
 }
 
 function formatLogRecording(log: string[], name: string): string {
+  if (!log.length)
+    return '';
   name = ` ${name} logs `;
   const headerLength = 60;
   const leftLength = (headerLength - name.length) / 2;

src/timeoutSettings.ts

@@ -48,16 +48,16 @@ export class TimeoutSettings {
     return DEFAULT_TIMEOUT;
   }
 
-  private _timeout(): number {
+  timeout(): number {
     if (this._defaultTimeout !== null)
       return this._defaultTimeout;
     if (this._parent)
-      return this._parent._timeout();
+      return this._parent.timeout();
     return DEFAULT_TIMEOUT;
   }
 
   computeDeadline(options: TimeoutOptions = {}) {
-    return TimeoutSettings.computeDeadline(options.timeout, this._timeout());
+    return TimeoutSettings.computeDeadline(options.timeout, this.timeout());
   }
 
   static computeDeadline(timeout: number | undefined, defaultValue = DEFAULT_TIMEOUT): number {

test/apicoverage.spec.js

@@ -28,10 +28,12 @@ function traceAPICoverage(apiCoverage, events, className, classType) {
     if (methodName === 'constructor' || typeof methodName !== 'string' || methodName.startsWith('_') || typeof method !== 'function')
       continue;
     apiCoverage.set(`${className}.${methodName}`, false);
-    Reflect.set(classType.prototype, methodName, function(...args) {
+    const override = function(...args) {
       apiCoverage.set(`${className}.${methodName}`, true);
       return method.call(this, ...args);
-    });
+    };
+    Object.defineProperty(override, 'name', { writable: false, value: methodName });
+    Reflect.set(classType.prototype, methodName, override);
   }
 
   if (events[classType.name]) {

test/waittask.spec.js

@@ -110,15 +110,15 @@ describe('Frame.waitForFunction', function() {
     let error = null;
     await page.waitForFunction('false', {}, {timeout: 10}).catch(e => error = e);
     expect(error).toBeTruthy();
-    expect(error.message).toContain('waiting for function failed: timeout');
+    expect(error.message).toContain('Timeout 10ms exceeded during page.waitForFunction');
     expect(error).toBeInstanceOf(playwright.errors.TimeoutError);
   });
   it('should respect default timeout', async({page}) => {
     page.setDefaultTimeout(1);
     let error = null;
     await page.waitForFunction('false').catch(e => error = e);
     expect(error).toBeInstanceOf(playwright.errors.TimeoutError);
-    expect(error.message).toContain('waiting for function failed: timeout');
+    expect(error.message).toContain('Timeout 1ms exceeded during page.waitForFunction');
   });
   it('should disable timeout when its set to 0', async({page}) => {
     const watchdog = page.waitForFunction(() => {
@@ -277,10 +277,10 @@ describe('Frame.waitForSelector', function() {
   it('should not consider visible when zero-sized', async({page, server}) => {
     await page.setContent(`<div style='width: 0; height: 0;'>1</div>`);
     let error = await page.waitForSelector('div', { timeout: 1000 }).catch(e => e);
-    expect(error.message).toContain('timeout exceeded');
+    expect(error.message).toContain('Timeout 1000ms exceeded during page.waitForSelector');
     await page.evaluate(() => document.querySelector('div').style.width = '10px');
     error = await page.waitForSelector('div', { timeout: 1000 }).catch(e => e);
-    expect(error.message).toContain('timeout exceeded');
+    expect(error.message).toContain('Timeout 1000ms exceeded during page.waitForSelector');
     await page.evaluate(() => document.querySelector('div').style.height = '10px');
     expect(await page.waitForSelector('div', { timeout: 1000 })).toBeTruthy();
   });
@@ -333,15 +333,17 @@ describe('Frame.waitForSelector', function() {
     let error = null;
     await page.waitForSelector('div', { timeout: 10, state: 'attached' }).catch(e => error = e);
     expect(error).toBeTruthy();
-    expect(error.message).toContain('waiting for selector "div" failed: timeout');
+    expect(error.message).toContain('Timeout 10ms exceeded during page.waitForSelector');
+    expect(error.message).toContain('Waiting for selector "div"...');
     expect(error).toBeInstanceOf(playwright.errors.TimeoutError);
   });
   it('should have an error message specifically for awaiting an element to be hidden', async({page, server}) => {
     await page.setContent(`<div>content</div>`);
     let error = null;
     await page.waitForSelector('div', { state: 'hidden', timeout: 1000 }).catch(e => error = e);
     expect(error).toBeTruthy();
-    expect(error.message).toContain('waiting for selector "div" to be hidden failed: timeout');
+    expect(error.message).toContain('Timeout 1000ms exceeded during page.waitForSelector');
+    expect(error.message).toContain('Waiting for selector "div" to be hidden...');
   });
   it('should respond to node attribute mutation', async({page, server}) => {
     let divFound = false;
@@ -421,7 +423,8 @@ describe('Frame.waitForSelector xpath', function() {
     let error = null;
     await page.waitForSelector('//div', { state: 'attached', timeout: 10 }).catch(e => error = e);
     expect(error).toBeTruthy();
-    expect(error.message).toContain('waiting for selector "//div" failed: timeout');
+    expect(error.message).toContain('Timeout 10ms exceeded during page.waitForSelector');
+    expect(error.message).toContain('Waiting for selector "//div"...');
     expect(error).toBeInstanceOf(playwright.errors.TimeoutError);
   });
   it('should run in specified frame', async({page, server}) => {