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

.toThrowError() #1301

Merged
merged 1 commit into from
Aug 17, 2016
Merged

.toThrowError() #1301

merged 1 commit into from
Aug 17, 2016

Conversation

aaronabramov
Copy link
Contributor

No description provided.

describe('.toThrowError()', () => {
describe('strings', () => {
it('passes', () => {
expect(() => { throw new Error('lol'); }).toThrowError('lol');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lol and lmao are pretty awkward in tests. Can we fall back to banana and apple like jest-haste-map does? :)

@cpojer
Copy link
Member

cpojer commented Jul 19, 2016

Looks good except for a few minor nits and those clowny strings in your tests :P

@@ -78,9 +84,27 @@ const addMatchers = (matchersObj: MatchersObject): void => {
Object.assign(global[GLOBAL_MATCHERS_OBJECT_SYMBOL], matchersObj);
};

const _validateResult = result => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using an arrow function here also causes an unnecassary .bind(this) call. It'd be more efficient to use a normal function here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true. i'd be interested in benchmarking this stuff. I don't think it's going to affect the overall performance that much though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There aren't any unnecessary bind calls because this is run in node 4 without compiling arrow functions to functions. Also babel doesn't bind if there is no this access iirc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:themoreyouknow: I benchmarked the arrow vs regular function in JavaScriptCore and to my surprise, arrow functions are natively faster.

normal x 53,987,644 ops/sec ±3.50% (44 runs sampled)
arrow x 57,679,587 ops/sec ±3.08% (58 runs sampled)

@@ -75,6 +90,9 @@ const stringify = (obj: any): string => {
});
};

// highlight an object
const h = (obj: any) => chalk.cyan.bold(stringify(obj));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this highlight? Then we don't need the comment and people will understand what h means.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants