Changed value for hostname to allow for two failing tests to pass. Th… #14781

icarter09 · 2017-08-12T03:31:30Z

…e tests in question were test-net-better-error-messages-port-hostname and test-net-connect-immediate-finish.

Instead of changing the test to permit 'ENOTFOUND' or 'EAI_AGAIN', the value '***' was changed to 'this.hostname.is.invalid'. The change is also reflected in the fact that RFC 2606 reserves the '.invalid' TLD for invalid domain names.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

node test package

Trott · 2017-08-12T03:42:52Z

Hi, @icarter09! Welcome and thanks for doing this!

If it's not too much trouble, it would be great if you could take a moment to format the commit message according to https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines. In particular, the first line of the commit message should be something like test: use reserved invalid hostname for tests. If not, then whoever lands this can fix it up. But if you could do it, it would save them a small bit of work.

Thanks again!

Trott · 2017-08-12T03:43:08Z

/cc @ncopa

Trott · 2017-08-12T03:43:29Z

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

Trott

LGTM if CI is green

Trott · 2017-08-12T03:46:42Z

(Also, if you're editing the commit message, maybe put Refs: https://tools.ietf.org/html/rfc2606#section-2 at the bottom of the commit message on a line by itself. Thanks again!)

ncopa · 2017-08-12T12:30:57Z

I agree that this is better than the ***, and I have verified that it does not break anything on musl libc. 👍
thanks!

icarter09 · 2017-08-12T17:12:01Z

@Trott I can make the changes to the commit message if it hasn't already been changed. As well as add in the extra line you suggested.

icarter09 · 2017-08-12T17:59:46Z

Looks like my rebase to amend the message is trying to bring in other commits.

Let me know if this pull request should be reopened and fixed or a new one created.

gibfahn · 2017-08-12T22:11:38Z

Let me know if this pull request should be reopened and fixed or a new one created.

Reopening and fixing would be ideal. If you have any issues doing it feel free to ask a collaborator (I'm @gibfahn on twitter if you don't use IRC).

Recording the existing commit here just in case something goes wrong:

Patch:

From d0e619382f07947e8a68fa8d80b7fd867170c930 Mon Sep 17 00:00:00 2001
From: icarter09 <icarter1391@gmail.com>
Date: Fri, 11 Aug 2017 23:04:41 -0400
Subject: [PATCH] test: use reserved invalid hostname for tests

---
 test/parallel/test-net-better-error-messages-port-hostname.js | 4 ++--
 test/parallel/test-net-connect-immediate-finish.js            | 7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/test/parallel/test-net-better-error-messages-port-hostname.js b/test/parallel/test-net-better-error-messages-port-hostname.js
index 04974908c9..dac1714167 100644
--- a/test/parallel/test-net-better-error-messages-port-hostname.js
+++ b/test/parallel/test-net-better-error-messages-port-hostname.js
@@ -4,12 +4,12 @@ const net = require('net');
 const assert = require('assert');
 
 // Using port 0 as hostname used is already invalid.
-const c = net.createConnection(0, '***');
+const c = net.createConnection(0, 'this.hostname.is.invalid');
 
 c.on('connect', common.mustNotCall());
 
 c.on('error', common.mustCall(function(e) {
   assert.strictEqual(e.code, 'ENOTFOUND');
   assert.strictEqual(e.port, 0);
-  assert.strictEqual(e.hostname, '***');
+  assert.strictEqual(e.hostname, 'this.hostname.is.invalid');
 }));
diff --git a/test/parallel/test-net-connect-immediate-finish.js b/test/parallel/test-net-connect-immediate-finish.js
index 1006a62d33..1b65ce15ab 100644
--- a/test/parallel/test-net-connect-immediate-finish.js
+++ b/test/parallel/test-net-connect-immediate-finish.js
@@ -24,14 +24,17 @@ const common = require('../common');
 const assert = require('assert');
 const net = require('net');
 
-const client = net.connect({ host: '***', port: common.PORT });
+const client = net.connect({
+  host: 'this.hostname.is.invalid',
+  port: common.PORT
+});
 
 client.once('error', common.mustCall((err) => {
   assert(err);
   assert.strictEqual(err.code, err.errno);
   assert.strictEqual(err.code, 'ENOTFOUND');
   assert.strictEqual(err.host, err.hostname);
-  assert.strictEqual(err.host, '***');
+  assert.strictEqual(err.host, 'this.hostname.is.invalid');
   assert.strictEqual(err.syscall, 'getaddrinfo');
 }));

gibfahn · 2017-08-12T22:14:37Z

@icarter09 out of interest, what was the issue with the ***, was the test failing for you? Agreed that the .invalid makes more sense (and is clearer).

icarter09 · 2017-08-13T00:48:22Z

@gibfahn yes both of those tests were failing for me.

Refs: https://tools.ietf.org/html/rfc2606#section-2

icarter09 · 2017-08-13T01:39:14Z

Message is now fixed. Pull request is straightened out.

Much props to @gibfahn for helping me untangle the mess I created. Learned what not to do and what to do going forward.

Trott · 2017-08-13T01:59:41Z

@gibfahn Just in case it's detail that's helpful, the issue that @icarter09 was having with *** was that it was resulting in EAI_AGAIN instead of ENOTFOUND This was similar to @ncopa's issue with ... (which was the impetus for switching to ***). But host.is.invalid or whatever is much clearer and appears to work everywhere.

tniessen · 2017-08-16T09:45:47Z

Landed in 0309619, welcome aboard! 🎉

PR-URL: #14781 Refs: https://tools.ietf.org/html/rfc2606#section-2 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Refs: nodejs#14781 Refs: https://tools.ietf.org/html/rfc2606#section-2

PR-URL: #14863 Refs: #14781 Refs: https://tools.ietf.org/html/rfc2606#section-2 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

PR-URL: nodejs/node#14781 Refs: https://tools.ietf.org/html/rfc2606#section-2 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

joyeecheung · 2017-08-30T16:52:32Z

Note that if your DNS is hijacked by your current ISP, which redirects requests to unknown hosts to their own server, like this:

▶ ping this.hostname.is.invalid
PING this.hostname.is.invalid (218.205.57.154): 56 data bytes
Request timeout for icmp_seq 0
Request timeout for icmp_seq 1
...

then these tests could fail. This can be addressed by always using a trusted public DNS though (like this, although I am not using Google DNS since it is blocked in China ¯\(ツ)/¯). I wonder if this tip should be documented somewhere...maybe in the guides or test/README.md?

Trott · 2017-09-02T01:28:54Z

then these tests could fail

@joyeecheung Have you seen this test fail that way?

This can be addressed by always using a trusted public DNS

These tests work for me with no network connectivity at all, so another option might be to disconnect from the network.

joyeecheung · 2017-09-02T05:40:05Z

@Trott

Have you seen this test fail that way?

Yes, these are the errors that I saw when I was with a DNS-hijacking ISP:

See errors

=== release test-http-client-req-error-dont-double-fire ===
Path: parallel/test-http-client-req-error-dont-double-fire
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/joyee/projects/node/test/common/index.js:477:10)
    at Object.<anonymous> (/Users/joyee/projects/node/test/parallel/test-http-client-req-error-dont-double-fire.js:10:24)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)
    at Function.Module.runMain (module.js:609:10)
    at startup (bootstrap_node.js:202:16)
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/joyee/projects/node/test/common/index.js:477:10)
    at Object.<anonymous> (/Users/joyee/projects/node/test/parallel/test-http-client-req-error-dont-double-fire.js:14:40)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)
    at Function.Module.runMain (module.js:609:10)
    at startup (bootstrap_node.js:202:16)
Command: out/Release/node /Users/joyee/projects/node/test/parallel/test-http-client-req-error-dont-double-fire.js
=== release test-net-better-error-messages-port-hostname ===
Path: parallel/test-net-better-error-messages-port-hostname
assert.js:41
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'EADDRNOTAVAIL' === 'ENOTFOUND'
    at Socket.<anonymous> (/Users/joyee/projects/node/test/parallel/test-net-better-error-messages-port-hostname.js:12:10)
    at Socket.<anonymous> (/Users/joyee/projects/node/test/common/index.js:509:15)
    at emitOne (events.js:115:13)
    at Socket.emit (events.js:210:7)
    at emitErrorNT (internal/streams/destroy.js:64:8)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)
Command: out/Release/node /Users/joyee/projects/node/test/parallel/test-net-better-error-messages-port-hostname.js
=== release test-net-connect-immediate-finish ===
Path: parallel/test-net-connect-immediate-finish
assert.js:41
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'ETIMEDOUT' === 'ENOTFOUND'
    at Socket.client.once.common.mustCall (/Users/joyee/projects/node/test/parallel/test-net-connect-immediate-finish.js:35:10)
    at Socket.<anonymous> (/Users/joyee/projects/node/test/common/index.js:509:15)
    at Object.onceWrapper (events.js:316:30)
    at emitOne (events.js:115:13)
    at Socket.emit (events.js:210:7)
    at emitErrorNT (internal/streams/destroy.js:64:8)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)
Command: out/Release/node /Users/joyee/projects/node/test/parallel/test-net-connect-immediate-finish.js
[02:00|% 100|+ 1805|-   3]: Done
make: *** [test] Error 1

These tests work for me with no network connectivity at all, so another option might be to disconnect from the network.

Ah, that's a good (and simpler) solution as well.

tniessen · 2017-09-02T09:48:55Z

Note that if your DNS is hijacked by your current ISP, which redirects requests to unknown hosts to their own server,

My ISP did that as well, but luckily, they allowed me to change that setting. this.hostname.is.invalid must never be resolved according to RFC, plus I don't think the ISP's behavior is particularly useful here, so I strongly recommend to disable this "feature" (if possible). You might actually run into problems with this depending on your network setup, e.g. due to DNS caching (also depending on the TTL they use).

PR-URL: #14781 Refs: https://tools.ietf.org/html/rfc2606#section-2 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins · 2017-09-12T01:20:55Z

I just ran into this problem myself due to isp issues where I'm currently staying.

I'm not convinced that the change is worth introducing this weird edge case, that I can say from first hand experience is very odd and not obvious to track down.

I've backed this out of v8.x-staging / v8.5.0-proposal and would like to see this change reverted

Thoughts?

Trott · 2017-09-12T02:50:54Z

@addaleax Do you know whether or not this is a problem that your DNS test double can solve?

tniessen · 2017-09-12T08:59:26Z

We could solve this by using a public DNS server (e.g. the famous 8.8.8.8) in our tests which rely on a proper DNS implementation which some ISPs don't provide.

addaleax · 2017-09-12T09:21:52Z

I don’t think so, because net doesn’t use the c-ares-based methods for dns resolution by default. That’s also why @tniessen’s suggestion wouldn’t work, I think; it would still require overriding the OS’s dns config…

tniessen · 2017-09-12T11:32:24Z

Oh, right, we never implemented a way to use resolvers with the rest of our API...

bnoordhuis · 2017-09-12T11:40:04Z

@tniessen net.connect({ lookup, ...options }) where lookup is a custom resolve function, or do I misunderstand?

tniessen · 2017-09-12T12:18:40Z

@bnoordhuis Yes, we could use that, but the signature of the callback of lookup does not exactly match the resolver API, so we will need to wrap that in another function. Doesn't sound like a pretty solution to me.
IMO having a DNS server configured which resolves non-existent hostnames (especially invalid hostnames) should not be considered a valid test setup.

PR-URL: #14781 Refs: https://tools.ietf.org/html/rfc2606#section-2 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 12, 2017

Trott approved these changes Aug 12, 2017

View reviewed changes

mscdex added the net Issues and PRs related to the net subsystem. label Aug 12, 2017

icarter09 closed this Aug 12, 2017

icarter09 reopened this Aug 12, 2017

test: use reserved invalid hostname for tests

3338455

Refs: https://tools.ietf.org/html/rfc2606#section-2

gibfahn approved these changes Aug 13, 2017

View reviewed changes

jasnell approved these changes Aug 13, 2017

View reviewed changes

tniessen approved these changes Aug 16, 2017

View reviewed changes

tniessen closed this Aug 16, 2017

tniessen added a commit to tniessen/node that referenced this pull request Aug 16, 2017

test: use invalid host according to RFC2606

2b69d13

Refs: nodejs#14781 Refs: https://tools.ietf.org/html/rfc2606#section-2

tniessen mentioned this pull request Aug 16, 2017

test: use invalid host according to RFC2606 #14863

Closed

2 tasks

joyeecheung mentioned this pull request Aug 30, 2017

tcp socket localPort option does not work #15084

Closed

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the land-on-v6.x label Oct 17, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

Marsup mentioned this pull request Nov 6, 2017

test: DNS for bare domain fails with fritz.box search domain #9799

Closed

joyeecheung mentioned this pull request Nov 25, 2017

test: mock dns lookup function in parallel tests #17296

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed value for hostname to allow for two failing tests to pass. Th… #14781

Changed value for hostname to allow for two failing tests to pass. Th… #14781

icarter09 commented Aug 12, 2017 •

edited by tniessen

Loading

Trott commented Aug 12, 2017

Trott commented Aug 12, 2017

Trott commented Aug 12, 2017

Trott left a comment

Trott commented Aug 12, 2017

ncopa commented Aug 12, 2017

icarter09 commented Aug 12, 2017

icarter09 commented Aug 12, 2017 •

edited

Loading

gibfahn commented Aug 12, 2017 •

edited

Loading

gibfahn commented Aug 12, 2017

icarter09 commented Aug 13, 2017

icarter09 commented Aug 13, 2017

Trott commented Aug 13, 2017

tniessen commented Aug 16, 2017

joyeecheung commented Aug 30, 2017

Trott commented Sep 2, 2017 •

edited

Loading

joyeecheung commented Sep 2, 2017 •

edited

Loading

tniessen commented Sep 2, 2017

MylesBorins commented Sep 12, 2017

Trott commented Sep 12, 2017

tniessen commented Sep 12, 2017

addaleax commented Sep 12, 2017

tniessen commented Sep 12, 2017

bnoordhuis commented Sep 12, 2017

tniessen commented Sep 12, 2017

Changed value for hostname to allow for two failing tests to pass. Th… #14781

Changed value for hostname to allow for two failing tests to pass. Th… #14781

Conversation

icarter09 commented Aug 12, 2017 • edited by tniessen Loading

Checklist

Affected core subsystem(s)

Trott commented Aug 12, 2017

Trott commented Aug 12, 2017

Trott commented Aug 12, 2017

Trott left a comment

Choose a reason for hiding this comment

Trott commented Aug 12, 2017

ncopa commented Aug 12, 2017

icarter09 commented Aug 12, 2017

icarter09 commented Aug 12, 2017 • edited Loading

gibfahn commented Aug 12, 2017 • edited Loading

gibfahn commented Aug 12, 2017

icarter09 commented Aug 13, 2017

icarter09 commented Aug 13, 2017

Trott commented Aug 13, 2017

tniessen commented Aug 16, 2017

joyeecheung commented Aug 30, 2017

Trott commented Sep 2, 2017 • edited Loading

joyeecheung commented Sep 2, 2017 • edited Loading

tniessen commented Sep 2, 2017

MylesBorins commented Sep 12, 2017

Trott commented Sep 12, 2017

tniessen commented Sep 12, 2017

addaleax commented Sep 12, 2017

tniessen commented Sep 12, 2017

bnoordhuis commented Sep 12, 2017

tniessen commented Sep 12, 2017

icarter09 commented Aug 12, 2017 •

edited by tniessen

Loading

icarter09 commented Aug 12, 2017 •

edited

Loading

gibfahn commented Aug 12, 2017 •

edited

Loading

Trott commented Sep 2, 2017 •

edited

Loading

joyeecheung commented Sep 2, 2017 •

edited

Loading