From e12cf190127f8a9d08ed7d639072345f3497c650 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 21 Dec 2022 15:31:08 -0800 Subject: [PATCH] chore: refactor frame.expect to not use rerunnable task (#19626) --- packages/playwright-core/src/server/frames.ts | 160 +++++++++++++----- .../src/server/injected/injectedScript.ts | 78 ++------- .../playwright-core/src/server/progress.ts | 7 - 3 files changed, 135 insertions(+), 110 deletions(-) diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index 55ca72f62b..2e41a5384b 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -1074,14 +1074,7 @@ export class Frame extends SdkObject { continue; return result as R; } catch (e) { - // Always fail on JavaScript errors or when the main connection is closed. - if (js.isJavaScriptErrorInEvaluate(e) || isSessionClosedError(e)) - throw e; - // Certain error opt-out of the retries, throw. - if (dom.isNonRecoverableDOMError(e)) - throw e; - // If the call is made on the detached frame - throw. - if (this.isDetached()) + if (this._isErrorThatCannotBeRetried(e)) throw e; // If there is scope, and scope is within the frame we use to select, assume context is destroyed and // operation is not recoverable. @@ -1095,6 +1088,52 @@ export class Frame extends SdkObject { return undefined as any; } + async retryWithProgressAndTimeouts(progress: Progress, timeouts: number[], action: (continuePolling: symbol) => Promise): Promise { + const continuePolling = Symbol('continuePolling'); + timeouts = [0, ...timeouts]; + let timeoutIndex = 0; + while (progress.isRunning()) { + const timeout = timeouts[Math.min(timeoutIndex++, timeouts.length - 1)]; + if (timeout) { + // Make sure we react immediately upon page close or frame detach. + // We need this to show expected/received values in time. + await Promise.race([ + this._page._disconnectedPromise, + this._page._crashedPromise, + this._detachedPromise, + new Promise(f => setTimeout(f, timeout)), + ]); + } + progress.throwIfAborted(); + try { + const result = await action(continuePolling); + if (result === continuePolling) + continue; + return result as R; + } catch (e) { + if (this._isErrorThatCannotBeRetried(e)) + throw e; + continue; + } + } + progress.throwIfAborted(); + return undefined as any; + } + + private _isErrorThatCannotBeRetried(e: Error) { + // Always fail on JavaScript errors or when the main connection is closed. + if (js.isJavaScriptErrorInEvaluate(e) || isSessionClosedError(e)) + return true; + // Certain errors opt-out of the retries, throw. + if (dom.isNonRecoverableDOMError(e) || isInvalidSelectorError(e)) + return true; + // If the call is made on the detached frame - throw. + if (this.isDetached()) + return true; + // Retry upon all other errors. + return false; + } + private async _retryWithProgressIfNotConnected( progress: Progress, selector: string, @@ -1350,7 +1389,8 @@ export class Frame extends SdkObject { async expect(metadata: CallMetadata, selector: string, options: FrameExpectParams): Promise<{ matches: boolean, received?: any, log?: string[], timedOut?: boolean }> { let timeout = this._page._timeoutSettings.timeout(options); const start = timeout > 0 ? monotonicTime() : 0; - const resultOneShot = await this._expectInternal(metadata, selector, options, true, timeout); + const lastIntermediateResult: { received?: any, isSet: boolean } = { isSet: false }; + const resultOneShot = await this._expectInternal(metadata, selector, options, true, timeout, lastIntermediateResult); if (resultOneShot.matches !== options.isNot) return resultOneShot; if (timeout > 0) { @@ -1359,44 +1399,64 @@ export class Frame extends SdkObject { } if (timeout < 0) return { matches: options.isNot, log: metadata.log, timedOut: true }; - return await this._expectInternal(metadata, selector, options, false, timeout); + return await this._expectInternal(metadata, selector, options, false, timeout, lastIntermediateResult); } - private async _expectInternal(metadata: CallMetadata, selector: string, options: FrameExpectParams, oneShot: boolean, timeout: number): Promise<{ matches: boolean, received?: any, log?: string[], timedOut?: boolean }> { + 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 }> { const controller = new ProgressController(metadata, this); - const isArray = options.expression === 'to.have.count' || options.expression.endsWith('.array'); - const mainWorld = options.expression === 'to.have.property'; - - // List all combinations that are satisfied with the detached node(s). - let omitAttached = oneShot; - if (!options.isNot && options.expression === 'to.be.hidden') - omitAttached = true; - else if (options.isNot && options.expression === 'to.be.visible') - omitAttached = true; - else if (!options.isNot && options.expression === 'to.have.count' && options.expectedNumber === 0) - omitAttached = true; - else if (options.isNot && options.expression === 'to.have.count' && options.expectedNumber !== 0) - omitAttached = true; - else if (!options.isNot && options.expression.endsWith('.array') && options.expectedText!.length === 0) - omitAttached = true; - else if (options.isNot && options.expression.endsWith('.array') && options.expectedText!.length > 0) - omitAttached = true; - - return controller.run(async outerProgress => { + return controller.run(async progress => { if (oneShot) - outerProgress.log(`${metadata.apiName}${timeout ? ` with timeout ${timeout}ms` : ''}`); - return await this._scheduleRerunnableTaskWithProgress(outerProgress, selector, (progress, element, options, elements) => { - return progress.injectedScript.expect(progress, element, options, elements); - }, { ...options, oneShot }, { strict: true, querySelectorAll: isArray, mainWorld, omitAttached, logScale: true, ...options }); + 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 => { + const selectorInFrame = await this.resolveFrameForSelectorNoWait(selector, { strict: true }); + progress.throwIfAborted(); + + const { frame, info } = selectorInFrame || { frame: this, info: undefined }; + const world = options.expression === 'to.have.property' ? 'main' : (info?.world ?? 'utility'); + const context = await frame._context(world); + const injected = await context.injectedScript(); + progress.throwIfAborted(); + + const { log, matches, received } = await injected.evaluate((injected, { info, options, snapshotName }) => { + const elements = info ? injected.querySelectorAll(info.parsed, document) : []; + const isArray = options.expression === 'to.have.count' || options.expression.endsWith('.array'); + let log = ''; + if (isArray) + log = ` locator resolved to ${elements.length} element${elements.length === 1 ? '' : 's'}`; + else if (elements.length > 1) + throw injected.strictModeViolationError(info!.parsed, elements); + else if (elements.length) + log = ` locator resolved to ${injected.previewNode(elements[0])}`; + if (snapshotName) + injected.markTargetElements(new Set(elements), snapshotName); + return { log, ...injected.expect(elements[0], options, elements) }; + }, { info, options, snapshotName: progress.metadata.afterSnapshot }); + + if (log) + progress.log(log); + if (matches === options.isNot) { + lastIntermediateResult.received = received; + lastIntermediateResult.isSet = true; + if (!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 }; - const intermediateResult = controller.lastIntermediateResult(); - if (intermediateResult) - result.received = intermediateResult.value; + if (lastIntermediateResult.isSet) + result.received = lastIntermediateResult.received; else result.timedOut = true; return result; @@ -1653,7 +1713,7 @@ export class Frame extends SdkObject { return { frame, info: this._page.parseSelector(frameChunks[frameChunks.length - 1], options) }; } - async resolveFrameForSelectorNoWait(selector: string, options: types.StrictOptions & types.TimeoutOptions = {}, scope?: dom.ElementHandle): Promise { + async resolveFrameForSelectorNoWait(selector: string, options: types.StrictOptions = {}, scope?: dom.ElementHandle): Promise { let frame: Frame | null = this; const frameChunks = splitSelectorByFrame(selector); @@ -1827,3 +1887,27 @@ function verifyLifecycle(name: string, waitUntil: types.LifecycleEvent): types.L throw new Error(`${name}: expected one of (load|domcontentloaded|networkidle|commit)`); return waitUntil; } + +function renderUnexpectedValue(expression: string, received: any): string { + if (expression === 'to.be.checked') + return received ? 'checked' : 'unchecked'; + if (expression === 'to.be.unchecked') + return received ? 'unchecked' : 'checked'; + if (expression === 'to.be.visible') + return received ? 'visible' : 'hidden'; + if (expression === 'to.be.hidden') + return received ? 'hidden' : 'visible'; + if (expression === 'to.be.enabled') + return received ? 'enabled' : 'disabled'; + if (expression === 'to.be.disabled') + return received ? 'disabled' : 'enabled'; + if (expression === 'to.be.editable') + return received ? 'editable' : 'readonly'; + if (expression === 'to.be.readonly') + return received ? 'readonly' : 'editable'; + if (expression === 'to.be.empty') + return received ? 'empty' : 'not empty'; + if (expression === 'to.be.focused') + return received ? 'focused' : 'not focused'; + return received; +} diff --git a/packages/playwright-core/src/server/injected/injectedScript.ts b/packages/playwright-core/src/server/injected/injectedScript.ts index cc967d47b2..d1cd880e6b 100644 --- a/packages/playwright-core/src/server/injected/injectedScript.ts +++ b/packages/playwright-core/src/server/injected/injectedScript.ts @@ -43,12 +43,10 @@ export type InjectedScriptProgress = { aborted: boolean; log: (message: string) => void; logRepeating: (message: string) => void; - setIntermediateResult: (intermediateResult: any) => void; }; export type LogEntry = { message?: string; - intermediateResult?: string; }; export type FrameExpectParams = Omit & { expectedValue?: any }; @@ -439,7 +437,6 @@ export class InjectedScript { }); let lastMessage = ''; - let lastIntermediateResult: any = undefined; const progress: InjectedScriptProgress = { injectedScript: this, aborted: false, @@ -453,13 +450,6 @@ export class InjectedScript { if (message !== lastMessage) progress.log(message); }, - setIntermediateResult: (intermediateResult: any) => { - if (lastIntermediateResult === intermediateResult) - return; - lastIntermediateResult = intermediateResult; - unsentLog.push({ intermediateResult }); - logReady(); - }, }; const run = () => { @@ -1118,39 +1108,21 @@ export class InjectedScript { this.onGlobalListenersRemoved.add(addHitTargetInterceptorListeners); } - expect(progress: InjectedScriptProgress, element: Element | undefined, options: FrameExpectParams & { oneShot: boolean }, elements: Element[]) { + expect(element: Element | undefined, options: FrameExpectParams, elements: Element[]) { const isArray = options.expression === 'to.have.count' || options.expression.endsWith('.array'); - - let result: { matches: boolean, received?: any }; - - if (isArray) { - result = this.expectArray(elements, options); - } else { - if (!element) { - // expect(locator).toBeHidden() passes when there is no element. - if (!options.isNot && options.expression === 'to.be.hidden') - return { matches: true }; - // expect(locator).not.toBeVisible() passes when there is no element. - if (options.isNot && options.expression === 'to.be.visible') - return { matches: false }; - // When none of the above applies, keep waiting for the element. - return options.oneShot ? { matches: options.isNot } : progress.continuePolling; - } - result = this.expectSingleElement(element, options); + if (isArray) + return this.expectArray(elements, options); + if (!element) { + // expect(locator).toBeHidden() passes when there is no element. + if (!options.isNot && options.expression === 'to.be.hidden') + return { matches: true }; + // expect(locator).not.toBeVisible() passes when there is no element. + if (options.isNot && options.expression === 'to.be.visible') + return { matches: false }; + // When none of the above applies, expect does not match. + return { matches: options.isNot }; } - - if (result.matches === options.isNot) { - // Keep waiting in these cases: - // expect(locator).conditionThatDoesNotMatch - // expect(locator).not.conditionThatDoesMatch - progress.setIntermediateResult(result.received); - if (!Array.isArray(result.received)) - progress.log(` unexpected value "${this.renderUnexpectedValue(options.expression, result.received)}"`); - return options.oneShot ? result : progress.continuePolling; - } - - // Reached the expected state! - return result; + return this.expectSingleElement(element, options); } private expectSingleElement(element: Element, options: FrameExpectParams): { matches: boolean, received?: any } { @@ -1252,30 +1224,6 @@ export class InjectedScript { throw this.createStacklessError('Unknown expect matcher: ' + expression); } - private renderUnexpectedValue(expression: string, received: any): string { - if (expression === 'to.be.checked') - return received ? 'checked' : 'unchecked'; - if (expression === 'to.be.unchecked') - return received ? 'unchecked' : 'checked'; - if (expression === 'to.be.visible') - return received ? 'visible' : 'hidden'; - if (expression === 'to.be.hidden') - return received ? 'hidden' : 'visible'; - if (expression === 'to.be.enabled') - return received ? 'enabled' : 'disabled'; - if (expression === 'to.be.disabled') - return received ? 'disabled' : 'enabled'; - if (expression === 'to.be.editable') - return received ? 'editable' : 'readonly'; - if (expression === 'to.be.readonly') - return received ? 'readonly' : 'editable'; - if (expression === 'to.be.empty') - return received ? 'empty' : 'not empty'; - if (expression === 'to.be.focused') - return received ? 'focused' : 'not focused'; - return received; - } - private expectArray(elements: Element[], options: FrameExpectParams): { matches: boolean, received?: any } { const expression = options.expression; diff --git a/packages/playwright-core/src/server/progress.ts b/packages/playwright-core/src/server/progress.ts index 06df262ab3..94e7cdf1ff 100644 --- a/packages/playwright-core/src/server/progress.ts +++ b/packages/playwright-core/src/server/progress.ts @@ -43,7 +43,6 @@ export class ProgressController { private _state: 'before' | 'running' | 'aborted' | 'finished' = 'before'; private _deadline: number = 0; private _timeout: number = 0; - private _lastIntermediateResult: { value: any } | undefined; readonly metadata: CallMetadata; readonly instrumentation: Instrumentation; readonly sdkObject: SdkObject; @@ -59,10 +58,6 @@ export class ProgressController { this._logName = logName; } - lastIntermediateResult(): { value: any } | undefined { - return this._lastIntermediateResult; - } - abort(error: Error) { this._forceAbortPromise.reject(error); } @@ -89,8 +84,6 @@ export class ProgressController { // Note: we might be sending logs after progress has finished, for example browser logs. this.instrumentation.onCallLog(this.sdkObject, this.metadata, this._logName, message); } - if ('intermediateResult' in entry) - this._lastIntermediateResult = { value: entry.intermediateResult }; }, timeUntilDeadline: () => this._deadline ? this._deadline - monotonicTime() : 2147483647, // 2^31-1 safe setTimeout in Node. isRunning: () => this._state === 'running',