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

test: refactor the code in test-dns-ipv6 #10219

Closed
wants to merge 1 commit into from
Closed

test: refactor the code in test-dns-ipv6 #10219

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 61 additions & 68 deletions test/internet/test-dns-ipv6.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ const dns = require('dns');
const net = require('net');
const isIPv6 = net.isIPv6;

let expected = 0;
let completed = 0;
let running = false;
const queue = [];

Expand All @@ -17,7 +15,7 @@ if (!common.hasIPv6) {

function TEST(f) {
function next() {
var f = queue.shift();
const f = queue.shift();
if (f) {
running = true;
console.log(f.name);
Expand All @@ -27,11 +25,9 @@ function TEST(f) {

function done() {
running = false;
completed++;
process.nextTick(next);
}

expected++;
queue.push(f);

if (!running) {
Expand All @@ -44,46 +40,46 @@ function checkWrap(req) {
}

TEST(function test_resolve6(done) {
var req = dns.resolve6('ipv6.google.com', function(err, ips) {
if (err) throw err;
const req = dns.resolve6('ipv6.google.com',
common.mustCall((err, ips) => {
Copy link
Member

@santigimeno santigimeno Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I would align the dns.resolve6 arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

@santigimeno , sorry buts just learning about the codestyle, could you please provide an example of how to properly align this:

const req = dns.resolve6('ipv6.google.com',
    common.mustCall((err, ips) => {
      assert.ifError(err);

      assert.ok(ips.length > 0);

      for (let i = 0; i < ips.length; i++)
        assert.ok(isIPv6(ips[i]));

      done();
    }));```

Copy link
Member

Choose a reason for hiding this comment

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

I would do something like this, but I don't think is that important:

TEST(function test_resolve6(done) {
  const req = dns.resolve6('ipv6.google.com',
                           common.mustCall((err, ips) => {
    assert.ifError(err);
    assert.ok(ips.length > 0);
    for (let i = 0; i < ips.length; i++)
      assert.ok(isIPv6(ips[i]));

    done();
  }));

  checkWrap(req);
});

Copy link
Member Author

@edsadr edsadr Dec 16, 2016 •
edited by Trott
Loading

Choose a reason for hiding this comment

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

well... that way has problems with the linter as it is expecting the code to be like this:

const req = dns.resolve6('ipv6.google.com',
                            common.mustCall((err, ips) => {
                              assert.ifError(err);

                              assert.ok(ips.length > 0);

                              for (let i = 0; i < ips.length; i++)
                                assert.ok(isIPv6(ips[i]));

                              done();
                            }));

in the previous way I didn't have linter problems... just let me know what to do here then to keep the current or ident the whole function code...

I implemented all the other suggestions 🙂

Copy link
Member

Choose a reason for hiding this comment

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

@edsadr I think you have common.mustCall() and everything below it indented by 3 spaces too much. I think it should be like this:

const req = dns.resolve6('ipv6.google.com',
                         common.mustCall((err, ips) => {
                           assert.ifError(err);

                           assert.ok(ips.length > 0);

                           for (let i = 0; i < ips.length; i++)
                             assert.ok(isIPv6(ips[i]));

                           done();
                         }));

Copy link
Member

Choose a reason for hiding this comment

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

And, of course, you can always do something like this too:

const callback = common.mustCall((err, ipe) => {
  assert.ifError(err);
  ...
});

const req = dns.resolve6('ipv6.google.com', callback);

Copy link
Member

Choose a reason for hiding this comment

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

@edsadr forget about my styling comments. It's been more trouble than anything.

Copy link
Member Author

@edsadr edsadr Dec 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

no prob @santigimeno, still your suggestions helped me to learn a little bit about the codestyling ... I like @Trott suggestions, but I would propose to keep the current format for consistency, if I implement as suggested some other tests below will look weird... so.. what do you say?

Copy link
Member

Choose a reason for hiding this comment

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

@edsadr I think any reasonable indentation for this that the linter finds acceptable is acceptable by me. I think @santigimeno seems to feel the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok @Trott ... then I think I will let it with the current identation... is not offending the linter, and looks ok

assert.ifError(err);

assert.ok(ips.length > 0);
assert.ok(ips.length > 0);

for (var i = 0; i < ips.length; i++) {
assert.ok(isIPv6(ips[i]));
}
for (let i = 0; i < ips.length; i++)
assert.ok(isIPv6(ips[i]));

done();
});
done();
}));

checkWrap(req);
});

TEST(function test_reverse_ipv6(done) {
var req = dns.reverse('2001:4860:4860::8888', function(err, domains) {
if (err) throw err;
const req = dns.reverse('2001:4860:4860::8888',
common.mustCall((err, domains) => {
assert.ifError(err);

assert.ok(domains.length > 0);
assert.ok(domains.length > 0);

for (var i = 0; i < domains.length; i++) {
assert.ok(domains[i]);
assert.ok(typeof domains[i] === 'string');
}
for (let i = 0; i < domains.length; i++)
assert.ok(typeof domains[i] === 'string');

done();
});
done();
}));

checkWrap(req);
});

TEST(function test_lookup_ipv6_explicit(done) {
var req = dns.lookup('ipv6.google.com', 6, function(err, ip, family) {
if (err) throw err;
assert.ok(net.isIPv6(ip));
assert.strictEqual(family, 6);
const req = dns.lookup('ipv6.google.com', 6,
common.mustCall((err, ip, family) => {
Copy link
Member

Choose a reason for hiding this comment

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

arg aligment

assert.ifError(err);
assert.ok(isIPv6(ip));
assert.strictEqual(family, 6);

done();
});
done();
}));

checkWrap(req);
});
Expand All @@ -103,24 +99,24 @@ TEST(function test_lookup_ipv6_implicit(done) {
*/

TEST(function test_lookup_ipv6_explicit_object(done) {
var req = dns.lookup('ipv6.google.com', {
const req = dns.lookup('ipv6.google.com', {
family: 6
}, function(err, ip, family) {
if (err) throw err;
assert.ok(net.isIPv6(ip));
}, common.mustCall((err, ip, family) => {
assert.ifError(err);
assert.ok(isIPv6(ip));
assert.strictEqual(family, 6);

done();
});
}));

checkWrap(req);
});

TEST(function test_lookup_ipv6_hint(done) {
var req = dns.lookup('www.google.com', {
const req = dns.lookup('www.google.com', {
family: 6,
hints: dns.V4MAPPED
}, function(err, ip, family) {
}, common.mustCall((err, ip, family) => {
if (err) {
// FreeBSD does not support V4MAPPED
if (common.isFreeBSD) {
Expand All @@ -130,66 +126,69 @@ TEST(function test_lookup_ipv6_hint(done) {
assert.ok(/getaddrinfo EAI_BADFLAGS/.test(err.message));
done();
return;
} else {
throw err;
}

assert.ifError(err);
}
assert.ok(net.isIPv6(ip));

assert.ok(isIPv6(ip));
assert.strictEqual(family, 6);

done();
});
}));

checkWrap(req);
});

TEST(function test_lookup_ip_ipv6(done) {
var req = dns.lookup('::1', function(err, ip, family) {
if (err) throw err;
assert.ok(net.isIPv6(ip));
assert.strictEqual(family, 6);
const req = dns.lookup('::1',
common.mustCall((err, ip, family) => {
assert.ifError(err);
assert.ok(isIPv6(ip));
assert.strictEqual(family, 6);

done();
});
done();
}));

checkWrap(req);
});

TEST(function test_lookup_all_ipv6(done) {
var req = dns.lookup(
const req = dns.lookup(
'www.google.com',
{all: true, family: 6},
function(err, ips) {
if (err) throw err;
common.mustCall((err, ips) => {
assert.ifError(err);
assert.ok(Array.isArray(ips));
assert.ok(ips.length > 0);

ips.forEach(function(ip) {
ips.forEach((ip) => {
assert.ok(isIPv6(ip.address),
'Invalid IPv6: ' + ip.address.toString());
assert.strictEqual(ip.family, 6);
});

done();
}
);
));

checkWrap(req);
});

TEST(function test_lookupservice_ip_ipv6(done) {
var req = dns.lookupService('::1', 80, function(err, host, service) {
if (err) {
// Not skipping the test, rather checking an alternative result,
// i.e. that ::1 may not be configured (e.g. in /etc/hosts)
assert.strictEqual(err.code, 'ENOTFOUND');
return done();
}
assert.equal(typeof host, 'string');
assert(host);
assert(['http', 'www', '80'].includes(service));
done();
});
const req = dns.lookupService('::1', 80,
common.mustCall((err, host, service) => {
if (err) {
// Not skipping the test, rather checking an alternative result,
// i.e. that ::1 may not be configured (e.g. in /etc/hosts)
assert.strictEqual(err.code, 'ENOTFOUND');
return done();
}
assert.strictEqual(typeof host, 'string');
assert(host);
assert(['http', 'www', '80'].includes(service));
done();
}));

checkWrap(req);
});
Expand All @@ -206,9 +205,3 @@ TEST(function test_lookupservice_ip_ipv6(done) {

checkWrap(req);
}); */

process.on('exit', function() {
console.log(completed + ' tests completed');
assert.equal(running, false);
assert.strictEqual(expected, completed);
});