-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Conversation
doc/guides/using-internal-errors.md
Outdated
as we can trust the error helper implementation. An example of this kind of | ||
error would be: | ||
|
||
``` |
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:
94:1 error Expected indentation of 2 spaces but found 4 indent
95:1 error Expected indentation of 2 spaces but found 4 indent
107:1 error Expected indentation of 0 spaces but found 1 indent
108:1 error Expected indentation of 2 spaces but found 4 indent
109:1 error Expected indentation of 0 spaces but found 2 indent
110:1 error Expected indentation of 2 spaces but found 4 indent
111:1 error Expected indentation of 2 spaces but found 4 indent
112:1 error Expected indentation of 0 spaces but found 2 indent
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.
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.
% lint
doc/guides/using-internal-errors.md
Outdated
``` | ||
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
should avoided
-> should be avoided
Although I'd rewrite this entire sentence as:
Avoid changing the format of the message after the error has been created.
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.
Or even (if this is what is meant):
Avoid changing existing messages.
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.
Took first suggested alternative sentence.
doc/guides/using-internal-errors.md
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the parens in (s)
.
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.
done
doc/guides/using-internal-errors.md
Outdated
If the error message is not a constant string then test(s) to validate | ||
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
done
doc/guides/using-internal-errors.md
Outdated
creating the error should be added to | ||
`.../test/parallel/test-internal-errors.js`. These tests should validate | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
As -> A
would be -> is
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.
done
@vsemozhetbyt @Trott @cjihrig pushed change to address comments. |
doc/guides/using-internal-errors.md
Outdated
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
done
Linter CI is green. |
|
CI was green, will land |
Landed as 9ab63d6 |
PR-URL: #14207 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #14207 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #14207 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
doc, errors