From bb2b29631ad43c03b578689c7868230d90243b6d Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Fri, 19 Feb 2021 09:33:24 -0800 Subject: [PATCH] feat(inspector): pause on page/context close (#5319) --- src/browserServerImpl.ts | 2 +- src/dispatchers/browserContextDispatcher.ts | 8 ++-- src/dispatchers/pageDispatcher.ts | 2 +- src/protocol/channels.ts | 2 + src/protocol/protocol.yml | 1 + src/protocol/validator.ts | 1 + src/server/browser.ts | 7 ---- src/server/browserContext.ts | 24 ++++++------ src/server/page.ts | 4 +- src/server/supplements/inspectorController.ts | 16 ++++---- .../supplements/recorder/recorderApp.ts | 4 +- src/server/supplements/recorderSupplement.ts | 17 +++++++-- test/pause.spec.ts | 37 ++++++++++++++++++- 13 files changed, 85 insertions(+), 40 deletions(-) diff --git a/src/browserServerImpl.ts b/src/browserServerImpl.ts index 23ff0be8ed..eb035e43cf 100644 --- a/src/browserServerImpl.ts +++ b/src/browserServerImpl.ts @@ -171,7 +171,7 @@ class ConnectedBrowser extends BrowserDispatcher { async close(): Promise { // Only close our own contexts. - await Promise.all(this._contexts.map(context => context.close())); + await Promise.all(this._contexts.map(context => context.close({}, internalCallMetadata()))); this._didClose(); } diff --git a/src/dispatchers/browserContextDispatcher.ts b/src/dispatchers/browserContextDispatcher.ts index e8441b6a28..740ed0c275 100644 --- a/src/dispatchers/browserContextDispatcher.ts +++ b/src/dispatchers/browserContextDispatcher.ts @@ -65,8 +65,8 @@ export class BrowserContextDispatcher extends Dispatcher { - return { page: lookupDispatcher(await this._context.newPage()) }; + async newPage(params: channels.BrowserContextNewPageParams, metadata: CallMetadata): Promise { + return { page: lookupDispatcher(await this._context.newPage(metadata)) }; } async cookies(params: channels.BrowserContextCookiesParams): Promise { @@ -123,8 +123,8 @@ export class BrowserContextDispatcher extends Dispatcher { - await this._context.close(); + async close(params: channels.BrowserContextCloseParams, metadata: CallMetadata): Promise { + await this._context.close(metadata); } async recorderSupplementEnable(params: channels.BrowserContextRecorderSupplementEnableParams): Promise { diff --git a/src/dispatchers/pageDispatcher.ts b/src/dispatchers/pageDispatcher.ts index 19940c5c6e..9ec902ae14 100644 --- a/src/dispatchers/pageDispatcher.ts +++ b/src/dispatchers/pageDispatcher.ts @@ -146,7 +146,7 @@ export class PageDispatcher extends Dispatcher i } async close(params: channels.PageCloseParams, metadata: CallMetadata): Promise { - await this._page.close(params); + await this._page.close(metadata, params); } async setFileChooserInterceptedNoReply(params: channels.PageSetFileChooserInterceptedNoReplyParams, metadata: CallMetadata): Promise { diff --git a/src/protocol/channels.ts b/src/protocol/channels.ts index 81d194e011..03f175e5ad 100644 --- a/src/protocol/channels.ts +++ b/src/protocol/channels.ts @@ -738,6 +738,7 @@ export type BrowserContextPauseResult = void; export type BrowserContextRecorderSupplementEnableParams = { language?: string, startRecording?: boolean, + pauseOnNextStatement?: boolean, launchOptions?: any, contextOptions?: any, device?: string, @@ -747,6 +748,7 @@ export type BrowserContextRecorderSupplementEnableParams = { export type BrowserContextRecorderSupplementEnableOptions = { language?: string, startRecording?: boolean, + pauseOnNextStatement?: boolean, launchOptions?: any, contextOptions?: any, device?: string, diff --git a/src/protocol/protocol.yml b/src/protocol/protocol.yml index 65279dcb83..dba0a267a7 100644 --- a/src/protocol/protocol.yml +++ b/src/protocol/protocol.yml @@ -648,6 +648,7 @@ BrowserContext: parameters: language: string? startRecording: boolean? + pauseOnNextStatement: boolean? launchOptions: json? contextOptions: json? device: string? diff --git a/src/protocol/validator.ts b/src/protocol/validator.ts index 5e10293a05..d69edaeb12 100644 --- a/src/protocol/validator.ts +++ b/src/protocol/validator.ts @@ -363,6 +363,7 @@ export function createScheme(tChannel: (name: string) => Validator): Scheme { scheme.BrowserContextRecorderSupplementEnableParams = tObject({ language: tOptional(tString), startRecording: tOptional(tBoolean), + pauseOnNextStatement: tOptional(tBoolean), launchOptions: tOptional(tAny), contextOptions: tOptional(tAny), device: tOptional(tString), diff --git a/src/server/browser.ts b/src/server/browser.ts index 15f5c67d4b..ef14ed3155 100644 --- a/src/server/browser.ts +++ b/src/server/browser.ts @@ -72,13 +72,6 @@ export abstract class Browser extends SdkObject { abstract isConnected(): boolean; abstract version(): string; - async newPage(options: types.BrowserContextOptions): Promise { - const context = await this.newContext(options); - const page = await context.newPage(); - page._ownedContext = context; - return page; - } - _downloadCreated(page: Page, uuid: string, url: string, suggestedFilename?: string) { const download = new Download(page, this.options.downloadsPath || '', uuid, url, suggestedFilename); this._downloads.set(uuid, download); diff --git a/src/server/browserContext.ts b/src/server/browserContext.ts index 62411f030b..21d95c0206 100644 --- a/src/server/browserContext.ts +++ b/src/server/browserContext.ts @@ -27,7 +27,7 @@ import { Progress } from './progress'; import { Selectors, serverSelectors } from './selectors'; import * as types from './types'; import path from 'path'; -import { CallMetadata, SdkObject } from './instrumentation'; +import { CallMetadata, internalCallMetadata, SdkObject } from './instrumentation'; export class Video { readonly _videoId: string; @@ -209,8 +209,8 @@ export abstract class BrowserContext extends SdkObject { // - chromium fails to change isMobile for existing page; // - webkit fails to change locale for existing page. const oldPage = pages[0]; - await this.newPage(); - await oldPage.close(); + await this.newPage(progress.metadata); + await oldPage.close(progress.metadata); } } @@ -245,7 +245,7 @@ export abstract class BrowserContext extends SdkObject { return this._closedStatus !== 'open'; } - async close() { + async close(metadata: CallMetadata) { if (this._closedStatus === 'open') { this.emit(BrowserContext.Events.BeforeClose); this._closedStatus = 'closing'; @@ -255,7 +255,7 @@ export abstract class BrowserContext extends SdkObject { if (this._isPersistentContext) { // Close all the pages instead of the context, // because we cannot close the default context. - await Promise.all(this.pages().map(page => page.close())); + await Promise.all(this.pages().map(page => page.close(metadata))); } else { // Close the context. await this._doClose(); @@ -286,7 +286,7 @@ export abstract class BrowserContext extends SdkObject { await this._closePromise; } - async newPage(): Promise { + async newPage(metadata: CallMetadata): Promise { const pageDelegate = await this.newPageDelegate(); const pageOrError = await pageDelegate.pageOrError(); if (pageOrError instanceof Page) { @@ -307,7 +307,8 @@ export abstract class BrowserContext extends SdkObject { origins: [] }; if (this._origins.size) { - const page = await this.newPage(); + const internalMetadata = internalCallMetadata(); + const page = await this.newPage(internalMetadata); await page._setServerRequestInterceptor(handler => { handler.fulfill({ body: '' }).catch(() => {}); }); @@ -315,13 +316,13 @@ export abstract class BrowserContext extends SdkObject { const originStorage: types.OriginStorage = { origin, localStorage: [] }; result.origins.push(originStorage); const frame = page.mainFrame(); - await frame.goto(metadata, origin); + await frame.goto(internalMetadata, origin); const storage = await frame._evaluateExpression(`({ localStorage: Object.keys(localStorage).map(name => ({ name, value: localStorage.getItem(name) })), })`, false, undefined, 'utility'); originStorage.localStorage = storage.localStorage; } - await page.close(); + await page.close(internalMetadata); } return result; } @@ -330,7 +331,8 @@ export abstract class BrowserContext extends SdkObject { if (state.cookies) await this.addCookies(state.cookies); if (state.origins && state.origins.length) { - const page = await this.newPage(); + const internalMetadata = internalCallMetadata(); + const page = await this.newPage(internalMetadata); await page._setServerRequestInterceptor(handler => { handler.fulfill({ body: '' }).catch(() => {}); }); @@ -343,7 +345,7 @@ export abstract class BrowserContext extends SdkObject { localStorage.setItem(name, value); }`, true, originState, 'utility'); } - await page.close(); + await page.close(internalMetadata); } } diff --git a/src/server/page.ts b/src/server/page.ts index cd7d8e15ac..dd7fa21512 100644 --- a/src/server/page.ts +++ b/src/server/page.ts @@ -428,7 +428,7 @@ export class Page extends SdkObject { this._timeoutSettings.timeout(options)); } - async close(options?: { runBeforeUnload?: boolean }) { + async close(metadata: CallMetadata, options?: { runBeforeUnload?: boolean }) { if (this._closedState === 'closed') return; const runBeforeUnload = !!options && !!options.runBeforeUnload; @@ -442,7 +442,7 @@ export class Page extends SdkObject { if (!runBeforeUnload) await this._closedPromise; if (this._ownedContext) - await this._ownedContext.close(); + await this._ownedContext.close(metadata); } private _setIsError() { diff --git a/src/server/supplements/inspectorController.ts b/src/server/supplements/inspectorController.ts index 4ecbc76545..8d8f8bdc9c 100644 --- a/src/server/supplements/inspectorController.ts +++ b/src/server/supplements/inspectorController.ts @@ -25,7 +25,7 @@ export class InspectorController implements InstrumentationListener { async onContextCreated(context: BrowserContext): Promise { if (isDebugMode()) - RecorderSupplement.getOrCreate(context); + RecorderSupplement.getOrCreate(context, { pauseOnNextStatement: true }); } async onBeforeCall(sdkObject: SdkObject, metadata: CallMetadata): Promise { @@ -52,12 +52,8 @@ export class InspectorController implements InstrumentationListener { } } - if (metadata.method === 'pause') { - // Force create recorder on pause. - if (!context._browser.options.headful && !isUnderTest()) - return; - RecorderSupplement.getOrCreate(context); - } + if (shouldOpenInspector(sdkObject, metadata)) + RecorderSupplement.getOrCreate(context, { pauseOnNextStatement: true }); const recorder = await RecorderSupplement.getNoCreate(context); await recorder?.onBeforeCall(sdkObject, metadata); @@ -104,3 +100,9 @@ export class InspectorController implements InstrumentationListener { await recorder?.updateCallLog([metadata]); } } + +function shouldOpenInspector(sdkObject: SdkObject, metadata: CallMetadata): boolean { + if (!sdkObject.attribution.browser?.options.headful && !isUnderTest()) + return false; + return metadata.method === 'pause'; +} diff --git a/src/server/supplements/recorder/recorderApp.ts b/src/server/supplements/recorder/recorderApp.ts index 30eccc7ca3..6c5de1f98e 100644 --- a/src/server/supplements/recorder/recorderApp.ts +++ b/src/server/supplements/recorder/recorderApp.ts @@ -53,7 +53,7 @@ export class RecorderApp extends EventEmitter { } async close() { - await this._page.context().close(); + await this._page.context().close(internalCallMetadata()); } private async _init() { @@ -85,7 +85,7 @@ export class RecorderApp extends EventEmitter { this._page.once('close', () => { this.emit('close'); - this._page.context().close().catch(e => console.error(e)); + this._page.context().close(internalCallMetadata()).catch(e => console.error(e)); }); const mainFrame = this._page.mainFrame(); diff --git a/src/server/supplements/recorderSupplement.ts b/src/server/supplements/recorderSupplement.ts index b61fd6504e..5e0e9f729f 100644 --- a/src/server/supplements/recorderSupplement.ts +++ b/src/server/supplements/recorderSupplement.ts @@ -50,7 +50,7 @@ export class RecorderSupplement { private _params: channels.BrowserContextRecorderSupplementEnableParams; private _currentCallsMetadata = new Map(); private _pausedCallsMetadata = new Map void>(); - private _pauseOnNextStatement = false; + private _pauseOnNextStatement: boolean; private _recorderSources: Source[]; private _userSources = new Map(); @@ -72,6 +72,7 @@ export class RecorderSupplement { this._context = context; this._params = params; this._mode = params.startRecording ? 'recording' : 'none'; + this._pauseOnNextStatement = !!params.pauseOnNextStatement; const language = params.language || context._options.sdkLanguage; const languages = new Set([ @@ -367,7 +368,7 @@ export class RecorderSupplement { this._currentCallsMetadata.set(metadata, sdkObject); this._updateUserSources(); this.updateCallLog([metadata]); - if (metadata.method === 'pause' || (this._pauseOnNextStatement && metadata.method === 'goto')) + if (shouldPauseOnCall(sdkObject, metadata) || (this._pauseOnNextStatement && shouldPauseOnStep(sdkObject, metadata))) await this.pause(metadata); if (metadata.params && metadata.params.selector) { this._highlightedSelector = metadata.params.selector; @@ -477,4 +478,14 @@ function languageForFile(file: string) { if (file.endsWith('.cs')) return 'csharp'; return 'javascript'; -} \ No newline at end of file +} + +function shouldPauseOnCall(sdkObject: SdkObject, metadata: CallMetadata): boolean { + if (!sdkObject.attribution.browser?.options.headful && !isUnderTest()) + return false; + return metadata.method === 'pause'; +} + +function shouldPauseOnStep(sdkObject: SdkObject, metadata: CallMetadata): boolean { + return metadata.method === 'goto' || metadata.method === 'close'; +} diff --git a/test/pause.spec.ts b/test/pause.spec.ts index 9506355c19..db3340c074 100644 --- a/test/pause.spec.ts +++ b/test/pause.spec.ts @@ -17,11 +17,20 @@ import { expect } from 'folio'; import { Page } from '..'; import { folio } from './recorder.fixtures'; -const { it, describe} = folio; +const { afterEach, it, describe } = folio; describe('pause', (suite, { mode }) => { suite.skip(mode !== 'default'); }, () => { + afterEach(async ({ recorderPageGetter }) => { + try { + const recorderPage = await recorderPageGetter(); + recorderPage.click('[title=Resume]').catch(() => {}); + } catch (e) { + // Some tests close context. + } + }); + it('should pause and resume the script', async ({ page, recorderPageGetter }) => { const scriptPromise = (async () => { await page.pause(); @@ -117,7 +126,7 @@ describe('pause', (suite, { mode }) => { expect(Math.abs(x1 - x2) < 2).toBeTruthy(); expect(Math.abs(y1 - y2) < 2).toBeTruthy(); - await recorderPage.click('[title="Step over"]'); + await recorderPage.click('[title=Resume]'); await scriptPromise; }); @@ -196,6 +205,30 @@ describe('pause', (suite, { mode }) => { const error = await scriptPromise; expect(error.message).toContain('Not a checkbox or radio button'); }); + + it('should pause on page close', async ({ page, recorderPageGetter }) => { + const scriptPromise = (async () => { + await page.pause(); + await page.close(); + })(); + const recorderPage = await recorderPageGetter(); + await recorderPage.click('[title="Step over"]'); + await recorderPage.waitForSelector('.source-line-paused:has-text("page.close();")'); + await recorderPage.click('[title=Resume]'); + await scriptPromise; + }); + + it('should pause on context close', async ({ page, recorderPageGetter }) => { + const scriptPromise = (async () => { + await page.pause(); + await page.context().close(); + })(); + const recorderPage = await recorderPageGetter(); + await recorderPage.click('[title="Step over"]'); + await recorderPage.waitForSelector('.source-line-paused:has-text("page.context().close();")'); + await recorderPage.click('[title=Resume]'); + await scriptPromise; + }); }); async function sanitizeLog(recorderPage: Page): Promise {