assert: improve assertion errors - #27781
Conversation
Sorry, something went wrong.
|
Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3574 |
Sorry, something went wrong.
This comment has been minimized.
|
@nodejs/assert @nodejs/testing PTAL. This is actually quite a decent improvement for some cases. |
Sorry, something went wrong.
|
Introduction of any runtime deprecation is semver major and I don't know that this should be an exception? |
Sorry, something went wrong.
|
I agree with @Trott on semver-major. We should be consistent with the policy |
Sorry, something went wrong.
|
I'm not sure I agree with this. If the validation function returns false, then the error should be re-thrown (or an AssertionError thrown). I don't think we want to change that. I think that's the right result. Validation functions should return true or false (and we should probably accept truthy/falsy values too, but I'm less opinionated about that). If the validation function returns false, then the behavior I would expect is an AssertionError to be thrown or the error would be re-thrown. Anything else is surprising and non-ergonomic in my opinion. 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 false is surprising and not very ergonomic IMO. The benefit of enforcing that needs to be enormous. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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`.
|
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 AssertionError that the user returned other than true to give a better context) instead of having a deprecation in this case. @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 |
Sorry, something went wrong.
|
Not opposed to this, but I'm still not sure that re-throwing the error is all that undesirable? Pondering some more... |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
Resumed build: https://ci.nodejs.org/job/node-test-commit/29385/ |
Sorry, something went wrong.
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 <rtrott@gmail.com>
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 <rtrott@gmail.com>
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 <rtrott@gmail.com>
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 <rtrott@gmail.com>
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 <rtrott@gmail.com>
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 <rtrott@gmail.com>
|
@BridgeAR, should this land on v10.x? Please add the lts-watch label if so |
Sorry, something went wrong.
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() and assert.rejects(). It also improves the error message for simple assertions when used in combination with new Function and adds a deprecation warning for validation functions that return anything besides true.
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