From b7df4d57a46482bc171cc2b362c542699477da93 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 1 Jun 2020 08:54:18 -0700 Subject: [PATCH] chore: migrate wait tasks to Progress (#2422) --- src/debug/stackTrace.ts | 3 +- src/frames.ts | 107 +++++++++++++++++++-------------------- src/helper.ts | 6 ++- src/progress.ts | 40 +++++++++------ src/timeoutSettings.ts | 6 +-- test/apicoverage.spec.js | 6 ++- test/waittask.spec.js | 17 ++++--- 7 files changed, 98 insertions(+), 87 deletions(-) diff --git a/src/debug/stackTrace.ts b/src/debug/stackTrace.ts index 23c230f640..0eba24fc7f 100644 --- a/src/debug/stackTrace.ts +++ b/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; } diff --git a/src/frames.ts b/src/frames.ts index 0eb8a8c162..1054d9fce5 100644 --- a/src/frames.ts +++ b/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 | null> { - if (options && (options as any).visibility) + async waitForSelector(selector: string, options: types.WaitForElementOptions = {}): Promise | 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; - 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; + 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 { - 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(selector: string, pageFunction: types.FuncOn, arg: Arg): Promise; @@ -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; try { @@ -803,7 +809,6 @@ export class Frame { async waitForFunction(pageFunction: types.Func1, arg?: any, options?: types.WaitForFunctionOptions): Promise>; async waitForFunction(pageFunction: types.Func1, arg: Arg, options: types.WaitForFunctionOptions = {}): Promise> { 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)) @@ -811,12 +816,16 @@ export class Frame { 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 { @@ -836,9 +845,9 @@ export class Frame { this._parentFrame = null; } - private _scheduleRerunnableTask(task: SchedulableTask, contextType: ContextType, deadline: number, title?: string): Promise> { + private _scheduleRerunnableTask(progress: Progress, contextType: ContextType, task: SchedulableTask): Promise> { 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 = (context: dom.FrameExecutionContext) => Promise class RerunnableTask { readonly promise: Promise>; - terminate: (reason: Error) => void = () => {}; private _task: SchedulableTask; private _resolve: (result: types.SmartHandle) => void = () => {}; private _reject: (reason: Error) => void = () => {}; - private _terminatedPromise: Promise; + private _progress: Progress; - constructor(data: ContextData, task: SchedulableTask, deadline: number, title?: string) { + constructor(data: ContextData, progress: Progress, task: SchedulableTask) { 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>((resolve, reject) => { + this.promise = progress.race(new Promise>((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 { 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); diff --git a/src/helper.ts b/src/helper.ts index ad1b925de2..3c533117ae 100644 --- a/src/helper.ts +++ b/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); } } diff --git a/src/progress.ts b/src/progress.ts index e6f7e7d7d1..259202b68d 100644 --- a/src/progress.ts +++ b/src/progress.ts @@ -24,27 +24,31 @@ import { getCurrentApiCall, rewriteErrorMessage } from './debug/stackTrace'; class AbortError extends Error {} export class Progress { - static async runCancelableTask(task: (progress: Progress) => Promise, timeoutOptions: types.TimeoutOptions, logger: InnerLogger, apiName?: string): Promise { + static async runCancelableTask(task: (progress: Progress) => Promise, timeoutOptions: types.TimeoutOptions, logger: InnerLogger, timeoutSettings?: TimeoutSettings, apiName?: string): Promise { + apiName = apiName || getCurrentApiCall(); + + const defaultTimeout = timeoutSettings ? timeoutSettings.timeout() : DEFAULT_TIMEOUT; + const { timeout = defaultTimeout } = timeoutOptions; + const deadline = TimeoutSettings.computeDeadline(timeout); + + let rejectCancelPromise: (error: Error) => void = () => {}; + const cancelPromise = new Promise((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(timeoutOptions, logger, new Promise(resolve => resolveCancelation = resolve), apiName); - - 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((resolve, x) => rejectWithTimeout = x); - const timeoutTimer = setTimeout(() => rejectWithTimeout(timeoutError), helper.timeUntilDeadline(progress.deadline)); - + 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,6 +58,7 @@ export class Progress { readonly apiName: string; readonly deadline: number; // To be removed? + readonly cancel: (error: Error) => void; readonly _canceled: Promise; private _logger: InnerLogger; @@ -61,9 +66,10 @@ export class Progress { private _cleanups: (() => any)[] = []; private _running = true; - constructor(options: types.TimeoutOptions, logger: InnerLogger, canceled: Promise, apiName?: string) { - this.apiName = apiName || getCurrentApiCall(); - this.deadline = TimeoutSettings.computeDeadline(options.timeout); + constructor(deadline: number, logger: InnerLogger, canceled: Promise, 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; diff --git a/src/timeoutSettings.ts b/src/timeoutSettings.ts index 25673510ef..3a4a306314 100644 --- a/src/timeoutSettings.ts +++ b/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 { diff --git a/test/apicoverage.spec.js b/test/apicoverage.spec.js index e70f13cc68..7339153ce0 100644 --- a/test/apicoverage.spec.js +++ b/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]) { diff --git a/test/waittask.spec.js b/test/waittask.spec.js index 2b52882981..0d5979405b 100644 --- a/test/waittask.spec.js +++ b/test/waittask.spec.js @@ -110,7 +110,7 @@ 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}) => { @@ -118,7 +118,7 @@ describe('Frame.waitForFunction', function() { 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(`
1
`); 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,7 +333,8 @@ 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}) => { @@ -341,7 +342,8 @@ describe('Frame.waitForSelector', function() { 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}) => {