lib: set properties in error by BridgeAR · Pull Request #26752 · nodejs/node (original) (raw)
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.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Please have a look at the commit messages for a proper description.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes- tests and/or benchmarks are included
- documentation is changed or added
- commit message follows commit guidelines
These tests tested internal functionality in a way that bypassed all code that could reach these cases. Testing code like this code to be covered even though it might actually never be reached from anywhere if public APIs are used. It gives a false feeling of safety that some code works as intended while there is no guarantee that it indeed works as it should.
Therefore it seemed best to remove all of these. The only thing that should be tested is the raw functionality of the internal errors.
This encapsulates the Node.js errors more by adding extra properties to an error inside of the function to create the error message instead of adding the properties at the call site. That simplifies the usage of our errors and makes sure the expected properties are always set.
| @@ -56,7 +57,12 @@ class SystemError extends Error { |
|---|
| if (context.dest !== undefined) |
| message += ` => ${context.dest}`; |
| super(message); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? I'd prefer to have it there.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer it that way as well. The problem is that you may not use this before calling super() and we need the context to be able to attach properties to the error from within the function that constructs the error message.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
BridgeAR added a commit to BridgeAR/node that referenced this pull request
These tests tested internal functionality in a way that bypassed all code that could reach these cases. It gives a false feeling of safety that some code works as intended while there is no guarantee that it indeed works as it should.
Therefore it seemed best to remove all of these. The only thing that should be tested is the raw functionality of the internal errors.
PR-URL: nodejs#26752 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: James M Snell jasnell@gmail.com
BridgeAR added a commit to BridgeAR/node that referenced this pull request
This encapsulates the Node.js errors more by adding extra properties to an error inside of the function to create the error message instead of adding the properties at the call site. That simplifies the usage of our errors and makes sure the expected properties are always set.
PR-URL: nodejs#26752 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: James M Snell jasnell@gmail.com
targos pushed a commit to targos/node that referenced this pull request
These tests tested internal functionality in a way that bypassed all code that could reach these cases. It gives a false feeling of safety that some code works as intended while there is no guarantee that it indeed works as it should.
Therefore it seemed best to remove all of these. The only thing that should be tested is the raw functionality of the internal errors.
PR-URL: nodejs#26752 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: James M Snell jasnell@gmail.com
targos pushed a commit to targos/node that referenced this pull request
This encapsulates the Node.js errors more by adding extra properties to an error inside of the function to create the error message instead of adding the properties at the call site. That simplifies the usage of our errors and makes sure the expected properties are always set.
PR-URL: nodejs#26752 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: James M Snell jasnell@gmail.com
targos pushed a commit that referenced this pull request
These tests tested internal functionality in a way that bypassed all code that could reach these cases. It gives a false feeling of safety that some code works as intended while there is no guarantee that it indeed works as it should.
Therefore it seemed best to remove all of these. The only thing that should be tested is the raw functionality of the internal errors.
PR-URL: #26752 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: James M Snell jasnell@gmail.com
targos pushed a commit that referenced this pull request
This encapsulates the Node.js errors more by adding extra properties to an error inside of the function to create the error message instead of adding the properties at the call site. That simplifies the usage of our errors and makes sure the expected properties are always set.
PR-URL: #26752 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: James M Snell jasnell@gmail.com
Labels
PRs that have at least one approval, no pending requests for changes, and a CI started.
Issues and PRs related to general changes in the lib or src directory.