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.

Sign up for GitHub

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

Jump to bottom

dns, errors: Migrate to use internal/errors #14212

Closed
wants to merge 1 commit into from
Closed

dns, errors: Migrate to use internal/errors #14212

wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Jul 13, 2017 •
edited
Loading

Migrate dns errors to use internal/errors.

Ref: #11273

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)

dns, errors

@nodejs-github-bot nodejs-github-bot added dns Issues and PRs related to the dns subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jul 13, 2017
jasnell
jasnell reviewed Jul 14, 2017
View reviewed changes
doc/api/errors.md Outdated
@@ -580,6 +580,10 @@ by the `assert` module.

Used when attempting to perform an operation outside the bounds of a `Buffer`.

<a id="ERR_CARES_FAILED"></a>
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps ERR_DNS_SET_SERVERS_FAILED?

jasnell
jasnell requested changes Jul 14, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Very close! thank for doing this. Left a comment I'd like to see addressed.

@starkwang starkwang force-pushed the dns-internal-errors branch 2 times, most recently from 1ae02d3 to 56d8131 Compare July 18, 2017 07:59
jasnell
jasnell approved these changes Jul 19, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 19, 2017
XadillaX
XadillaX approved these changes Jul 21, 2017
View reviewed changes
Copy link
Contributor

@XadillaX XadillaX left a comment

Choose a reason for hiding this comment

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

LGTM

@XadillaX
Copy link
Contributor

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

@starkwang
Copy link
Contributor Author

starkwang commented Jul 22, 2017 •
edited
Loading

One failure in CI looks unrelated.
https://ci.nodejs.org/job/node-test-binary-arm/RUN_SUBSET=0,label=pi1-raspbian-wheezy/9392/console

  ...
not ok 23 parallel/test-child-process-fork-exec-path
  ---
  duration_ms: 8.891
  severity: fail
  stack: |-
  ...

refack
refack approved these changes Jul 24, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Some nits

lib/dns.js Outdated
throw new TypeError('Invalid arguments: ' +
'hostname must be a string or falsey');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'hostname',
['string', 'falsey'], hostname);
Copy link
Contributor

Choose a reason for hiding this comment

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

new indentation rule requires aligning to the call (
#14403

lib/dns.js Outdated
@@ -290,13 +292,14 @@ function resolve(hostname, rrtype, callback) {
resolver = resolveMap.A;
callback = rrtype;
} else {
throw new TypeError('"rrtype" argument must be a string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'rrtype',
'string', rrtype);
Copy link
Contributor

Choose a reason for hiding this comment

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

i

lib/internal/errors.js Outdated
@@ -113,6 +113,10 @@ E('ERR_BUFFER_OUT_OF_BOUNDS', bufferOutOfBounds);
E('ERR_CONSOLE_WRITABLE_STREAM',
'Console expects a writable stream instance for %s');
E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s');
E('ERR_DNS_SET_SERVERS_FAILED',
(err, servers) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

probably better (err, servers) => `c-ares failed to set servers: "${err}" [${servers}]`

test/parallel/test-c-ares.js Outdated
// Try calling resolve with an unsupported type that's an object key
'toString'
].forEach((val) => {
assert.throws(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

New feature of expectsError can be used without assert.throws:

common.expectsError(
  dns.resolve('www.google.com', val),
  {
    code: 'ERR_INVALID_OPT_VALUE',
    type: TypeError,
    message: `The value "${val}" is invalid for option "rrtype"`
  }
);

refack
refack reviewed Jul 24, 2017
View reviewed changes
doc/api/errors.md Outdated
@@ -591,6 +591,10 @@ Used when `Console` is instantiated without `stdout` stream or when `stdout` or

Used when the native call from `process.cpuUsage` cannot be processed properly.

<a id="ERR_DNS_SET_SERVERS_FAILED"></a>

Used when `c-ares` failed to set the server.
Copy link
Contributor

@refack refack Jul 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Used when `c-ares` failed to set the DNS servers.

@jasnell jasnell mentioned this pull request Jul 24, 2017
Closed
80 tasks
@refack
Copy link
Contributor

refack commented Jul 24, 2017

@nodejs/ctc @fhinkel @mcollina @mhdawson

@starkwang
Copy link
Contributor Author

Pushed commit to address comments.

mcollina
mcollina approved these changes Jul 24, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@refack
Copy link
Contributor

refack commented Jul 24, 2017

@starkwang thank you.
CI: https://ci.nodejs.org/job/node-test-pull-request/9319/

@refack
Copy link
Contributor

refack commented Jul 24, 2017

Landed in 9cb390d

@refack refack closed this Jul 24, 2017
refack pushed a commit that referenced this pull request Jul 24, 2017
@starkwang @refack
errors: migrate dns to use internal/errors
9cb390d 
PR-URL: #14212
Refs: #11273
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@starkwang starkwang mentioned this pull request Jul 27, 2017
Closed
4 tasks
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@XadillaX XadillaX XadillaX approved these changes

Assignees
No one assigned
Labels
dns Issues and PRs related to the dns subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@starkwang @XadillaX @refack @mcollina @jasnell @nodejs-github-bot