Skip to content

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

Closed
edsadr wants to merge 1 commit into
nodejs:masterfrom
edsadr:test-dns-ipv4
Closed

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

Conversation

@edsadr

Copy link
Copy Markdown
Contributor
Checklist
  • make -j8 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

use let and const instead of var
add missing common.mustCall for anonymous functions

@nodejs-github-botnodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 9, 2016
@mscdexmscdex added the dns Issues and PRs related to the dns subsystem. label Dec 9, 2016
@mscdex

mscdex commented Dec 9, 2016

Copy link
Copy Markdown
Contributor

Can't we also get rid of the expected and completed variables now that we're using common.mustCall()?

Comment threadtest/internet/test-dns-ipv4.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would you mind replacing all of these lines in the file with assert.ifError(err);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

will perform these changes later tonight

@edsadredsadr changed the title test: improve the code in test-dns-ipv4test: refactor the code in test-dns-ipv4Dec 10, 2016
@edsadr

Copy link
Copy Markdown
ContributorAuthor

@mscdex@cjihrig just applied the changes you requested

Comment threadtest/internet/test-dns-ipv4.js Outdated

@mscdexmscdexDec 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather either see this indented or use an arrow function to keep it on the previous line. If you go the arrow function route, we should be more consistent though and use it for the other callbacks too.

Same goes for the other instances below.

Comment threadtest/internet/test-dns-ipv4.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

common should be required before anything else.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

will do this change too after get the #10219 approved, I am learning the right format for the code

* 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

Copy link
Copy Markdown
ContributorAuthor

changed the style to match the one agreed in #10219

@italoacasas

Copy link
Copy Markdown

@edsadr

Copy link
Copy Markdown
ContributorAuthor

any update on this one?

@julianduque

Copy link
Copy Markdown
Contributor

@italoacasas

italoacasas commented Dec 23, 2016

Copy link
Copy Markdown

landing this tomorrow if not objections @nodejs/collaboration

@gibfahn

Copy link
Copy Markdown
Member

@italoacasas I think you meant @nodejs/collaborators

italoacasas pushed a commit that referenced this pull request Dec 28, 2016
* 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: #10200
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@italoacasas

Copy link
Copy Markdown

Landed 4d3b487

@evanlucasevanlucas mentioned this pull request Jan 3, 2017
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
* 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: #10200
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
* 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: #10200
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
* 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: #10200
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
* 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: #10200
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
* 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: #10200
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dnsIssues and PRs related to the dns subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@edsadr@mscdex@italoacasas@julianduque@gibfahn@thefourtheye@cjihrig@MylesBorins@Fishrock123@nodejs-github-bot