Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

assert: put info in err.message, not err.name #5293

Closed
wants to merge 1 commit into from
Closed

assert: put info in err.message, not err.name #5293

wants to merge 1 commit into from

Conversation

hackedy
Copy link

@hackedy hackedy commented Apr 14, 2013

(fixes #5292)

4716dc6 made assert.equal() and related functions work better by generating a better toString() from the expected, actual, and operator values passed to fail(). Unfortunately, this was accomplished by putting the generated message into the error's name property. When you passed in a custom error message, the error would put the custom error into name and message, resulting in helpful string representations like "AssertionError: Oh no: Oh no".

This commit resolves that issue by storing the generated message in the message property and leaving the error's name alone.

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Ryan Doenges

Please see CONTRIBUTING.md for more information

@isaacs
Copy link

isaacs commented Apr 14, 2013

@hackedy Did you sign the cla already? If not, can you go fill this out and click the button that makes the lawyer types happy? http://nodejs.org/cla.html

@hackedy
Copy link
Author

hackedy commented Apr 14, 2013

I'm 17. Can I legally sign it? 😕

@isaacs
Copy link

isaacs commented Apr 14, 2013

Also, it'd be nice if this had a new test. Just add something that catches an assertion error and verifies that the first line of the stack is correct for assert.equal(1, 2) and assert.equal(1, 2, 'oh no'). You can add it to test/simple/test-assert.js.

Otherwise, LGTM, and seems to do the right thing.

@isaacs
Copy link

isaacs commented Apr 14, 2013

I'm not sure, I'm not a lawyer. Maybe have your legal guardian sign it for you, and then sign it yourself next year? (Great to see people getting started with open source so young, though, that's fantastic! Keep it up!)

@hackedy
Copy link
Author

hackedy commented Apr 14, 2013

got my dad to sign it, put an explanation in the "corporate contributions" box \o/

@TooTallNate
Copy link

+1 for this btw. I bumped into this bug yesterday.

@hackedy
Copy link
Author

hackedy commented Apr 14, 2013

Tests added.

@mscdex
Copy link

mscdex commented Apr 16, 2013

+9001

4716dc6 made assert.equal() and related functions work better by
generating a better toString() from the expected, actual, and operator
values passed to fail(). Unfortunately, this was accomplished by putting
the generated message into the error's `name` property. When you passed
in a custom error message, the error would put the custom error into
`name` *and* `message`, resulting in helpful string representations like
"AssertionError: Oh no: Oh no".

This commit resolves that issue by storing the generated message in the
`message` property while leaving the error's name alone and adding
a regression test so that this doesn't pop back up later.

Closes #5292.
@hackedy
Copy link
Author

hackedy commented Apr 16, 2013

rebased and merge-able @isaacs @bnoordhuis

@isaacs
Copy link

isaacs commented Apr 18, 2013

Landed (in v0.10) on 6101eb1. Thanks!

@isaacs isaacs closed this Apr 18, 2013
@hackedy
Copy link
Author

hackedy commented Apr 18, 2013

🎉

@hackedy hackedy deleted the gh_5292 branch April 18, 2013 22:41
@brianc
Copy link

brianc commented Apr 25, 2013

💃 go go go @hackedy

gibfahn added a commit to ibmruntimes/node that referenced this pull request Mar 23, 2016
Original commit: d7b81b5

Float v8 patch, which has been committed to v8 master and
backported to 4.8 and 4.9 in google repos, onto 4.8 v8 in
deps to resolve nodejs#5089

Original title/commit from google repos for 4.8 is:
 PPC: [turbofan] Support for CPU models lacking isel.
 v8/v8@2e4da65

PR-URL: nodejs#5293
Fixes: nodejs#5089
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants