chore: enforce naming of errors in catch blocks (#5763)

puppeteer · Apr 28, 2020 · 1ccfbcb · 1ccfbcb
1 parent 3bf9bd1
commit 1ccfbcb
Show file tree

Hide file tree

Showing 35 changed files with 191 additions and 186 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -9,7 +9,8 @@ module.exports = {
 
     "plugins": [
         "mocha",
-        "@typescript-eslint"
+        "@typescript-eslint",
+        "unicorn"
     ],
 
     "rules": {
@@ -99,7 +100,10 @@ module.exports = {
         }],
 
         // ensure we don't have any it.only or describe.only in prod
-        "mocha/no-exclusive-tests": "error"
+        "mocha/no-exclusive-tests": "error",
+
+        // enforce the variable in a catch block is named error
+        "unicorn/catch-error-name": "error"
     },
     "overrides": [
         {

diff --git a/package.json b/package.json
@@ -69,6 +69,7 @@
     "cross-env": "^5.0.5",
     "eslint": "^6.8.0",
     "eslint-plugin-mocha": "^6.3.0",
+    "eslint-plugin-unicorn": "^19.0.1",
     "esprima": "^4.0.0",
     "expect": "^25.2.7",
     "jpeg-js": "^0.3.7",

diff --git a/src/BrowserFetcher.ts b/src/BrowserFetcher.ts
@@ -429,7 +429,7 @@ function installDMG(dmgPath: string, folderPath: string): Promise<void> {
     });
   }
 
-  return new Promise<void>(mountAndCopy).catch(err => { console.error(err); }).finally(unmount);
+  return new Promise<void>(mountAndCopy).catch(error => { console.error(error); }).finally(unmount);
 }
 
 function httpRequest(url: string, method: string, response: (x: http.IncomingMessage) => void): http.ClientRequest {

diff --git a/src/Coverage.ts b/src/Coverage.ts
@@ -105,9 +105,9 @@ class JSCoverage {
       const response = await this._client.send('Debugger.getScriptSource', {scriptId: event.scriptId});
       this._scriptURLs.set(event.scriptId, event.url);
       this._scriptSources.set(event.scriptId, response.scriptSource);
-    } catch (e) {
+    } catch (error) {
       // This might happen if the page has already navigated away.
-      debugError(e);
+      debugError(error);
     }
   }
 
@@ -196,9 +196,9 @@ class CSSCoverage {
       const response = await this._client.send('CSS.getStyleSheetText', {styleSheetId: header.styleSheetId});
       this._stylesheetURLs.set(header.styleSheetId, header.sourceURL);
       this._stylesheetSources.set(header.styleSheetId, response.text);
-    } catch (e) {
+    } catch (error) {
       // This might happen if the page has already navigated away.
-      debugError(e);
+      debugError(error);
     }
   }
 

diff --git a/src/DOMWorld.js b/src/DOMWorld.js
@@ -579,8 +579,8 @@ class WaitTask {
     let error = null;
     try {
       success = await (await this._domWorld.executionContext()).evaluateHandle(waitForPredicatePageFunction, this._predicateBody, this._polling, this._timeout, ...this._args);
-    } catch (e) {
-      error = e;
+    } catch (error_) {
+      error = error_;
     }
 
     if (this._terminated || runCount !== this._runCount) {
@@ -592,7 +592,7 @@ class WaitTask {
     // Ignore timeouts in pageScript - we track timeouts ourselves.
     // If the frame's execution context has already changed, `frame.evaluate` will
     // throw an error - ignore this predicate run altogether.
-    if (!error && await this._domWorld.evaluate(s => !s, success).catch(e => true)) {
+    if (!error && await this._domWorld.evaluate(s => !s, success).catch(error_ => true)) {
       await success.dispose();
       return;
     }

diff --git a/src/ExecutionContext.ts b/src/ExecutionContext.ts
@@ -72,7 +72,7 @@ export class ExecutionContext {
     let functionText = pageFunction.toString();
     try {
       new Function('(' + functionText + ')');
-    } catch (e1) {
+    } catch (error) {
       // This means we might have a function shorthand. Try another
       // time prefixing 'function '.
       if (functionText.startsWith('async '))
@@ -81,7 +81,7 @@ export class ExecutionContext {
         functionText = 'function ' + functionText;
       try {
         new Function('(' + functionText  + ')');
-      } catch (e2) {
+      } catch (error) {
         // We tried hard to serialize, but there's a weird beast here.
         throw new Error('Passed function is not well-serializable!');
       }
@@ -96,10 +96,10 @@ export class ExecutionContext {
         awaitPromise: true,
         userGesture: true
       });
-    } catch (err) {
-      if (err instanceof TypeError && err.message.startsWith('Converting circular structure to JSON'))
-        err.message += ' Are you passing a nested JSHandle?';
-      throw err;
+    } catch (error) {
+      if (error instanceof TypeError && error.message.startsWith('Converting circular structure to JSON'))
+        error.message += ' Are you passing a nested JSHandle?';
+      throw error;
     }
     const {exceptionDetails, result: remoteObject} = await callFunctionOnPromise.catch(rewriteError);
     if (exceptionDetails)

diff --git a/src/Launcher.js b/src/Launcher.js
@@ -98,7 +98,7 @@ class BrowserRunner {
         if (this._tempDirectory) {
           removeFolderAsync(this._tempDirectory)
               .then(() => fulfill())
-              .catch(err => console.error(err));
+              .catch(error => console.error(error));
         } else {
           fulfill();
         }
@@ -819,9 +819,9 @@ function getWSEndpoint(browserURL) {
   request.on('error', reject);
   request.end();
 
-  return promise.catch(e => {
-    e.message = `Failed to fetch browser webSocket url from ${endpointURL}: ` + e.message;
-    throw e;
+  return promise.catch(error => {
+    error.message = `Failed to fetch browser webSocket url from ${endpointURL}: ` + error.message;
+    throw error;
   });
 }
 

diff --git a/src/Page.js b/src/Page.js
@@ -179,9 +179,9 @@ class Page extends EventEmitter {
     let callback;
     const promise = new Promise(x => callback = x);
     this._fileChooserInterceptors.add(callback);
-    return helper.waitWithTimeout(promise, 'waiting for file chooser', timeout).catch(e => {
+    return helper.waitWithTimeout(promise, 'waiting for file chooser', timeout).catch(error => {
       this._fileChooserInterceptors.delete(callback);
-      throw e;
+      throw error;
     });
   }
 
@@ -851,10 +851,10 @@ class Page extends EventEmitter {
   async emulateTimezone(timezoneId) {
     try {
       await this._client.send('Emulation.setTimezoneOverride', {timezoneId: timezoneId || ''});
-    } catch (exception) {
-      if (exception.message.includes('Invalid timezone'))
+    } catch (error) {
+      if (error.message.includes('Invalid timezone'))
         throw new Error(`Invalid timezone ID: ${timezoneId}`);
-      throw exception;
+      throw error;
     }
   }
 

diff --git a/src/helper.ts b/src/helper.ts
@@ -90,12 +90,12 @@ function installAsyncStackHooks(classType: AnyClass): void {
         stack: ''
       };
       Error.captureStackTrace(syncStack);
-      return method.call(this, ...args).catch(e => {
+      return method.call(this, ...args).catch(error => {
         const stack = syncStack.stack.substring(syncStack.stack.indexOf('\n') + 1);
         const clientStack = stack.substring(stack.indexOf('\n'));
-        if (e instanceof Error && e.stack && !e.stack.includes(clientStack))
-          e.stack += '\n  -- ASYNC --\n' + stack;
-        throw e;
+        if (error instanceof Error && error.stack && !error.stack.includes(clientStack))
+          error.stack += '\n  -- ASYNC --\n' + stack;
+        throw error;
       });
     });
   }
@@ -149,9 +149,9 @@ async function waitForEvent<T extends any>(emitter: NodeJS.EventEmitter, eventNa
   const result = await Promise.race([promise, abortPromise]).then(r => {
     cleanup();
     return r;
-  }, e => {
+  }, error => {
     cleanup();
-    throw e;
+    throw error;
   });
   if (result instanceof Error)
     throw result;

diff --git a/test/CDPSession.spec.js b/test/CDPSession.spec.js
@@ -72,8 +72,8 @@ describeChromeOnly('Target.createCDPSession', function() {
     let error = null;
     try {
       await client.send('Runtime.evaluate', {expression: '3 + 1', returnByValue: true});
-    } catch (e) {
-      error = e;
+    } catch (error_) {
+      error = error_;
     }
     expect(error.message).toContain('Session closed.');
   });

diff --git a/test/browsercontext.spec.js b/test/browsercontext.spec.js
@@ -26,7 +26,7 @@ describe('BrowserContext', function() {
     const defaultContext = browser.browserContexts()[0];
     expect(defaultContext.isIncognito()).toBe(false);
     let error = null;
-    await defaultContext.close().catch(e => error = e);
+    await defaultContext.close().catch(error_ => error = error_);
     expect(browser.defaultBrowserContext()).toBe(defaultContext);
     expect(error.message).toContain('cannot be closed');
   });
@@ -104,7 +104,7 @@ describe('BrowserContext', function() {
     const {browser, server, puppeteer} = getTestState();
 
     const context = await browser.createIncognitoBrowserContext();
-    const error = await context.waitForTarget(target => target.url() === server.EMPTY_PAGE, {timeout: 1}).catch(e => e);
+    const error = await context.waitForTarget(target => target.url() === server.EMPTY_PAGE, {timeout: 1}).catch(error_ => error_);
     expect(error).toBeInstanceOf(puppeteer.errors.TimeoutError);
     await context.close();
   });

diff --git a/test/chromiumonly.spec.js b/test/chromiumonly.spec.js
@@ -46,7 +46,7 @@ describeChromeOnly('Chromium-Specific Launcher tests', function() {
       const browserURL = 'http://127.0.0.1:21222';
 
       let error = null;
-      await puppeteer.connect({browserURL, browserWSEndpoint: originalBrowser.wsEndpoint()}).catch(e => error = e);
+      await puppeteer.connect({browserURL, browserWSEndpoint: originalBrowser.wsEndpoint()}).catch(error_ => error = error_);
       expect(error.message).toContain('Exactly one of browserWSEndpoint, browserURL or transport');
 
       originalBrowser.close();
@@ -60,7 +60,7 @@ describeChromeOnly('Chromium-Specific Launcher tests', function() {
       const browserURL = 'http://127.0.0.1:32333';
 
       let error = null;
-      await puppeteer.connect({browserURL}).catch(e => error = e);
+      await puppeteer.connect({browserURL}).catch(error_ => error = error_);
       expect(error.message).toContain('Failed to fetch browser webSocket url from');
       originalBrowser.close();
     });

diff --git a/test/click.spec.js b/test/click.spec.js
@@ -69,7 +69,7 @@ describe('Page.click', function() {
     await Promise.all([
       newPage.close(),
       newPage.mouse.click(1, 2),
-    ]).catch(e => {});
+    ]).catch(error => {});
   });
   it('should click the button after navigation ', async() => {
     const {page, server} = getTestState();
@@ -197,7 +197,7 @@ describe('Page.click', function() {
 
     await page.goto(server.PREFIX + '/input/button.html');
     let error = null;
-    await page.click('button.does-not-exist').catch(e => error = e);
+    await page.click('button.does-not-exist').catch(error_ => error = error_);
     expect(error.message).toBe('No node found for selector: button.does-not-exist');
   });
   // @see https://github.com/puppeteer/puppeteer/issues/161

diff --git a/test/cookies.spec.js b/test/cookies.spec.js
@@ -270,8 +270,8 @@ describe('Cookie specs', () => {
       let error = null;
       try {
         await page.setCookie({name: 'example-cookie', value: 'best'});
-      } catch (e) {
-        error = e;
+      } catch (error_) {
+        error = error_;
       }
       expect(error.message).toContain('At least one of the url and domain needs to be specified');
     });
@@ -285,8 +285,8 @@ describe('Cookie specs', () => {
             {name: 'example-cookie', value: 'best'},
             {url: 'about:blank', name: 'example-cookie-blank', value: 'best'}
         );
-      } catch (e) {
-        error = e;
+      } catch (error_) {
+        error = error_;
       }
       expect(error.message).toEqual(
           `Blank page can not have cookie "example-cookie-blank"`
@@ -299,8 +299,8 @@ describe('Cookie specs', () => {
       await page.goto('data:,Hello%2C%20World!');
       try {
         await page.setCookie({name: 'example-cookie', value: 'best'});
-      } catch (e) {
-        error = e;
+      } catch (error_) {
+        error = error_;
       }
       expect(error.message).toContain('At least one of the url and domain needs to be specified');
     });

diff --git a/test/elementhandle.spec.js b/test/elementhandle.spec.js
@@ -183,7 +183,7 @@ describe('ElementHandle specs', function() {
       await page.goto(server.PREFIX + '/input/button.html');
       const buttonTextNode = await page.evaluateHandle(() => document.querySelector('button').firstChild);
       let error = null;
-      await buttonTextNode.click().catch(err => error = err);
+      await buttonTextNode.click().catch(error_ => error = error_);
       expect(error.message).toBe('Node is not of type HTMLElement');
     });
     it('should throw for detached nodes', async() => {
@@ -193,7 +193,7 @@ describe('ElementHandle specs', function() {
       const button = await page.$('button');
       await page.evaluate(button => button.remove(), button);
       let error = null;
-      await button.click().catch(err => error = err);
+      await button.click().catch(error_ => error = error_);
       expect(error.message).toBe('Node is detached from document');
     });
     it('should throw for hidden nodes', async() => {
@@ -202,7 +202,7 @@ describe('ElementHandle specs', function() {
       await page.goto(server.PREFIX + '/input/button.html');
       const button = await page.$('button');
       await page.evaluate(button => button.style.display = 'none', button);
-      const error = await button.click().catch(err => err);
+      const error = await button.click().catch(error_ => error_);
       expect(error.message).toBe('Node is either not visible or not an HTMLElement');
     });
     it('should throw for recursively hidden nodes', async() => {
@@ -211,15 +211,15 @@ describe('ElementHandle specs', function() {
       await page.goto(server.PREFIX + '/input/button.html');
       const button = await page.$('button');
       await page.evaluate(button => button.parentElement.style.display = 'none', button);
-      const error = await button.click().catch(err => err);
+      const error = await button.click().catch(error_ => error_);
       expect(error.message).toBe('Node is either not visible or not an HTMLElement');
     });
     itFailsFirefox('should throw for <br> elements', async() => {
       const {page} = getTestState();
 
       await page.setContent('hello<br>goodbye');
       const br = await page.$('br');
-      const error = await br.click().catch(err => err);
+      const error = await br.click().catch(error_ => error_);
       expect(error.message).toBe('Node is either not visible or not an HTMLElement');
     });
   });

diff --git a/test/emulation.spec.js b/test/emulation.spec.js
@@ -148,7 +148,7 @@ describe('Emulation', () => {
       const {page} = getTestState();
 
       let error = null;
-      await page.emulateMedia('bad').catch(e => error = e);
+      await page.emulateMedia('bad').catch(error_ => error = error_);
       expect(error.message).toBe('Unsupported media type: bad');
     });
   });
@@ -173,7 +173,7 @@ describe('Emulation', () => {
       const {page} = getTestState();
 
       let error = null;
-      await page.emulateMediaType('bad').catch(e => error = e);
+      await page.emulateMediaType('bad').catch(error_ => error = error_);
       expect(error.message).toBe('Unsupported media type: bad');
     });
   });
@@ -213,7 +213,7 @@ describe('Emulation', () => {
       const {page} = getTestState();
 
       let error = null;
-      await page.emulateMediaFeatures([{name: 'bad', value: ''}]).catch(e => error = e);
+      await page.emulateMediaFeatures([{name: 'bad', value: ''}]).catch(error_ => error = error_);
       expect(error.message).toBe('Unsupported media feature: bad');
     });
   });
@@ -242,9 +242,9 @@ describe('Emulation', () => {
       const {page} = getTestState();
 
       let error = null;
-      await page.emulateTimezone('Foo/Bar').catch(e => error = e);
+      await page.emulateTimezone('Foo/Bar').catch(error_ => error = error_);
       expect(error.message).toBe('Invalid timezone ID: Foo/Bar');
-      await page.emulateTimezone('Baz/Qux').catch(e => error = e);
+      await page.emulateTimezone('Baz/Qux').catch(error_ => error = error_);
       expect(error.message).toBe('Invalid timezone ID: Baz/Qux');
     });
   });