http_client, errors: migrate to use internal/errors by starkwang · Pull Request #14423 · nodejs/node

starkwang · 2017-07-22T11:44:44Z

Checklist

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

Affected core subsystem(s)

errors, http_client

refack · 2017-07-22T14:29:00Z

@starkwang thank you so much for helping with this!

We recently updated the recommended way to test internal Errors — https://github.com/nodejs/node/pull/14207/files
tl;dr need to test the message only if it's variable, and once is enough (in test/parallel/test-internal-errors.js).
IMHO the PR is good as is, but it would be great if you followed up and made it even better.

/cc @jasnell @fhinkel @Trott @mhdawson

refack · 2017-07-22T14:29:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9297/

refack · 2017-07-22T14:34:36Z

lib/internal/errors.js

FYI you could just use %s place holders, but IMHO this way is more readable.

But: consistency 😉

There is none anyway...
I'll open a PR to change them all.

tniessen · 2017-07-22T15:57:39Z

Whoever lands this: I think the subsystem should be http_client with an underscore.

starkwang · 2017-07-22T16:33:25Z

@refack I've added some tests for the new errors in test/parallel/test-internal-errors.js😀

Trott · 2017-07-22T22:13:01Z

semver-major so pinging @nodejs/ctc for some reviews

refack

💯

mcollina

LGTM

addaleax · 2017-07-26T18:18:22Z

@starkwang Sorry to be a bummer, but can you rebase this against master? There are some merge conflicts here…

starkwang · 2017-07-27T02:22:41Z

doc/api/errors.md

It seems that I missing some titles for errors.md in #14212 . So I added the missing titles in this PR

starkwang · 2017-07-27T02:22:50Z

doc/api/errors.md

It seems that I missing some titles for errors.md in #14212 . So I added the missing titles in this PR

starkwang · 2017-07-27T02:24:05Z

@addaleax I've just rebased the branch : )

addaleax · 2017-07-27T10:41:12Z

Fresh CI: https://ci.nodejs.org/job/node-test-commit/11395/

tniessen

From using-internal-errors.md:

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 all of the different ways parameters can be used to generate the final message string.

And this section:

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

I don't exactly care about this, just remember that this PR does not strictly comply with these guidelines.

Thanks for your efforts @starkwang! 😃

tniessen · 2017-07-27T11:03:18Z

test/parallel/test-http-client-check-http-token.js

Unless I am missing something, it does not appear to be necessary to use a regular expression here, a string literal should work just fine. By the way, if you want to match an exact string using regular expressions, use start (^) and end markers ($). Otherwise, the match will succeed even if there is content before or after the expression:

const expr = /The "method" argument must be of type string/; const str = 'The "method" argument must be of type string. Also, this sentence should not be here'; expr.test(str) === true

tniessen · 2017-07-27T11:03:49Z

test/parallel/test-http-hostname-typechecking.js

Same here, why is this a regular expression?

tniessen · 2017-07-27T11:04:00Z

test/parallel/test-http-hostname-typechecking.js

tniessen · 2017-07-28T10:50:26Z

doc/api/errors.md

Grammar: have been sent. Also, http probably refers to the protocol instead of the module name, so I would prefer HTTP over http.

Used when new HTTP headers are added to a response, after the headers part has already been sent.

tniessen · 2017-07-28T10:52:09Z

doc/api/errors.md

http probably refers to the protocol instead of the module name, so I would prefer HTTP over http.

tniessen · 2017-07-28T10:53:03Z

doc/api/errors.md

Grammar: that be passed. I would just say "... failed to parse the host or hostname option".

Used when `hostname` can not be parsed from a provided URL.

tniessen · 2017-07-28T10:54:13Z

doc/api/errors.md

Grammar: when receive. Should probably be when receiving.

Used when a string that contains unescaped characters was received.

tniessen · 2017-07-28T11:00:38Z

lib/_http_client.js

I don't have a strong opinion on this, I would prefer a more meaningful name for this error code. From the error code, the actual reason of the error is unclear, and the message implies that having the headers sent already is the only reason sending headers can fail, ever, and I am not too sure about that. Maybe something like ERR_HEADERS_SENT? Doesn't sound perfect either... cc @refack

ERR_HTTP_HEADERS_SENT

refack · 2017-07-29T21:01:22Z

IMHO the documentation improvements should be non blockers, since simply migrating the Errors is of higher priority. But it's definatly nice to have while we wait for another @nodejs/ctc approval.

Trott · 2017-07-29T21:04:16Z

This PR has http, internal errors, and is semver major. Seems like @jasnell should be all over it. Ping! :-D

Ref: nodejs#11273

starkwang · 2017-07-30T13:41:55Z

Pushed commit to address comments

refack

💯

refack · 2017-08-01T11:26:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9433/

tniessen · 2017-08-01T12:28:22Z

Landed in bdfbce9.

PR-URL: #14423 Refs: #11273 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This error code originally landed in a semver-major commit and is used by the ESM implementation. This backport includes the error message and the documentation for the error. I did attempt to write a test for this, but it did not seem possible to catch an exception during import, I was also unable to execute `node --experimental-modules` properly inside of a child_process. I'll dig more into getting a test together, but we should backport this fix in the mean time. Refs: nodejs#14423 Fixes: nodejs#15374

This error code originally landed in a semver-major commit and is used by the ESM implementation. This backport includes the error message and the documentation for the error. I did attempt to write a test for this, but it did not seem possible to catch an exception during import, I was also unable to execute `node --experimental-modules` properly inside of a child_process. I'll dig more into getting a test together, but we should backport this fix in the mean time. Refs: #14423 Fixes: #15374

This error code originally landed in a semver-major commit and is used by the ESM implementation. This backport includes the error message and the documentation for the error. I did attempt to write a test for this, but it did not seem possible to catch an exception during import, I was also unable to execute `node --experimental-modules` properly inside of a child_process. I'll dig more into getting a test together, but we should backport this fix in the mean time. Refs: #14423 Fixes: #15374 PR-URL: #15388 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. labels Jul 22, 2017

refack approved these changes Jul 22, 2017

View reviewed changes

jasnell mentioned this pull request Jul 22, 2017

Tracking Issue: Migrate errors to internal/errors.js #11273

Closed

80 tasks

refack reviewed Jul 22, 2017

View reviewed changes

starkwang force-pushed the http-client-internal-errors branch from 6031cca to 4ad89b7 Compare July 22, 2017 16:29

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 22, 2017

refack approved these changes Jul 22, 2017

View reviewed changes

mcollina approved these changes Jul 23, 2017

View reviewed changes

starkwang force-pushed the http-client-internal-errors branch 2 times, most recently from c54858b to 4c62748 Compare July 27, 2017 02:20

starkwang commented Jul 27, 2017

View reviewed changes

tniessen reviewed Jul 27, 2017

View reviewed changes

starkwang force-pushed the http-client-internal-errors branch 3 times, most recently from 58fd145 to d4c91a7 Compare July 28, 2017 02:42

starkwang changed the title ~~http client, errors: migrate to use internal/errors~~ http_client, errors: migrate to use internal/errors Jul 28, 2017

tniessen reviewed Jul 28, 2017

View reviewed changes

http_client, errors: migrate to use internal/errors

db8fb2a

Ref: nodejs#11273

starkwang force-pushed the http-client-internal-errors branch from d4c91a7 to db8fb2a Compare July 30, 2017 13:41

refack approved these changes Jul 30, 2017

View reviewed changes

tniessen approved these changes Jul 30, 2017

View reviewed changes

jasnell approved these changes Jul 31, 2017

View reviewed changes

tniessen closed this Aug 1, 2017

MylesBorins mentioned this pull request Sep 13, 2017

errors: backport ERR_INVALID_PROTOCOL to v8.x #15388

Closed

Trott mentioned this pull request Oct 4, 2017

test: fix assert.throws in test-http-invalid-urls #15678

Closed

2 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Uh oh!

Conversation

starkwang commented Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Jul 22, 2017

Uh oh!

refack commented Jul 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen commented Jul 22, 2017

Uh oh!

starkwang commented Jul 22, 2017

Uh oh!

Trott commented Jul 22, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starkwang commented Jul 27, 2017

Uh oh!

addaleax commented Jul 27, 2017

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Jul 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 29, 2017

Uh oh!

Trott commented Jul 29, 2017

Uh oh!

starkwang commented Jul 30, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 1, 2017

Uh oh!

tniessen commented Aug 1, 2017

starkwang commented Jul 22, 2017 •

edited

Loading

refack Jul 29, 2017 •

edited

Loading