test: refactor the code in test-dns-ipv6 by edsadr · Pull Request #10219 · nodejs/node · GitHub

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

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

Fixing this now for both

@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

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

@thefourtheye just fixed it as suggested

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

@Trott just fixed as suggested

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

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

@nodejs/testing

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

ping @nodejs/testing

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

I think you can remove the braces

arg aligment

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

Is this check really necessary?

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

Yes, that's what I was suggesting

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

maybe use assert.ifError(err); here

LGTM with some suggestions

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

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

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

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

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

test: refactor the code in test-dns-ipv6 by edsadr · Pull Request #10219 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

test: refactor the code in test-dns-ipv6#10219edsadr wants to merge 1 commit intonodejs:masternodejs/node:masterfrom edsadr:test-dns-ipv6edsadr/node:test-dns-ipv6Copy head branch name to clipboard

Conversation

edsadr commented Dec 10, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edsadr commented Dec 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edsadr commented Dec 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edsadr commented Dec 11, 2016

Uh oh!

italoacasas commented Dec 13, 2016

Uh oh!

edsadr commented Dec 13, 2016

Uh oh!

Trott commented Dec 13, 2016

Uh oh!

edsadr commented Dec 15, 2016

Uh oh!

italoacasas commented Dec 15, 2016

Uh oh!

santigimeno Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edsadr Dec 16, 2016 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edsadr Dec 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santigimeno left a comment

Choose a reason for hiding this comment

Uh oh!

test: refactor the code in test-dns-ipv6#10219
edsadr wants to merge 1 commit into
nodejs:masterfrom
edsadr:test-dns-ipv6

santigimeno Dec 15, 2016 •
edited
Loading

edsadr Dec 16, 2016 •
edited by Trott
Loading

edsadr Dec 17, 2016 •
edited
Loading