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.
puppeteer · Oct 27, 2021 · 07febca · 07febca
1 parent 4f540d4
commit 07febca
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 33 deletions.
diff --git 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<string, BrowserContext>;
   private _screenshotTaskQueue: TaskQueue;
+  private _ignoredTargets = new Set<string>();
   /**
    * @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<void> {
+    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
@@ -115,6 +115,10 @@ export class DOMWorld {
 
   async _setContext(context?: ExecutionContext): Promise<void> {
     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
@@ -229,8 +229,7 @@ export class HTTPRequest {
    */
   async finalizeInterceptions(): Promise<void> {
     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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/);
+    });
   });
 });