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

assert: throw when block is not a function #308

Closed
wants to merge 1 commit into from
Closed

assert: throw when block is not a function #308

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 12, 2015

Currently, anything passed as the block argument to throws() and doesNotThrow() is interpreted as a function, which can lead to unexpected results. This commit checks the type of block, and throws a TypeError if it is not a function.

We might want to hold off until after 1.0.0, not sure. Closes #275.

R=@bnoordhuis

Currently, anything passed as the block argument to throws()
and doesNotThrow() is interpreted as a function, which can
lead to unexpected results. This commit checks the type of
block, and throws a TypeError if it is not a function.
@bnoordhuis
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 12, 2015

@bnoordhuis should I land this before or after 1.0.0? It's a small behavior change. Also, how do you feel about this somewhat related PR - nodejs/node-v0.x-archive#8734

@bnoordhuis
Copy link
Member

I think this PR can land now, the current behavior is never the desired one. Worst case, it exposes bugs in people's test suites.

nodejs/node-v0.x-archive#8734 looks reasonable at a glance but perhaps needs more careful inspection. I could see it cause regressions when you .deepEqual() on very large objects, for example.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 12, 2015

I could see it cause regressions when you .deepEqual() on very large objects, for example.

Could you elaborate? Does util.inspect() not work with large objects? I assumed the only regressions would occur if you were comparing the AssertionError messages.

@bnoordhuis
Copy link
Member

It works but it can be very slow. I suppose it's only an issue when the assert fails.

cjihrig added a commit that referenced this pull request Jan 12, 2015
Currently, anything passed as the block argument to throws()
and doesNotThrow() is interpreted as a function, which can
lead to unexpected results. This commit checks the type of
block, and throws a TypeError if it is not a function.

Fixes: #275
PR-URL: #308
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 12, 2015

Landed in 14dc917

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants