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: find IPv6 address from localhost names #7806

Closed
wants to merge 2 commits into from

Conversation

thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Jul 20, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Try all the possible hostnames listed in common.localIPv6Hosts to get
an IPv6 address. If none of them give a valid address, skip the tests.

cc @nodejs/testing

@thefourtheye thefourtheye added the test Issues and PRs related to the tests. label Jul 20, 2016
@thefourtheye
Copy link
Contributor Author

thefourtheye commented Jul 20, 2016

Try all the possible hostnames listed in `common.localIPv6Hosts` to get
an IPv6 address. If none of them give a valid address, skip the tests.
@thefourtheye
Copy link
Contributor Author

thefourtheye commented Jul 20, 2016

@bnoordhuis I found a way to fix that. Testing it in my local VMs. I'll update shortly.

@thefourtheye
Copy link
Contributor Author

thefourtheye commented Jul 20, 2016

@thefourtheye
Copy link
Contributor Author

Okay. All green now (Except Fedora 24, which keeps building and testing again and again). https://ci.nodejs.org/job/node-test-pull-request/3352/

@bnoordhuis
Copy link
Member

I have to say that the changes to test/common.js don't look very appealing but I'll defer to @nodejs/testing.

@thefourtheye
Copy link
Contributor Author

@bnoordhuis Any suggestions to make it better?

@thefourtheye
Copy link
Contributor Author

thefourtheye commented Jul 20, 2016

@bnoordhuis Will this be better?

exports.getLocalIPv6Address = function getLocalIPv6Address(callback) {
  if (exports.localIPv6Hosts.length === 0 || localhostIPv6 === undefined) {
    localhostIPv6 = undefined;
    return process.nextTick(
      () => callback(new Error('Unable to determine IPv6 address'))
    );
  }
  (function processNextHost(current) {
    if (current < exports.localIPv6Hosts.length) {
      const host = exports.localIPv6Hosts[current];
      return dns.lookup(host, {family: 6}, (_, addr) => {
        if (addr && typeof addr === 'string') {
          localhostIPv6 = {hostname: host, address: addr};
          return callback(null, localhostIPv6);
        }
        return processNextHost(current + 1);
      });
    }
    localhostIPv6 = undefined;
    callback(new Error('Unable to determine IPv6 address'));
  })(0);
};

Edit: I probably can move the Error creation the recursive function and nextTick, so that the stacktrace will be better.

@Trott
Copy link
Member

Trott commented Jul 20, 2016

I have to say that the changes to test/common.js don't look very appealing but I'll defer to @nodejs/testing.

I feel the same, but would be curious how more people on @nodejs/testing felt.

This function seems to be only needed in two tests so it's not clear to me that common.js is the place for it.

@thefourtheye thefourtheye deleted the fix-ipv6-tests branch July 20, 2016 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants