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: fix localhost error in test-dns-ipv6 #8254

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@gibfahn
Member

gibfahn commented Aug 24, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

If localhost doesn't resolve to ::1 in IPv6, we should skip the test.

This is the same fix as 814b8c3 (#7766)

cc: @Trott

@mscdex mscdex added the dns label Aug 24, 2016

@jasnell

View changes

Show outdated Hide outdated test/internet/test-dns-ipv6.js
common.skip('localhost does not resolve to ::1');
return;
}
throw err;

This comment has been minimized.

@jasnell

jasnell Aug 24, 2016

Member

This should assert instead of throw

@jasnell

jasnell Aug 24, 2016

Member

This should assert instead of throw

This comment has been minimized.

@Trott

Trott Aug 24, 2016

Member

This should assert instead of throw

Why is that? (Not trolling; honest question. If there's an err object, then that's basically an exception, not a test failure per se. Might be system DNS misconfiguration, DNS server unavailability, network problems...)

@Trott

Trott Aug 24, 2016

Member

This should assert instead of throw

Why is that? (Not trolling; honest question. If there's an err object, then that's basically an exception, not a test failure per se. Might be system DNS misconfiguration, DNS server unavailability, network problems...)

This comment has been minimized.

@cjihrig

cjihrig Aug 25, 2016

Contributor

I think this should be assert.ifError().

@cjihrig

cjihrig Aug 25, 2016

Contributor

I think this should be assert.ifError().

This comment has been minimized.

@gibfahn

gibfahn Aug 25, 2016

Member

I'm happy to change it, but if assert.ifError() is just this:

assert.ifError = function(err) { if (err) throw err; };

what is the reason for having the extra layer of indirection?

@gibfahn

gibfahn Aug 25, 2016

Member

I'm happy to change it, but if assert.ifError() is just this:

assert.ifError = function(err) { if (err) throw err; };

what is the reason for having the extra layer of indirection?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 24, 2016

Member

This looks incorrect to me. It will skip these tests if localhost does not resolve to ::1 but none of these tests depend on that and they should run regardless. The exception is the test that's commented out and that test I suppose should be skipped if this lookup fails, perhaps. But that test is already being skipped everywhere. :-(

Is there an issue this is trying to fix? Can you link to that?

Member

Trott commented Aug 24, 2016

This looks incorrect to me. It will skip these tests if localhost does not resolve to ::1 but none of these tests depend on that and they should run regardless. The exception is the test that's commented out and that test I suppose should be skipped if this lookup fails, perhaps. But that test is already being skipped everywhere. :-(

Is there an issue this is trying to fix? Can you link to that?

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 25, 2016

Member

@Trott Yep you're right! I got confused. The problem we were having is that some machines didn't have the ::1 localhost line at all, so the test was failing on L147 with:

# /home/gib/code/node/labels/bvt/node/test/internet/test-dns-ipv6.js:182
#     if (err) throw err;
#              ˆ
#
# Error: getnameinfo ENOTFOUND ::1
#     at errnoException (dns.js:28:10)
#     at GetNameInfoReqWrap.onlookupservice [as oncomplete] (dns.js:180:26)

common.hasIPv6 returns true on the machine.

Obviously you could say that this is a machine issue, but I'm pretty sure this is the default for this machine, so it'd be nice to handle the failure in this test.

Also I agree it's not ideal to skip the entire test if one of the subtests has an issue, but I'm not aware of any mechanism for skipping subtests (in fact I don't think we've implemented subtests in node, although I know npm has them).

Member

gibfahn commented Aug 25, 2016

@Trott Yep you're right! I got confused. The problem we were having is that some machines didn't have the ::1 localhost line at all, so the test was failing on L147 with:

# /home/gib/code/node/labels/bvt/node/test/internet/test-dns-ipv6.js:182
#     if (err) throw err;
#              ˆ
#
# Error: getnameinfo ENOTFOUND ::1
#     at errnoException (dns.js:28:10)
#     at GetNameInfoReqWrap.onlookupservice [as oncomplete] (dns.js:180:26)

common.hasIPv6 returns true on the machine.

Obviously you could say that this is a machine issue, but I'm pretty sure this is the default for this machine, so it'd be nice to handle the failure in this test.

Also I agree it's not ideal to skip the entire test if one of the subtests has an issue, but I'm not aware of any mechanism for skipping subtests (in fact I don't think we've implemented subtests in node, although I know npm has them).

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 25, 2016

Member

Would the solution be to change line 182 from this:

    if (err) throw err;

To this?:

    if (err) {
      if (err.code === 'ENOTFOUND') {
        return done();
      }
      throw err;
    }
Member

Trott commented Aug 25, 2016

Would the solution be to change line 182 from this:

    if (err) throw err;

To this?:

    if (err) {
      if (err.code === 'ENOTFOUND') {
        return done();
      }
      throw err;
    }
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 26, 2016

Member

That does indeed stop the test from failing, but it also basically hides the fact that the test didn't actually pass due to ::1 not being resolved. Presumably this partially defeats the purpose of the having this particular test?

I understand there may not be any better solution.

Member

gibfahn commented Aug 26, 2016

That does indeed stop the test from failing, but it also basically hides the fact that the test didn't actually pass due to ::1 not being resolved. Presumably this partially defeats the purpose of the having this particular test?

I understand there may not be any better solution.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 26, 2016

Member

I think the problem is that the test is faulty. The test is making an assumption about a system that isn't actually true on many default Linux variants as we've seen in other tests and now we're seeing in this one with AIX.

The test amounts to "If you look up the IPv6 resolution for localhost, then you must get ::1." But as we've seen, there are multiple systems where IPv6 can be configured but localhost is not set to ::1 in /etc/hosts or whatever. As much as possible, we want to isolate out system configuration quirks from the tests. That's not always possible, but as much as we can, we should do it.

To that end, I'd propose that although still faulty, this is an improvement: "If you look up the IPv6 resolution for localhost, then you must get either ::1 or ENOTFOUND."

To that end, I'd change the code I wrote above to this:

  if (err) {
    assert.strictEqual(err.code, 'ENOTFOUND');
    return done();
  }

That makes it a little more clear that we're not skipping the test so much as checking an alternative acceptable result.

Member

Trott commented Aug 26, 2016

I think the problem is that the test is faulty. The test is making an assumption about a system that isn't actually true on many default Linux variants as we've seen in other tests and now we're seeing in this one with AIX.

The test amounts to "If you look up the IPv6 resolution for localhost, then you must get ::1." But as we've seen, there are multiple systems where IPv6 can be configured but localhost is not set to ::1 in /etc/hosts or whatever. As much as possible, we want to isolate out system configuration quirks from the tests. That's not always possible, but as much as we can, we should do it.

To that end, I'd propose that although still faulty, this is an improvement: "If you look up the IPv6 resolution for localhost, then you must get either ::1 or ENOTFOUND."

To that end, I'd change the code I wrote above to this:

  if (err) {
    assert.strictEqual(err.code, 'ENOTFOUND');
    return done();
  }

That makes it a little more clear that we're not skipping the test so much as checking an alternative acceptable result.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 26, 2016

Member

By the way, you may be wondering why you are seeing this on AIX while it never caused problems with the Linux variants that also do not have the localhost/::1 mapping set up by default.

The answer is because the test/internet tests are not run in CI and therefore are mostly not run at all! So thanks for running them and thanks for chasing down this issue!

Member

Trott commented Aug 26, 2016

By the way, you may be wondering why you are seeing this on AIX while it never caused problems with the Linux variants that also do not have the localhost/::1 mapping set up by default.

The answer is because the test/internet tests are not run in CI and therefore are mostly not run at all! So thanks for running them and thanks for chasing down this issue!

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 27, 2016

Member

@Trott That makes a lot of sense. I'll make that change and add a comment to note down why ENOTFOUND is an acceptable answer. Thanks for the help!

Member

gibfahn commented Aug 27, 2016

@Trott That makes a lot of sense. I'll make that change and add a comment to note down why ENOTFOUND is an acceptable answer. Thanks for the help!

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 31, 2016

Member

@Trott Updated, PTAL

Member

gibfahn commented Aug 31, 2016

@Trott Updated, PTAL

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 31, 2016

Member

It may be worthwhile to expand the code comment in the test to explain a bit more. Specifically, calling out something like:

// We're not skipping the test so much as checking an alternative acceptable result.

Or some such...

Otherwise, LGTM

Member

jasnell commented Aug 31, 2016

It may be worthwhile to expand the code comment in the test to explain a bit more. Specifically, calling out something like:

// We're not skipping the test so much as checking an alternative acceptable result.

Or some such...

Otherwise, LGTM

test: fix ::1 error in test-dns-ipv6
If ::1 can't be resolved, the test should still pass.
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 31, 2016

Member

Sounds good to me, updated.

Member

gibfahn commented Aug 31, 2016

Sounds good to me, updated.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 31, 2016

Member

LGTM

Member

jasnell commented Aug 31, 2016

LGTM

@Trott

This comment has been minimized.

Show comment
Hide comment
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Sep 1, 2016

Member

Looks like CI passed except for a Git checkout issue on linuxone. As the internet tests aren't actually run, I guess the only thing the CI really tells us is that the linter passed.

Member

gibfahn commented Sep 1, 2016

Looks like CI passed except for a Git checkout issue on linuxone. As the internet tests aren't actually run, I guess the only thing the CI really tells us is that the linter passed.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 1, 2016

Member

Landed in 3504a98

Member

Trott commented Sep 1, 2016

Landed in 3504a98

@Trott Trott closed this Sep 1, 2016

Trott added a commit to Trott/io.js that referenced this pull request Sep 1, 2016

test: fix ::1 error in test-dns-ipv6
If ::1 can't be resolved, the test should still pass.

PR-URL: nodejs#8254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

test: fix ::1 error in test-dns-ipv6
If ::1 can't be resolved, the test should still pass.

PR-URL: nodejs#8254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

test: fix ::1 error in test-dns-ipv6
If ::1 can't be resolved, the test should still pass.

PR-URL: #8254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 30, 2016

test: fix ::1 error in test-dns-ipv6
If ::1 can't be resolved, the test should still pass.

PR-URL: #8254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 10, 2016

test: fix ::1 error in test-dns-ipv6
If ::1 can't be resolved, the test should still pass.

PR-URL: #8254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

rvagg added a commit that referenced this pull request Oct 18, 2016

test: fix ::1 error in test-dns-ipv6
If ::1 can't be resolved, the test should still pass.

PR-URL: #8254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

test: fix ::1 error in test-dns-ipv6
If ::1 can't be resolved, the test should still pass.

PR-URL: #8254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@MylesBorins MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

@gibfahn gibfahn deleted the gibfahn:pr-test-dns-ipv6 branch Dec 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment