Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: enforce naming of errors in catch blocks #5763

Merged
merged 1 commit into from Apr 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions .eslintrc.js
Expand Up @@ -9,7 +9,8 @@ module.exports = {

"plugins": [
"mocha",
"@typescript-eslint"
"@typescript-eslint",
"unicorn"
],

"rules": {
Expand Down Expand Up @@ -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": [
{
Expand Down
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -68,6 +68,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",
Expand Down
2 changes: 1 addition & 1 deletion src/BrowserFetcher.ts
Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions src/Coverage.ts
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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);
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/DOMWorld.js
Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down
12 changes: 6 additions & 6 deletions src/ExecutionContext.ts
Expand Up @@ -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 '))
Expand All @@ -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!');
}
Expand All @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions src/Launcher.js
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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;
});
}

Expand Down
10 changes: 5 additions & 5 deletions src/Page.js
Expand Up @@ -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;
});
}

Expand Down Expand Up @@ -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;
}
}

Expand Down
12 changes: 6 additions & 6 deletions src/helper.ts
Expand Up @@ -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;
});
});
}
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions test/CDPSession.spec.js
Expand Up @@ -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.');
});
Expand Down
4 changes: 2 additions & 2 deletions test/browsercontext.spec.js
Expand Up @@ -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');
});
Expand Down Expand Up @@ -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();
});
Expand Down
4 changes: 2 additions & 2 deletions test/chromiumonly.spec.js
Expand Up @@ -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();
Expand All @@ -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();
});
Expand Down
4 changes: 2 additions & 2 deletions test/click.spec.js
Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down
12 changes: 6 additions & 6 deletions test/cookies.spec.js
Expand Up @@ -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');
});
Expand All @@ -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"`
Expand All @@ -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');
});
Expand Down
10 changes: 5 additions & 5 deletions test/elementhandle.spec.js
Expand Up @@ -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() => {
Expand All @@ -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() => {
Expand All @@ -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() => {
Expand All @@ -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');
});
});
Expand Down
10 changes: 5 additions & 5 deletions test/emulation.spec.js
Expand Up @@ -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');
});
});
Expand All @@ -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');
});
});
Expand Down Expand Up @@ -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');
});
});
Expand Down Expand Up @@ -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');
});
});
Expand Down