-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
doc: add guidance on testing new errors #14207
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,54 @@ for the error code should be added to the `doc/api/errors.md` file. This will | |
give users a place to go to easily look up the meaning of individual error | ||
codes. | ||
|
||
## Testing new errors | ||
|
||
When adding a new error, corresponding test(s) for the error message | ||
formatting may also be required. If the messasge for the error is a | ||
constant string then no test is required for the error message formatting | ||
as we can trust the error helper implementation. An example of this kind of | ||
error would be: | ||
|
||
``` | ||
E('ERR_SOCKET_ALREADY_BOUND', 'Socket is already bound'); | ||
``` | ||
|
||
If the error message is not a constant string then test(s) to validate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop the parens in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
the formatting of the message based on the parameters used when | ||
creating the error should be added to | ||
`.../test/parallel/test-internal-errors.js`. These tests should validate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
all of the different ways parameters can be used to generate the final | ||
message string. As simple example would be: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As -> A There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
``` | ||
// Test ERR_TLS_CERT_ALTNAME_INVALID | ||
assert.strictEqual( | ||
errors.message('ERR_TLS_CERT_ALTNAME_INVALID', ['altname']), | ||
'Hostname/IP does not match certificate\'s altnames: altname'); | ||
``` | ||
|
||
In addition, there should also be tests which validate the use of the | ||
error based on where it is used in the codebase. For these tests, except in | ||
special cases, they should only validate that the expected code is received | ||
and NOT validate the massage. This will reduce the amount of test change | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/massage/message/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
required when the message for an error changes. | ||
|
||
For example: | ||
|
||
``` | ||
assert.throws(() => { | ||
socket.bind(); | ||
}, common.expectsError({ | ||
code: 'ERR_SOCKET_ALREADY_BOUND', | ||
type: Error | ||
})); | ||
|
||
``` | ||
|
||
One final note is that it is bad practice to change the format of the message | ||
after the error has been created and should avoided. If it does make sense | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Although I'd rewrite this entire sentence as:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or even (if this is what is meant):
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Took first suggested alternative sentence. |
||
to do this for some reason, then additional tests validating the formatting of | ||
the error message for those cases will likely be required. | ||
|
||
## API | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: language labels (
js
) after code backticks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter errors when the language added:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that.