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.

Sign up for GitHub

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

Jump to bottom

assert: improve assertion errors #27781

Closed
wants to merge 4 commits into from
Closed

assert: improve assertion errors #27781

wants to merge 4 commits into from

Conversation

BridgeAR
Copy link
Member

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
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label May 20, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 20, 2019 •
edited
Loading

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3574
CI: https://ci.nodejs.org/job/node-test-pull-request/23807/
CI: https://ci.nodejs.org/job/node-test-pull-request/23820/

@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label May 20, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
targos
targos reviewed May 20, 2019
View reviewed changes
lib/assert.js Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member Author

@nodejs/assert @nodejs/testing PTAL. This is actually quite a decent improvement for some cases.

@BridgeAR BridgeAR added the review wanted PRs that need reviews. label May 29, 2019
Trott
Trott reviewed May 29, 2019
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
Trott
Trott reviewed May 29, 2019
View reviewed changes
doc/api/assert.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented May 29, 2019

Introduction of any runtime deprecation is semver major and I don't know that this should be an exception?

@jasnell
Copy link
Member

jasnell commented May 29, 2019

I agree with @Trott on semver-major. We should be consistent with the policy

@Trott
Copy link
Member

Trott commented May 29, 2019

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.

@cjihrig
Copy link
Contributor

cjihrig commented May 29, 2019

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.

@BridgeAR
assert: improve regular expression validation
6bf9806 
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.
@BridgeAR
assert: add partial support for evaluated code in simple assert
e36fd00 
This makes sure using `assert.ok()` in `new Function()` statements
visualizes the actual call site in the error message.
@BridgeAR
doc: update assert's validation functions
6dd4ba5 
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
Copy link
Member Author

BridgeAR commented Jun 5, 2019

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

@BridgeAR BridgeAR requested review from Trott, targos and cjihrig June 6, 2019 21:47
Trott
Trott reviewed Jun 6, 2019
View reviewed changes
doc/api/assert.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Jun 6, 2019

Not opposed to this, but I'm still not sure that re-throwing the error is all that undesirable? Pondering some more...

@BridgeAR
Copy link
Member Author

@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 BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 12, 2019
@BridgeAR
Copy link
Member Author

BridgeAR commented Jun 12, 2019 •
edited
Loading

Resumed build: https://ci.nodejs.org/job/node-test-commit/29385/
See #27781 (comment)

Trott pushed a commit to Trott/io.js that referenced this pull request Jun 13, 2019
@BridgeAR @Trott
assert: improve regular expression validation
4a3af65 
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>
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 13, 2019
@BridgeAR @Trott
assert: add partial support for evaluated code in simple assert
1a0fd60 
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>
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 13, 2019
@BridgeAR @Trott
doc: update assert's validation functions
0f90040 
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>
@Trott
Copy link
Member

Trott commented Jun 13, 2019

Landed in 8149656...0f900405e52061136f7b5d69e6f2453309064c8a

@Trott Trott closed this Jun 13, 2019
BridgeAR added a commit that referenced this pull request Jun 17, 2019
@BridgeAR
assert: improve regular expression validation
ef8f147 
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>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
@BridgeAR
assert: add partial support for evaluated code in simple assert
f03241f 
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>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
@BridgeAR
doc: update assert's validation functions
abe5d05 
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 BridgeAR mentioned this pull request Jun 17, 2019
Merged
@BethGriggs
Copy link
Member

@BridgeAR, should this land on v10.x? Please add the lts-watch label if so

@BridgeAR BridgeAR deleted the improve-assertion-errors branch January 20, 2020 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@targos targos Awaiting requested review from targos

@cjihrig cjihrig Awaiting requested review from cjihrig

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @nodejs-github-bot @Trott @jasnell @cjihrig @BethGriggs @targos