test: add test for error codes doc and impl #21470

ChALkeR · 2018-06-22T17:39:08Z

This adds several sanity checks for error codes, ref: #21440.
An early version of this test was used to find #21440 (and is something I came up after seeing #21421).

It scans:

all natives (js sources),
doc/api/*.md documentation
src/node_errors.h (errors definition from the C++ side).

There is also a whitelist of manually created errors from JS side, currently consisting of ERR_HTTP2_ERROR and ERR_UNKNOWN_BUILTIN_MODULE. That could be improved, I guess.

The performed checks:

All errors used from JS should be defined in internal/errors and present in its .codes object.
Whitelist with two exceptions of manually created errors (listed above) applies.
All errors instantiated from JS without arguments should support 0-arguments version.
As in new ERR_SOMETHING(), ref: fs: fsPromises.lchmod throws AssertionError on Ubuntu #21421.
All errors mentioned in doc should defined either in JS (internal/errors), C++ (src/node_errors.h), or in the whitelist with two manually created errors.
All errors mentioned anywhere should be documented.
Documentation of error codes should be sorted, have no repeats, and include exactly one entry formatted as /\n### (ERR_[A-Z0-9_]+)\n for every error code mentioned in the documentation.
All doc entries for error codes should have appropriate anchors.

The checks are a bit hacky (and scan the source code as text), but they work and I would prefer keeping it simple as we have a common markup for all those.

There is also a --report flag, which prints all the current issues and exits without asserting, for manual inspection.

Current output on v10.5.0:

$ node --expose-internals test/parallel/test-error-codes-impl-doc.js --report
Report mode
js: ERR_TLS_RENEGOTIATE - missing JS implementation (source)
js: ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK - missing JS implementation (source)
js: ERR_HTTP2_SETTINGS_CANCEL - missing JS implementation (source)
init: ERR_METHOD_NOT_IMPLEMENTED - failed 0 args init, but is called with "()"
impl: ERR_STREAM_READ_NOT_IMPLEMENTED - missing implementation, documented
impl: ERR_VALUE_OUT_OF_RANGE - missing implementation, documented
impl: ERR_HTTP2_STREAM_CLOSED - missing implementation, mentioned in doc/api/
doc: ERR_UNKNOWN_BUILTIN_MODULE - missing documentation
doc: ERR_TLS_RENEGOTIATE - missing documentation
doc: ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK - missing documentation
doc: ERR_HTTP2_SETTINGS_CANCEL - missing documentation
doc: ERR_HTTP2_ERROR - missing documentation
doc: ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER - missing documentation
doc: ERR_HTTP2_STREAM_CLOSED - missing documentation
doc: ERR_HTTP2_HEADERS_AFTER_RESPOND - is out of order
doc: ERR_STREAM_DESTROYED - is out of order
doc: ERR_TLS_RENEGOTIATION_DISABLED - is out of order
There were 17 problems found

I believe that all those are actual problems that need to be fixed.

This test currently fails and is blocked on #21440, i.e. fixing all those errors.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2018-06-22T17:39:09Z

@ChALkeR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/124/pipeline

ChALkeR · 2018-06-22T17:46:32Z

/cc @nodejs/tsc this is a bit hacky test, so I would be happy with some extra feedback on that.
I think it's valuable though (as it found some problems) and imo it should be relatively stable.

joyeecheung · 2018-06-22T19:09:30Z

I like the idea, a few notes:

All errors mentioned in doc (doc/api/errors.md) should defined either in JS (internal/errors), C++ (src/node_errors.h), or in the whitelist with two manually created errors.

This does not apply to errors that are removed (those are the missing implementation ones). We did remove errors and merge them, but the documentation for those error codes need to stay forever.

joyeecheung · 2018-06-22T19:06:19Z

test/parallel/test-error-codes-impl-doc.js

+// File containing error definitions
+const jsdata = natives['internal/errors'];
+// File containing error documentation
+const docdata = fs.readFileSync('doc/api/errors.md', 'utf-8');


Can you make this an absolute path so that it can be run somewhere other than the project directory?

Fixed, thanks @joyeecheung!

jasnell · 2018-06-22T20:53:43Z

Great idea :-)

ChALkeR · 2018-06-22T22:50:27Z

@joyeecheung @jasnell

We did remove errors and merge them, but the documentation for those error codes need to stay forever.

But unused errors were removed multiple times from documentation.
Is that process documented somewhere?

E.g.:

186857f — removes ERR_INVALID_ARRAY_LENGTH
564048d — removes ERR_INVALID_DOMAIN_NAME
3626944 — renames ERR_STRING_TOO_LARGE
301f6cc — removes ERR_FS_WATCHER_ALREADY_STARTED and ERR_FS_WATCHER_NOT_STARTED
5e3f516 — removes/renames ERR_ZLIB_BINDING_CLOSED
6e1c25c — removes ERR_HTTP_INVALID_CHAR, ERR_HTTP2_ALREADY_SHUTDOWN, ERR_HTTP2_FRAME_ERROR, ERR_HTTP2_HEADER_REQUIRED, ERR_HTTP2_HEADERS_OBJECT, ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND, ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK, ERR_NAPI_CONS_PROTOTYPE_OBJECT, ERR_PARSE_HISTORY_DATA, ERR_TLS_RENEGOTIATION_FAILED
a82b1b7 — removes ERR_OUTOFMEMORY
0babd18 — removes/renames ERR_HTTP2_STREAM_CLOSED
~~e9358af~~ — false positive, this removes a duplicate ERR_VALUE_OUT_OF_RANGE entry
1cdb41f — removes ERR_HTTP2_ERROR, ERR_UNKNOWN_BUILTIN_MODULE
(ERR_HTTP2_HEADER_SINGLE_VALUE is just moved around)
Upd: revert in doc: restore documentation for two error codes #21484.
e6b69b9 — removes ERR_INVALID_REPL_HISTORY
1b54371 — renames ERR_STREAM_HAS_STRINGDECODER
~~8ca9338~~ — false positive, ERR_NO_CRYPTO entry is just moved

Were all those done in strictly before those errors got into some releases? I expect not.
Do we want to restore those?

ChALkeR · 2018-06-23T08:19:01Z

I added a doc sorting/formatting check (see above, I updated the post), this is what it currently prints:

doc: ERR_HTTP2_HEADERS_AFTER_RESPOND - is out of order
doc: ERR_STREAM_DESTROYED - is out of order
doc: ERR_TLS_RENEGOTIATION_DISABLED - is out of order

I think it makes sense to sort the error codes in docs, as the current order looks like:

…
ERR_STREAM_WRITE_AFTER_END
ERR_SYSTEM_ERROR
ERR_STREAM_DESTROYED
…

Which is not ideal.

Also, this prevents future duplicate errors like what e9358af fixed — we had duplicate entries at some point.

ChALkeR · 2018-06-23T08:37:14Z

One more check added: all doc entries should have appropriate anchors.
ERR_SCRIPT_EXECUTION_TIMEOUT doesn't:

doc: ERR_SCRIPT_EXECUTION_TIMEOUT - missing anchor or not properly formatted

ChALkeR · 2018-06-23T11:27:54Z

Updated with a scan of all doc/api/*.md files, found this:

impl: ERR_HTTP2_STREAM_CLOSED - missing implementation, mentioned in doc/api/

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR nodejs#16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR nodejs#18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: nodejs#21440 Refs: nodejs#21470 Refs: nodejs#16874 Refs: nodejs#18857

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR #16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR #18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: #21440 Refs: #21470 Refs: #16874 Refs: #18857 PR-URL: #21493 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit 0babd18 (pull request nodejs#17406), and the testcase for http2stream.pushStream was changed accordingly, but the documentation change was overlooked. This commit fixes it and aligns the documentation with the testcase. This is a part of the fixes hinted by nodejs#21470, which includes some tests for error codes usage and documentation and enforces a stricter format. Refs: nodejs#21470 Refs: nodejs#17406 PR-URL: nodejs#21487 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit 0babd18 (pull request #17406), and the testcase for http2stream.pushStream was changed accordingly, but the documentation change was overlooked. This commit fixes it and aligns the documentation with the testcase. This is a part of the fixes hinted by #21470, which includes some tests for error codes usage and documentation and enforces a stricter format. Refs: #21470 Refs: #17406 PR-URL: #21487 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

ChALkeR · 2018-08-06T08:42:07Z

All the known issues with error codes were resolved (in other PRs), this should pass now.
CI: https://ci.nodejs.org/job/node-test-pull-request/16210/

Trott · 2018-08-06T23:48:32Z

@nodejs/testing

The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit 0babd18 (pull request nodejs#17406), and the testcase for http2stream.pushStream was changed accordingly, but the documentation change was overlooked. This commit fixes it and aligns the documentation with the testcase. This is a part of the fixes hinted by nodejs#21470, which includes some tests for error codes usage and documentation and enforces a stricter format. Refs: nodejs#21470 Refs: nodejs#17406 PR-URL: nodejs#21487 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

This adds several sanity checks for error codes. It scans: * all natives (js sources), * doc/api/*.md documentation * src/node_errors.h (errors definition from the C++ side). There is also a whitelist of manually created errors from JS side, currently consisting of ERR_HTTP2_ERROR and ERR_UNKNOWN_BUILTIN_MODULE. Alsom all ERR_NAPI_ codes are whitelisted, as those are created directly on the cpp side, without declaring them first. The performed checks: 1. All errors used from JS should be defined in `internal/errors` and present in its .codes object. Whitelist (mentioned above) applies. 2. All errors instantiated from JS without arguments should support 0-arguments version. 3. All errors mentioned in doc should defined either in JS, C++, or in the whitelist. 4. All errors mentioned anywhere should be documented. 5. Documentation of error codes should be sorted, have no repeats, and include exactly one entry for every error code mentioned in the documentation, formatted as `/\n### (ERR_[A-Z0-9_]+)\n`. 6. All doc entries for error codes should have appropriate anchors. There is also a --report flag, which prints all the current issues and exits without asserting, for manual inspection. Individual fixes for those issues are landed in separate commits. Refs: nodejs#21421 Refs: nodejs#21440 Refs: nodejs#21483 Refs: nodejs#21484 Refs: nodejs#21485 Refs: nodejs#21487 PR-URL: nodejs#21470

ChALkeR · 2018-09-08T15:10:13Z

Ping everyone again ;-).
I updated this to ignore legacy error codes from the docs.

targos · 2018-09-08T15:21:00Z

test/parallel/test-error-codes-impl-doc.js

+
+// Check that we can initialize errors called without args
+for (const name of set.noargs) {
+  if (!errors[name]) continue; // Already catched that above


catched -> caught

BridgeAR

This is a great test! Just left a few suggestions as I believe this could still be improved and we currently have a few custom eslint rules that test for the order and existence without being fully reliable. Since those rules do not work properly all the time (e.g., they do not detect changed docs right away) it would probably be best to remove those in favor of this test.

BridgeAR · 2018-09-08T15:29:38Z

test/parallel/test-error-codes-impl-doc.js

+for (const file of fs.readdirSync(docdir)) {
+  if (!file.endsWith('.md')) continue;
+  let data = fs.readFileSync(path.join(docdir, file), 'utf-8');
+  if (file === 'errors.md') data = data.replace(/## Legacy [\s\S]*/, '');


Should docdata maybe just be read here? That way the file must only be read once instead of twice.

BridgeAR · 2018-09-08T15:35:59Z

test/parallel/test-error-codes-impl-doc.js

+
+function addSource(source, type) {
+  // eslint-disable-next-line node-core/no-unescaped-regexp-dot
+  const re = /(.)?(ERR_[A-Z0-9_]+)(..)?/g;


If I understand the regexp correct the last part is there to potentially match a JS error called without arguments. If that's the case, could we just change this to: (\(\))??

BridgeAR · 2018-09-08T15:37:52Z

test/parallel/test-error-codes-impl-doc.js

+const report = process.argv[2] === '--report';
+const results = [];
+function check(ok, type, name, reason) {
+  const label = `${type}: ${name} - ${reason}`;


Since the last three argument combined become the label, wouldn't it be best to pass through the label directly?

BridgeAR · 2018-09-08T15:40:40Z

test/parallel/test-error-codes-impl-doc.js

+const natives = process.binding('natives');
+
+// --report prints only the list of failed checks and does not assert
+const report = process.argv[2] === '--report';


I suggest to change that to always asserting but checking all entries first instead of asserting on the first failure.

That way the failures are obvious from the assertion and the user does not have to run the test with this flag (and it's not that obvious that this flag must be used by just taking a glimpse at this).

The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit 0babd18 (pull request nodejs#17406), and the testcase for http2stream.pushStream was changed accordingly, but the documentation change was overlooked. This commit fixes it and aligns the documentation with the testcase. This is a part of the fixes hinted by nodejs#21470, which includes some tests for error codes usage and documentation and enforces a stricter format. Refs: nodejs#21470 Refs: nodejs#17406 PR-URL: nodejs#21487 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit 0babd18 (pull request #17406), and the testcase for http2stream.pushStream was changed accordingly, but the documentation change was overlooked. This commit fixes it and aligns the documentation with the testcase. This is a part of the fixes hinted by #21470, which includes some tests for error codes usage and documentation and enforces a stricter format. Refs: #21470 Refs: #17406 Backport-PR-URL: #22850 PR-URL: #21487 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Trott · 2018-11-21T03:24:40Z

@ChALkeR Still hoping to land this?

BridgeAR · 2019-03-10T01:04:03Z

Ping @ChALkeR

fhinkel · 2019-10-26T23:10:37Z

Closing this due to inactivity. Please reopen if needed.

ChALkeR added test Issues and PRs related to the tests. blocked PRs that are blocked by other issues or PRs. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jun 22, 2018

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 22, 2018

ChALkeR mentioned this pull request Jun 22, 2018

ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined #21440

Closed

joyeecheung reviewed Jun 22, 2018

View reviewed changes

ChALkeR force-pushed the test-error-codes-impl-doc branch from 927305c to 0bc333a Compare June 23, 2018 08:16

ChALkeR force-pushed the test-error-codes-impl-doc branch 3 times, most recently from 98c37fe to 3f639f2 Compare June 23, 2018 08:36

ChALkeR mentioned this pull request Jun 23, 2018

doc: add a missing anchor to error codes #21483

Merged

4 tasks

ChALkeR mentioned this pull request Jun 23, 2018

doc: restore documentation for two error codes #21484

Merged

4 tasks

ChALkeR force-pushed the test-error-codes-impl-doc branch from 3f639f2 to d2094e8 Compare June 23, 2018 10:02

ChALkeR mentioned this pull request Jun 23, 2018

doc: sort error codes in errors.md #21485

Merged

4 tasks

ChALkeR force-pushed the test-error-codes-impl-doc branch from d2094e8 to ee09862 Compare June 23, 2018 11:25

ChALkeR mentioned this pull request Jun 23, 2018

doc: fix http2stream.pushStream error doc #21487

Merged

4 tasks

ChALkeR force-pushed the test-error-codes-impl-doc branch 2 times, most recently from bbc374f to 69d2df3 Compare June 23, 2018 14:04

ChALkeR mentioned this pull request Jul 23, 2018

doc: add missing worker error #21947

Closed

4 tasks

ChALkeR force-pushed the test-error-codes-impl-doc branch from d731824 to cf25ca2 Compare July 23, 2018 16:32

ChALkeR force-pushed the test-error-codes-impl-doc branch from cf25ca2 to ac8b182 Compare August 6, 2018 08:40

ChALkeR removed the blocked PRs that are blocked by other issues or PRs. label Aug 6, 2018

ChALkeR force-pushed the test-error-codes-impl-doc branch from ac8b182 to 8e41fd7 Compare September 8, 2018 15:09

targos approved these changes Sep 8, 2018

View reviewed changes

BridgeAR reviewed Sep 8, 2018

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

fhinkel closed this Oct 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add test for error codes doc and impl #21470

test: add test for error codes doc and impl #21470

ChALkeR commented Jun 22, 2018 •

edited

nodejs-github-bot commented Jun 22, 2018

ChALkeR commented Jun 22, 2018

joyeecheung commented Jun 22, 2018

joyeecheung Jun 22, 2018

ChALkeR Jun 23, 2018

jasnell commented Jun 22, 2018

ChALkeR commented Jun 22, 2018 •

edited

ChALkeR commented Jun 23, 2018 •

edited

ChALkeR commented Jun 23, 2018

ChALkeR commented Jun 23, 2018

ChALkeR commented Aug 6, 2018 •

edited

Trott commented Aug 6, 2018

ChALkeR commented Sep 8, 2018

targos Sep 8, 2018

BridgeAR left a comment

BridgeAR Sep 8, 2018

BridgeAR Sep 8, 2018

BridgeAR Sep 8, 2018

BridgeAR Sep 8, 2018

Trott commented Nov 21, 2018

BridgeAR commented Mar 10, 2019

fhinkel commented Oct 26, 2019

test: add test for error codes doc and impl #21470

test: add test for error codes doc and impl #21470

Conversation

ChALkeR commented Jun 22, 2018 • edited

Checklist

nodejs-github-bot commented Jun 22, 2018

ChALkeR commented Jun 22, 2018

joyeecheung commented Jun 22, 2018

joyeecheung Jun 22, 2018

Choose a reason for hiding this comment

ChALkeR Jun 23, 2018

Choose a reason for hiding this comment

jasnell commented Jun 22, 2018

ChALkeR commented Jun 22, 2018 • edited

ChALkeR commented Jun 23, 2018 • edited

ChALkeR commented Jun 23, 2018

ChALkeR commented Jun 23, 2018

ChALkeR commented Aug 6, 2018 • edited

Trott commented Aug 6, 2018

ChALkeR commented Sep 8, 2018

targos Sep 8, 2018

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Sep 8, 2018

Choose a reason for hiding this comment

BridgeAR Sep 8, 2018

Choose a reason for hiding this comment

BridgeAR Sep 8, 2018

Choose a reason for hiding this comment

BridgeAR Sep 8, 2018

Choose a reason for hiding this comment

Trott commented Nov 21, 2018

BridgeAR commented Mar 10, 2019

fhinkel commented Oct 26, 2019

ChALkeR commented Jun 22, 2018 •

edited

ChALkeR commented Jun 22, 2018 •

edited

ChALkeR commented Jun 23, 2018 •

edited

ChALkeR commented Aug 6, 2018 •

edited