assert: improve assertion errors #27781
Conversation
BridgeAR
added
the
semver-minor
nodejs-github-bot
added
the
assert
BridgeAR
force-pushed
the
improve-assertion-errors
branch
from
fcc8010 to
098b057
Compare
This comment has been minimized.
This comment has been minimized.
|
@nodejs/assert @nodejs/testing PTAL. This is actually quite a decent improvement for some cases. |
|
Introduction of any runtime deprecation is semver major and I don't know that this should be an exception? |
|
I agree with @Trott on semver-major. We should be consistent with the policy |
|
I'm not sure I agree with this. If the validation function returns It's more than possible that I either haven't thought about this with sufficient care or else am simply missing the way this change improves things for the average user. But the idea that a validation function shouldn't return |
|
a584c57 seems like an unrelated change that shouldn't be part of the larger PR, and I disagree with it in general because I think it decreases visibility into our tests. I also agree with @Trott's #27781 (comment). I don't think that's a change we should make. |
This makes sure `assert.throws()` and `assert.rejects()` result in an easy to understand error message instead of rethrowing the actual error. This should significantly improve the debugging experience in case people use an regular expression to validate their errors. This also adds support for primitive errors that would have caused runtime errors using the mentioned functions. The input is now stringified before it's passed to the RegExp to circumvent that. As drive-by change this also adds some further comments and renames a variable for clarity.
This makes sure using `assert.ok()` in `new Function()` statements visualizes the actual call site in the error message.
Using `assert.throws()` or `assert.rejects()` in combination with validation function can be very powerful. Using them by returning any value besides `true` makes debugging very difficult though because there's no context or reason why the error validation failed. Thus, it is recommended to throw an error in such cases instead of returning anything besides `true`.
BridgeAR
force-pushed
the
improve-assertion-errors
branch
from
9a85f07 to
6dd4ba5
Compare
|
I just updated the PR and incorporated the feedback. I removed the deprecation completely and I'll open a separate PR that changes the behavior directly to an actually more desirable one (throwing an @cjihrig I also removed the commit that you mentioned. I just added it, since without it we receive a cryptic error message. I guess you would prefer to just throw a more meaningful error in that case instead of just logging it? @nodejs/assert PTAL |
|
Not opposed to this, but I'm still not sure that re-throwing the error is all that undesirable? Pondering some more... |
|
@Trott right now the user loses pretty much all context why this error occurred and there is no reference to any assertion going on at all. This struck me multiple times in tests and it takes more time to figure out what actually happened. By throwing inside of the validation function in case a specific validation fails, that's not a problem at all. That's why I definitely believe we should recommend this. |
BridgeAR
added
the
author ready
|
|
This makes sure `assert.throws()` and `assert.rejects()` result in an easy to understand error message instead of rethrowing the actual error. This should significantly improve the debugging experience in case people use an regular expression to validate their errors. This also adds support for primitive errors that would have caused runtime errors using the mentioned functions. The input is now stringified before it's passed to the RegExp to circumvent that. As drive-by change this also adds some further comments and renames a variable for clarity. PR-URL: nodejs#27781 Reviewed-By: Rich Trott <[email protected]>
This makes sure using `assert.ok()` in `new Function()` statements visualizes the actual call site in the error message. PR-URL: nodejs#27781 Reviewed-By: Rich Trott <[email protected]>
Using `assert.throws()` or `assert.rejects()` in combination with validation function can be very powerful. Using them by returning any value besides `true` makes debugging very difficult though because there's no context or reason why the error validation failed. Thus, it is recommended to throw an error in such cases instead of returning anything besides `true`. PR-URL: nodejs#27781 Reviewed-By: Rich Trott <[email protected]>
|
Landed in 8149656...0f900405e52061136f7b5d69e6f2453309064c8a |
This makes sure `assert.throws()` and `assert.rejects()` result in an easy to understand error message instead of rethrowing the actual error. This should significantly improve the debugging experience in case people use an regular expression to validate their errors. This also adds support for primitive errors that would have caused runtime errors using the mentioned functions. The input is now stringified before it's passed to the RegExp to circumvent that. As drive-by change this also adds some further comments and renames a variable for clarity. PR-URL: #27781 Reviewed-By: Rich Trott <[email protected]>
This makes sure using `assert.ok()` in `new Function()` statements visualizes the actual call site in the error message. PR-URL: #27781 Reviewed-By: Rich Trott <[email protected]>
Using `assert.throws()` or `assert.rejects()` in combination with validation function can be very powerful. Using them by returning any value besides `true` makes debugging very difficult though because there's no context or reason why the error validation failed. Thus, it is recommended to throw an error in such cases instead of returning anything besides `true`. PR-URL: #27781 Reviewed-By: Rich Trott <[email protected]>
|
@BridgeAR, should this land on v10.x? Please add the |
Please have a look at the commit messages for details.
This makes error messages in case of an error are improved while using
assert.throws()andassert.rejects(). It also improves the error message for simple assertions when used in combination withnew Functionand adds a deprecation warning for validation functions that return anything besidestrue.This should IMO be semver-minor, since it'll only impact code that fails the assertion (e.g., while writing new code). No passing assertion call is impacted by this.
@nodejs/assert PTAL
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes