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

edsadr · 2016-12-10T16:53:25Z

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

remove the manual control for functions execution
use common.mustCall to control the functions execution automatically
use let and const instead of var

mscdex · 2016-12-10T16:58:27Z

test/internet/test-dns-ipv6.js

-  var req = dns.resolve6('ipv6.google.com', function(err, ips) {
-    if (err) throw err;
+  const req = dns.resolve6('ipv6.google.com',
+  common.mustCall(function(err, ips) {


Same deal as the other dns test refactor PR, this should be formatted better.

Fixing this now for both

edsadr · 2016-12-10T17:25:31Z

@mscdex went for the arrow function, but in many cases even a new line was required, so idented them too, PTAL and if you are OK with this format will fix the other PR to match

thefourtheye · 2016-12-10T18:14:10Z

test/internet/test-dns-ipv6.js

+  const req = dns.lookup('ipv6.google.com', 6,
+    common.mustCall((err, ip, family) => {
+      assert.ifError(err);
+      assert.ok(net.isIPv6(ip));


Nit: isIPv6 is already defined at the beginning of the file. We can use that consistently.

edsadr · 2016-12-11T03:18:32Z

@thefourtheye just fixed it as suggested

Trott · 2016-12-11T05:28:09Z

test/internet/test-dns-ipv6.js

@@ -207,8 +208,6 @@ TEST(function test_lookupservice_ip_ipv6(done) {
  checkWrap(req);
 }); */

-process.on('exit', function() {
-  console.log(completed + ' tests completed');
+process.on('exit', () => {
  assert.equal(running, false);


Nit: either assert.strictEqual(running, false); or assert(!running);.

edsadr · 2016-12-11T13:10:31Z

@Trott just fixed as suggested

italoacasas · 2016-12-13T01:19:56Z

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

edsadr · 2016-12-13T02:10:37Z

the CI looks good, I guess just need the LGTM from @mscdex, @thefourtheye and @Trott about their requested changes

Trott · 2016-12-13T14:24:38Z

@nodejs/testing

edsadr · 2016-12-15T14:15:19Z

any feedback/update for this one? want to also update #10200 according which is also pending

italoacasas · 2016-12-15T15:38:03Z

ping @nodejs/testing

santigimeno · 2016-12-15T15:40:41Z

test/internet/test-dns-ipv6.js

-  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) => {


I would align the dns.resolve6 arguments

@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(); }));```

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); });

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 🙂

@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(); }));

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);

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

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?

@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.

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

santigimeno · 2016-12-15T15:40:58Z

test/internet/test-dns-ipv6.js

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


I think you can remove the braces

santigimeno · 2016-12-15T15:41:17Z

test/internet/test-dns-ipv6.js

-    assert.ok(net.isIPv6(ip));
-    assert.strictEqual(family, 6);
+  const req = dns.lookup('ipv6.google.com', 6,
+    common.mustCall((err, ip, family) => {


arg aligment

santigimeno · 2016-12-15T15:42:31Z

test/internet/test-dns-ipv6.js

@@ -134,62 +133,64 @@ TEST(function test_lookup_ipv6_hint(done) {
        throw err;


I would do return done(); as in other places in the file and would get rid of the else

santigimeno · 2016-12-15T15:46:27Z

test/internet/test-dns-ipv6.js

-  assert.equal(running, false);
-  assert.strictEqual(expected, completed);
+process.on('exit', () => {
+  assert.strictEqual(running, false);


Is this check really necessary?

@santigimeno so, should I just remove the whole process.on('exit'.. ?

Yes, that's what I was suggesting

santigimeno · 2016-12-15T15:49:25Z

test/internet/test-dns-ipv6.js

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


I think assert.ok(domains[i]); is redundant

santigimeno · 2016-12-15T15:58:21Z

test/internet/test-dns-ipv6.js

@@ -134,62 +133,64 @@ TEST(function test_lookup_ipv6_hint(done) {
        throw err;


maybe use assert.ifError(err); here

santigimeno

LGTM with some suggestions

santigimeno · 2016-12-16T06:59:52Z

test/internet/test-dns-ipv6.js

-  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) => {


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); });

santigimeno · 2016-12-16T07:01:48Z

test/internet/test-dns-ipv6.js

@@ -131,65 +127,67 @@ TEST(function test_lookup_ipv6_hint(done) {
        done();
        return;
      } else {
-        throw err;
+        assert.ifError(err);


Style nit. I would rewrite it like this:

if (common.isFreeBSD) { assert(err instanceof Error); assert.strictEqual(err.code, 'EAI_BADFLAGS'); assert.strictEqual(err.hostname, 'www.google.com'); assert.ok(/getaddrinfo EAI_BADFLAGS/.test(err.message)); return done(); } assert.ifError(err);

santigimeno · 2016-12-16T07:03:30Z

test/internet/test-dns-ipv6.js

-  assert.equal(running, false);
-  assert.strictEqual(expected, completed);
+process.on('exit', () => {
+  assert.strictEqual(running, false);


Yes, that's what I was suggesting

* remove the manual control for functions execution * use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead assert.equal

santigimeno · 2016-12-17T08:29:05Z

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

italoacasas · 2016-12-19T11:54:17Z

Landed 5e781a3

* remove the manual control for functions execution * use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead assert.equal PR-URL: #10219 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

* remove the manual control for functions execution * use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead assert.equal PR-URL: nodejs#10219 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

* remove the manual control for functions execution * use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead assert.equal PR-URL: #10219 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

nodejs-github-bot added dont-land-on-v7.x test Issues and PRs related to the tests. labels Dec 10, 2016

mscdex added the dns Issues and PRs related to the dns subsystem. label Dec 10, 2016

mscdex reviewed Dec 10, 2016

View reviewed changes

edsadr force-pushed the test-dns-ipv6 branch from bbf95a2 to c906f5f Compare December 10, 2016 17:10

nodejs-github-bot added the dont-land-on-v7.x label Dec 10, 2016

edsadr force-pushed the test-dns-ipv6 branch from c906f5f to 4504457 Compare December 10, 2016 17:22

nodejs-github-bot added the dont-land-on-v7.x label Dec 10, 2016

thefourtheye reviewed Dec 10, 2016

View reviewed changes

edsadr force-pushed the test-dns-ipv6 branch from 4504457 to 1dede9f Compare December 11, 2016 03:17

nodejs-github-bot added the dont-land-on-v7.x label Dec 11, 2016

edsadr mentioned this pull request Dec 11, 2016

test: refactor the code in test-dns-ipv4 #10200

Closed

3 tasks

Trott reviewed Dec 11, 2016

View reviewed changes

edsadr force-pushed the test-dns-ipv6 branch from 1dede9f to 6c25335 Compare December 11, 2016 13:09

nodejs-github-bot added the dont-land-on-v7.x label Dec 11, 2016

Fishrock123 added lts-watch-v6.x and removed dont-land-on-v7.x labels Dec 12, 2016

santigimeno reviewed Dec 15, 2016

View reviewed changes

edsadr force-pushed the test-dns-ipv6 branch from 6c25335 to 533c9c8 Compare December 15, 2016 23:32

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

santigimeno approved these changes Dec 16, 2016

View reviewed changes

edsadr force-pushed the test-dns-ipv6 branch from 533c9c8 to 42abcd2 Compare December 16, 2016 15:59

nodejs-github-bot added the lts-watch-v6.x label Dec 16, 2016

test: refactor the code in test-dns-ipv6

2899656

* remove the manual control for functions execution * use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead assert.equal

edsadr force-pushed the test-dns-ipv6 branch from 42abcd2 to 2899656 Compare December 16, 2016 16:02

nodejs-github-bot added the lts-watch-v6.x label Dec 16, 2016

italoacasas closed this Dec 19, 2016

italoacasas mentioned this pull request Dec 20, 2016

v7.3.0 proposal #10277

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v6.x labels Jan 22, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v6.10.0 proposal #10974

Merged

		@@ -134,62 +133,64 @@ TEST(function test_lookup_ipv6_hint(done) {
		throw err;

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

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

Conversation

edsadr commented Dec 10, 2016

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsadr commented Dec 10, 2016

Choose a reason for hiding this comment

edsadr commented Dec 11, 2016

Choose a reason for hiding this comment

edsadr commented Dec 11, 2016

italoacasas commented Dec 13, 2016

edsadr commented Dec 13, 2016

Trott commented Dec 13, 2016

edsadr commented Dec 15, 2016

italoacasas commented Dec 15, 2016

santigimeno Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsadr Dec 16, 2016 • edited by Trott Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsadr Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santigimeno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santigimeno commented Dec 17, 2016

italoacasas commented Dec 19, 2016

santigimeno Dec 15, 2016 •

edited

Loading

edsadr Dec 16, 2016 •

edited by Trott

Loading

edsadr Dec 17, 2016 •

edited

Loading