assert: support custom errors #15304
Conversation
nodejs-github-bot
added
the
assert
doc/api/assert.md
Outdated
| property set equal to the value of the `message` parameter. If the `message` | ||
| parameter is undefined, a default error message is assigned. | ||
| property set eq ual to the value of the `message` parameter. If the `message` | ||
| parameter is undefined, a default error message is assigned. f the `message` |
test/parallel/test-assert.js
Outdated
| try { | ||
| assert.strictEqual(1, 2, new RangeError('my range')); | ||
| } catch (e) { | ||
| assert.strictEqual(e.message, 'my range'); |
There was a problem hiding this comment.
If you store the RangeError in a variable, you can just assert that e equals it.
test/parallel/test-assert.js
Outdated
| }, TypeError, new RangeError('my range')); | ||
| } catch (e) { | ||
| assert.ok(e.message.includes('my range')); | ||
| assert.ok(e instanceof assert.AssertionError); |
There was a problem hiding this comment.
For these tests involving try...catch, we don't know if the assertions were actually run. Can you add checks like these
doc/api/assert.md
Outdated
| @@ -411,8 +422,10 @@ assert.notDeepEqual(obj1, obj4); | |||
| ``` | |||
|
|
|||
| If the values are deeply equal, an `AssertionError` is thrown with a `message` | |||
| property set equal to the value of the `message` parameter. If the `message` | |||
| parameter is undefined, a default error message is assigned. | |||
| property set eq ual to the value of the `message` parameter. If the `message` | |||
cjihrig
added
the
semver-major
|
@cjihrig @vsemozhetbyt thanks for the feedback... updated! |
| assert.ok(e instanceof RangeError, 'Incorrect error type thrown'); | ||
| } | ||
| assert.ok(threw); | ||
| threw = false; |
There was a problem hiding this comment.
It would actually be nice to use common.expectsError here. The test would be smaller in that case. You can check some other use cases as a reference.
There was a problem hiding this comment.
Thank you for the suggestion. I tried expectsError, but for this simple case it didn't make the code any cleaner/shorter/simpler. I'm keeping it as is for now.
|
Landed in e13d1df. Thanks! |
This commit adds special handling of Error instances when passed as the message argument to assert functions. With this commit, if an Error is passed as the message, then that Error is thrown instead of an AssertionError. PR-URL: #15304 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
|
I like the concept of this PR, but I think it was rushed.
(I'm not saying it should be reverted, but I do suggest we tweak it a bit before 9.0.0) |
|
@refack, definitely my bad on item 1. I apologize for that. Regarding item 2, that is how the surrounding tests in that file are written, so I don't necessarily think that is wrong. Maybe that could be a good first contribution. I don't really have any comment on item 3 other than the PR being open sufficiently long enough.
Do you want to submit a follow up PR? Again, my apologies for item 1. |
I totally agree there's nothing bad in this PR (on the contrary, I see it's usufulness). |
This commit adds special handling of Error instances when passed as the message argument to assert functions. With this commit, if an Error is passed as the message, then that Error is thrown instead of an AssertionError. PR-URL: nodejs#15304 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
assert - support a way to provide a custom Error type for assertions. This will help make assert more useful for validating types and ranges.