From 225145fc3e57cb73f209fe54332803dccfcb0b35 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 20 Oct 2021 12:01:05 -0800 Subject: [PATCH] fix(expect): do not fail on navigated frames while polling (#9659) --- packages/playwright-core/src/server/dom.ts | 22 ++++++--- packages/playwright-core/src/server/frames.ts | 45 ++++++++++++------- .../playwright-test/src/reporters/base.ts | 2 +- tests/page/matchers.misc.spec.ts | 25 +++++++++++ 4 files changed, 71 insertions(+), 23 deletions(-) create mode 100644 tests/page/matchers.misc.spec.ts diff --git a/packages/playwright-core/src/server/dom.ts b/packages/playwright-core/src/server/dom.ts index 9599c11625..8397ebf2f0 100644 --- a/packages/playwright-core/src/server/dom.ts +++ b/packages/playwright-core/src/server/dom.ts @@ -505,7 +505,7 @@ export class ElementHandle extends js.JSHandle { if (poll === 'error:notconnected') return poll; const pollHandler = new InjectedScriptPollHandler(progress, poll); - const result = await pollHandler.finish(); + const result = await pollHandler.finishMaybeNotConnected(); await this._page._doSlowMo(); return result; }); @@ -530,7 +530,7 @@ export class ElementHandle extends js.JSHandle { if (poll === 'error:notconnected') return poll; const pollHandler = new InjectedScriptPollHandler(progress, poll); - const filled = await pollHandler.finish(); + const filled = await pollHandler.finishMaybeNotConnected(); progress.throwIfAborted(); // Avoid action that has side-effects. if (filled === 'error:notconnected') return filled; @@ -556,7 +556,7 @@ export class ElementHandle extends js.JSHandle { return injected.waitForElementStatesAndPerformAction(node, ['visible'], force, injected.selectText.bind(injected)); }, options.force); const pollHandler = new InjectedScriptPollHandler(progress, throwRetargetableDOMError(poll)); - const result = await pollHandler.finish(); + const result = await pollHandler.finishMaybeNotConnected(); assertDone(throwRetargetableDOMError(result)); }, this._page._timeoutSettings.timeout(options)); } @@ -761,7 +761,7 @@ export class ElementHandle extends js.JSHandle { return injected.waitForElementStatesAndPerformAction(node, [state], false, () => 'done' as const); }, state); const pollHandler = new InjectedScriptPollHandler(progress, throwRetargetableDOMError(poll)); - assertDone(throwRetargetableDOMError(await pollHandler.finish())); + assertDone(throwRetargetableDOMError(await pollHandler.finishMaybeNotConnected())); }, this._page._timeoutSettings.timeout(options)); } @@ -808,7 +808,7 @@ export class ElementHandle extends js.JSHandle { if (poll === 'error:notconnected') return poll; const pollHandler = new InjectedScriptPollHandler(progress, poll); - const result = await pollHandler.finish(); + const result = await pollHandler.finishMaybeNotConnected(); if (waitForEnabled) progress.log(' element is visible, enabled and stable'); else @@ -867,7 +867,17 @@ export class InjectedScriptPollHandler { } } - async finish(): Promise { + async finish(): Promise { + try { + const result = await this._poll!.evaluate(poll => poll.run()); + await this._finishInternal(); + return result; + } finally { + await this.cancel(); + } + } + + async finishMaybeNotConnected(): Promise { try { const result = await this._poll!.evaluate(poll => poll.run()); await this._finishInternal(); diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index cf5f7f0e2a..400f3881fd 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -38,7 +38,7 @@ type ContextData = { contextPromise: Promise; contextResolveCallback: (c: dom.FrameExecutionContext) => void; context: dom.FrameExecutionContext | null; - rerunnableTasks: Set; + rerunnableTasks: Set>; }; type DocumentInfo = { @@ -1201,6 +1201,8 @@ export class Frame extends SdkObject { }, options, { strict: true, querySelectorAll, mainWorld, omitAttached: true, logScale: true, ...options }).catch(e => { if (js.isJavaScriptErrorInEvaluate(e)) throw 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. return { received: controller.lastIntermediateResult(), matches: options.isNot, log: metadata.log }; }); } @@ -1280,7 +1282,7 @@ export class Frame extends SdkObject { return controller.run(async progress => { progress.log(`waiting for selector "${selector}"`); - const rerunnableTask = new RerunnableTask(data, progress, injectedScript => { + const rerunnableTask = new RerunnableTask(data, progress, injectedScript => { return injectedScript.evaluateHandle((injected, { info, taskData, callbackText, querySelectorAll, logScale, omitAttached, snapshotName }) => { const callback = injected.eval(callbackText) as DomTaskBody; const poller = logScale ? injected.pollLogScale.bind(injected) : injected.pollRaf.bind(injected); @@ -1324,18 +1326,18 @@ export class Frame extends SdkObject { rerunnableTask.terminate(new Error('Frame got detached.')); if (data.context) rerunnableTask.rerun(data.context); - return await rerunnableTask.promise; + return await rerunnableTask.promise!; }, this._page._timeoutSettings.timeout(options)); } private _scheduleRerunnableHandleTask(progress: Progress, world: types.World, task: dom.SchedulableTask): Promise> { const data = this._contextData.get(world)!; - const rerunnableTask = new RerunnableTask(data, progress, task, false /* returnByValue */); + const rerunnableTask = new RerunnableTask(data, progress, task, false /* returnByValue */); if (this._detached) rerunnableTask.terminate(new Error('waitForFunction failed: frame got detached.')); if (data.context) rerunnableTask.rerun(data.context); - return rerunnableTask.promise; + return rerunnableTask.handlePromise!; } private _setContext(world: types.World, context: dom.FrameExecutionContext | null) { @@ -1394,31 +1396,42 @@ export class Frame extends SdkObject { } } -class RerunnableTask { - readonly promise: Promise; - private _task: dom.SchedulableTask; - private _resolve: (result: any) => void = () => {}; - private _reject: (reason: Error) => void = () => {}; +class RerunnableTask { + readonly promise: ManualPromise | undefined; + readonly handlePromise: ManualPromise> | undefined; + private _task: dom.SchedulableTask; private _progress: Progress; private _returnByValue: boolean; private _contextData: ContextData; - constructor(data: ContextData, progress: Progress, task: dom.SchedulableTask, returnByValue: boolean) { + constructor(data: ContextData, progress: Progress, task: dom.SchedulableTask, returnByValue: boolean) { this._task = task; this._progress = progress; this._returnByValue = returnByValue; + if (returnByValue) + this.promise = new ManualPromise(); + else + this.handlePromise = new ManualPromise>(); this._contextData = data; this._contextData.rerunnableTasks.add(this); - this.promise = 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; - }); } terminate(error: Error) { this._reject(error); } + private _resolve(value: T | js.SmartHandle) { + if (this.promise) + this.promise.resolve(value as T); + if (this.handlePromise) + this.handlePromise.resolve(value as js.SmartHandle); + } + + private _reject(error: Error) { + if (this.promise) + this.promise.reject(error); + if (this.handlePromise) + this.handlePromise.reject(error); + } async rerun(context: dom.FrameExecutionContext) { try { diff --git a/packages/playwright-test/src/reporters/base.ts b/packages/playwright-test/src/reporters/base.ts index 84b2e5e2a5..21c1553432 100644 --- a/packages/playwright-test/src/reporters/base.ts +++ b/packages/playwright-test/src/reporters/base.ts @@ -98,7 +98,7 @@ export class BaseReporter implements Reporter { } onError(error: TestError) { - console.log(formatError(error, colors.enabled)); + console.log(formatError(error, colors.enabled).message); } async onEnd(result: FullResult) { diff --git a/tests/page/matchers.misc.spec.ts b/tests/page/matchers.misc.spec.ts new file mode 100644 index 0000000000..360549177c --- /dev/null +++ b/tests/page/matchers.misc.spec.ts @@ -0,0 +1,25 @@ +/** + * Copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { test as it, expect } from './pageTest'; + +it('should outlive frame navigation', async ({ page, server }) => { + await page.goto(server.EMPTY_PAGE); + setTimeout(async () => { + await page.goto(server.PREFIX + '/grid.html').catch(() => {}); + }, 1000); + await expect(page.locator('.box').first()).toBeEmpty(); +});