test: refactor the code in test-dns-ipv6 #10219
Conversation
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
Same deal as the other dns test refactor PR, this should be formatted better.
There was a problem hiding this comment.
Fixing this now for both
bbf95a2 to
c906f5f
Compare
c906f5f to
4504457
Compare
|
@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 |
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
Nit: isIPv6 is already defined at the beginning of the file. We can use that consistently.
4504457 to
1dede9f
Compare
|
@thefourtheye just fixed it as suggested |
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
Nit: either assert.strictEqual(running, false); or assert(!running);.
1dede9f to
6c25335
Compare
|
@Trott just fixed as suggested |
|
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 |
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
I would align the dns.resolve6 arguments
There was a problem hiding this comment.
@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();
}));```There was a problem hiding this comment.
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);
});There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
@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();
}));There was a problem hiding this comment.
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);There was a problem hiding this comment.
@edsadr forget about my styling comments. It's been more trouble than anything.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
I think you can remove the braces
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
arg aligment
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
I would do return done(); as in other places in the file and would get rid of the else
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
Is this check really necessary?
There was a problem hiding this comment.
@santigimeno so, should I just remove the whole process.on('exit'.. ?
There was a problem hiding this comment.
Yes, that's what I was suggesting
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
I think assert.ok(domains[i]); is redundant
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
maybe use assert.ifError(err); here
6c25335 to
533c9c8
Compare
There was a problem hiding this comment.
LGTM with some suggestions
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
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);
});
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
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);
test/internet/test-dns-ipv6.js
Outdated
There was a problem hiding this comment.
Yes, that's what I was suggesting
533c9c8 to
42abcd2
Compare
* 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
42abcd2 to
2899656
Compare
|
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>
* 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: #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>
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change