From 322aa8e8a9152fdd6ae0d790b1e720a93eef0615 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Wed, 12 Jul 2017 17:46:17 -0400 Subject: [PATCH 1/3] doc: add guidance on testing new errors --- doc/guides/using-internal-errors.md | 48 +++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/doc/guides/using-internal-errors.md b/doc/guides/using-internal-errors.md index ea011aaa83a5d3..120e5bf1b606fd 100644 --- a/doc/guides/using-internal-errors.md +++ b/doc/guides/using-internal-errors.md @@ -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 +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 +all of the different ways parameters can be used to generate the final +message string. As simple example would be: + +``` +// 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 +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 +to do this for some reason, then additional tests validating the formatting of +the error message for those cases will likely be required. ## API From b657aec7b3249b9ee896f1c05a4b861600ff6b0a Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Thu, 13 Jul 2017 12:45:28 -0400 Subject: [PATCH 2/3] squash: address comments --- doc/guides/using-internal-errors.md | 37 ++++++++++++++--------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/doc/guides/using-internal-errors.md b/doc/guides/using-internal-errors.md index 120e5bf1b606fd..4ea4082f398232 100644 --- a/doc/guides/using-internal-errors.md +++ b/doc/guides/using-internal-errors.md @@ -77,22 +77,22 @@ 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: -``` +```js E('ERR_SOCKET_ALREADY_BOUND', 'Socket is already bound'); ``` -If the error message is not a constant string then test(s) to validate +If the error message is not a constant string then tests 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 +`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: +message string. A simple example is: -``` +```js // 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'); + 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 @@ -103,20 +103,19 @@ required when the message for an error changes. For example: -``` - assert.throws(() => { - socket.bind(); - }, common.expectsError({ - code: 'ERR_SOCKET_ALREADY_BOUND', - type: Error - })); - +```js +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 -to do this for some reason, then additional tests validating the formatting of -the error message for those cases will likely be required. +Avoid changing the format of the message after the error has been created. +If it does make sense to do this for some reason, then additional tests +validating the formatting of the error message for those cases will +likely be required. ## API From 48543e0ba5f585bd8cdbf4ce812035c22229a628 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Thu, 13 Jul 2017 18:08:15 -0400 Subject: [PATCH 3/3] squash: address comments --- doc/guides/using-internal-errors.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/guides/using-internal-errors.md b/doc/guides/using-internal-errors.md index 4ea4082f398232..3549a0da6bf0ac 100644 --- a/doc/guides/using-internal-errors.md +++ b/doc/guides/using-internal-errors.md @@ -98,7 +98,7 @@ assert.strictEqual( 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 +and NOT validate the message. This will reduce the amount of test change required when the message for an error changes. For example: