doc: add guidance on testing new errors#14207
Closed
+47 −0
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
3 tasks
doc/guides/using-internal-errors.md Outdated
| as we can trust the error helper implementation. An example of this kind of | ||
| error would be: | ||
| ``` |
Contributor
There was a problem hiding this comment.
nit: language labels (js) after code backticks.
Contributor
There was a problem hiding this comment.
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
vsemozhetbyt added errors 
Trott reviewed Jul 13, 2017
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 |
Member
There was a problem hiding this comment.
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.
Member
There was a problem hiding this comment.
Or even (if this is what is meant):
Avoid changing existing messages.
MemberAuthor
There was a problem hiding this comment.
Took first suggested alternative sentence.
cjihrig reviewed Jul 13, 2017
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 |
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 |
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: |
MemberAuthor
mhdawson commented Jul 13, 2017
@vsemozhetbyt@Trott@cjihrig pushed change to address comments. |
vsemozhetbyt approved these changes Jul 13, 2017
cjihrig approved these changes Jul 13, 2017
lpinca reviewed Jul 13, 2017
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 |
lpinca approved these changes Jul 13, 2017
MemberAuthor
mhdawson commented Jul 14, 2017
Contributor
vsemozhetbyt commented Jul 14, 2017
Linter CI is green. |
jasnell approved these changes Jul 14, 2017
Contributor
refack commented Jul 14, 2017
|
MemberAuthor
mhdawson commented Jul 18, 2017
CI was green, will land |
MemberAuthor
mhdawson commented Jul 18, 2017
Landed as 9ab63d6 |
mhdawson added a commit that referenced this pull request Jul 18, 2017
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]>
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
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]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
doc, errors