Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jun 22, 2018

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:

  1. all natives (js sources),
  2. doc/api/*.md documentation
  3. 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:

  1. 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.
  2. 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.
  3. 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.
  4. All errors mentioned anywhere should be documented.
  5. 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.
  6. 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

@ChALkeR 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 nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 22, 2018
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 22, 2018

/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
Copy link
Member

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks @joyeecheung!

@jasnell
Copy link
Member

jasnell commented Jun 22, 2018

Great idea :-)

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 22, 2018

@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
  • e9358affalse 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
  • 8ca9338false 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 ChALkeR force-pushed the test-error-codes-impl-doc branch from 927305c to 0bc333a Compare June 23, 2018 08:16
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 23, 2018

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 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
Copy link
Member Author

ChALkeR commented Jun 23, 2018

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 ChALkeR force-pushed the test-error-codes-impl-doc branch from 3f639f2 to d2094e8 Compare June 23, 2018 10:02
@ChALkeR ChALkeR mentioned this pull request Jun 23, 2018
4 tasks
@ChALkeR ChALkeR force-pushed the test-error-codes-impl-doc branch from d2094e8 to ee09862 Compare June 23, 2018 11:25
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 23, 2018

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

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

@ChALkeR ChALkeR force-pushed the test-error-codes-impl-doc branch 2 times, most recently from bbc374f to 69d2df3 Compare June 23, 2018 14:04
targos pushed a commit to ChALkeR/io.js that referenced this pull request Jun 30, 2018
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
targos pushed a commit that referenced this pull request Jun 30, 2018
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>
targos pushed a commit that referenced this pull request Jul 3, 2018
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>
ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Jul 10, 2018
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>
targos pushed a commit that referenced this pull request Jul 12, 2018
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 ChALkeR mentioned this pull request Jul 23, 2018
4 tasks
@ChALkeR ChALkeR force-pushed the test-error-codes-impl-doc branch from d731824 to cf25ca2 Compare July 23, 2018 16:32
@ChALkeR ChALkeR force-pushed the test-error-codes-impl-doc branch from cf25ca2 to ac8b182 Compare August 6, 2018 08:40
@ChALkeR ChALkeR removed the blocked PRs that are blocked by other issues or PRs. label Aug 6, 2018
@ChALkeR
Copy link
Member Author

ChALkeR commented Aug 6, 2018

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
Copy link
Member

Trott commented Aug 6, 2018

@nodejs/testing

kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
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 ChALkeR force-pushed the test-error-codes-impl-doc branch from ac8b182 to 8e41fd7 Compare September 8, 2018 15:09
@ChALkeR
Copy link
Member Author

ChALkeR commented Sep 8, 2018

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catched -> caught

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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]*/, '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: (\(\))??

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const natives = process.binding('natives');

// --report prints only the list of failed checks and does not assert
const report = process.argv[2] === '--report';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
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>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
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>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
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
Copy link
Member

Trott commented Nov 21, 2018

@ChALkeR Still hoping to land this?

@BridgeAR
Copy link
Member

Ping @ChALkeR

@fhinkel
Copy link
Member

fhinkel commented Oct 26, 2019

Closing this due to inactivity. Please reopen if needed.

@fhinkel fhinkel closed this Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants