From 07febca04b391893cfc872250e4391da142d4fe2 Mon Sep 17 00:00:00 2001 From: Jan Scheffler Date: Wed, 27 Oct 2021 13:43:57 +0200 Subject: [PATCH] feat: handle unhandled promise rejections in tests (#7722) In some situations, Puppeteer is left in an invalid state because protocol errors that could have been handled by the user where just hidden from them. This patch removes some of these cases and also makes sure that unhandled promise rejections lead to a test failure in mocha. --- src/common/Browser.ts | 4 +++ src/common/DOMWorld.ts | 4 +++ src/common/HTTPRequest.ts | 13 +++++++--- test/frame.spec.ts | 2 +- test/launcher.spec.ts | 8 +++--- test/mocha-utils.ts | 4 +++ test/network.spec.ts | 25 ------------------- test/requestinterception.spec.ts | 43 ++++++++++++++++++++++++++++++++ 8 files changed, 70 insertions(+), 33 deletions(-) diff --git a/src/common/Browser.ts b/src/common/Browser.ts index e8ff50bb601ef..16bbee1a5e607 100644 --- a/src/common/Browser.ts +++ b/src/common/Browser.ts @@ -240,6 +240,7 @@ export class Browser extends EventEmitter { private _defaultContext: BrowserContext; private _contexts: Map; private _screenshotTaskQueue: TaskQueue; + private _ignoredTargets = new Set(); /** * @internal * Used in Target.ts directly so cannot be marked private. @@ -374,6 +375,7 @@ export class Browser extends EventEmitter { const shouldAttachToTarget = this._targetFilterCallback(targetInfo); if (!shouldAttachToTarget) { + this._ignoredTargets.add(targetInfo.targetId); return; } @@ -398,6 +400,7 @@ export class Browser extends EventEmitter { } private async _targetDestroyed(event: { targetId: string }): Promise { + if (this._ignoredTargets.has(event.targetId)) return; const target = this._targets.get(event.targetId); target._initializedCallback(false); this._targets.delete(event.targetId); @@ -413,6 +416,7 @@ export class Browser extends EventEmitter { private _targetInfoChanged( event: Protocol.Target.TargetInfoChangedEvent ): void { + if (this._ignoredTargets.has(event.targetInfo.targetId)) return; const target = this._targets.get(event.targetInfo.targetId); assert(target, 'target should exist before targetInfoChanged'); const previousURL = target.url(); diff --git a/src/common/DOMWorld.ts b/src/common/DOMWorld.ts index 57bdd2efdda73..14d5b1037eb9d 100644 --- a/src/common/DOMWorld.ts +++ b/src/common/DOMWorld.ts @@ -115,6 +115,10 @@ export class DOMWorld { async _setContext(context?: ExecutionContext): Promise { if (context) { + assert( + this._contextResolveCallback, + 'Execution Context has already been set.' + ); this._ctxBindings.clear(); this._contextResolveCallback.call(null, context); this._contextResolveCallback = null; diff --git a/src/common/HTTPRequest.ts b/src/common/HTTPRequest.ts index d4ba66333f7ee..6f1dd4b543f62 100644 --- a/src/common/HTTPRequest.ts +++ b/src/common/HTTPRequest.ts @@ -229,8 +229,7 @@ export class HTTPRequest { */ async finalizeInterceptions(): Promise { await this._interceptActions.reduce( - (promiseChain, interceptAction) => - promiseChain.then(interceptAction).catch(handleError), + (promiseChain, interceptAction) => promiseChain.then(interceptAction), Promise.resolve() ); const [resolution] = this.interceptResolution(); @@ -440,7 +439,10 @@ export class HTTPRequest { postData: postDataBinaryBase64, headers: headers ? headersArray(headers) : undefined, }) - .catch(handleError); + .catch((error) => { + this._interceptionHandled = false; + return handleError(error); + }); } /** @@ -532,7 +534,10 @@ export class HTTPRequest { responseHeaders: headersArray(responseHeaders), body: responseBody ? responseBody.toString('base64') : undefined, }) - .catch(handleError); + .catch((error) => { + this._interceptionHandled = false; + return handleError(error); + }); } /** diff --git a/test/frame.spec.ts b/test/frame.spec.ts index f1310f6905dfd..29ae553a32392 100644 --- a/test/frame.spec.ts +++ b/test/frame.spec.ts @@ -88,7 +88,7 @@ describe('Frame specs', function () { // This test checks if Frame.evaluate allows a readonly array to be an argument. // See https://github.com/puppeteer/puppeteer/issues/6953. const readonlyArray: readonly string[] = ['a', 'b', 'c']; - mainFrame.evaluate((arr) => arr, readonlyArray); + await mainFrame.evaluate((arr) => arr, readonlyArray); }); }); diff --git a/test/launcher.spec.ts b/test/launcher.spec.ts index 9ec84e936684e..614161c99dc77 100644 --- a/test/launcher.spec.ts +++ b/test/launcher.spec.ts @@ -589,7 +589,8 @@ describe('Launcher specs', function () { await page.close(); await browser.close(); }); - it('should support targetFilter option', async () => { + // @see https://github.com/puppeteer/puppeteer/issues/4197 + itFailsFirefox('should support targetFilter option', async () => { const { server, puppeteer, defaultBrowserOptions } = getTestState(); const originalBrowser = await puppeteer.launch(defaultBrowserOptions); @@ -604,14 +605,15 @@ describe('Launcher specs', function () { const browser = await puppeteer.connect({ browserWSEndpoint, targetFilter: (targetInfo: Protocol.Target.TargetInfo) => - !targetInfo.url.includes('should-be-ignored'), + !targetInfo.url?.includes('should-be-ignored'), }); const pages = await browser.pages(); await page2.close(); await page1.close(); - await browser.close(); + await browser.disconnect(); + await originalBrowser.close(); expect(pages.map((p: Page) => p.url()).sort()).toEqual([ 'about:blank', diff --git a/test/mocha-utils.ts b/test/mocha-utils.ts index 238addfef23ac..6f701f470c707 100644 --- a/test/mocha-utils.ts +++ b/test/mocha-utils.ts @@ -221,6 +221,10 @@ console.log( }` ); +process.on('unhandledRejection', (reason) => { + throw reason; +}); + export const setupTestBrowserHooks = (): void => { before(async () => { const browser = await puppeteer.launch(defaultBrowserOptions); diff --git a/test/network.spec.ts b/test/network.spec.ts index 9450a76be2878..de6f1402ba9a0 100644 --- a/test/network.spec.ts +++ b/test/network.spec.ts @@ -67,31 +67,6 @@ describe('network', function () { expect(requests.length).toBe(2); }); }); - - describeFailsFirefox('Request.continue', () => { - it('should split a request header at new line characters and add the header multiple times instead', async () => { - const { page, server } = getTestState(); - - let resolve; - const errorPromise = new Promise((r) => { - resolve = r; - }); - - await page.setRequestInterception(true); - page.on('request', async (request) => { - await request - .continue({ - headers: { - 'X-Test-Header': 'a\nb', - }, - }) - .catch(resolve); - }); - page.goto(server.PREFIX + '/empty.html'); - const error = await errorPromise; - expect(error).toBeTruthy(); - }); - }); describe('Request.frame', function () { it('should work for main frame navigation request', async () => { const { page, server } = getTestState(); diff --git a/test/requestinterception.spec.ts b/test/requestinterception.spec.ts index 600e882961e07..0bde69e013c87 100644 --- a/test/requestinterception.spec.ts +++ b/test/requestinterception.spec.ts @@ -626,6 +626,26 @@ describe('request interception', function () { expect(serverRequest.method).toBe('POST'); expect(await serverRequest.postBody).toBe('doggo'); }); + it('should fail if the header value is invalid', async () => { + const { page, server } = getTestState(); + + let error; + await page.setRequestInterception(true); + page.on('request', async (request) => { + await request + .continue({ + headers: { + 'X-Invalid-Header': 'a\nb', + }, + }) + .catch((error_) => { + error = error_; + }); + await request.continue(); + }); + await page.goto(server.PREFIX + '/empty.html'); + expect(error.message).toMatch(/Invalid header/); + }); }); describeFailsFirefox('Request.respond', function () { @@ -732,6 +752,29 @@ describe('request interception', function () { 'Yo, page!' ); }); + it('should fail if the header value is invalid', async () => { + const { page, server } = getTestState(); + + let error; + await page.setRequestInterception(true); + page.on('request', async (request) => { + await request + .respond({ + headers: { + 'X-Invalid-Header': 'a\nb', + }, + }) + .catch((error_) => { + error = error_; + }); + await request.respond({ + status: 200, + body: 'Hello World', + }); + }); + await page.goto(server.PREFIX + '/empty.html'); + expect(error.message).toMatch(/Invalid header/); + }); }); });