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

Jump to bottom

test: refactor test-http-dns-fail #10243

Closed
wants to merge 1 commit into from
Closed

test: refactor test-http-dns-fail #10243

wants to merge 1 commit into from

Conversation

edsadr
Copy link
Member

@edsadr edsadr commented Dec 12, 2016 •
edited
Loading

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

refactor test-http-dns-fail.js

  • remove counter used to control function execution
  • use commont.mustCall to control the function execution
  • use const and let instead of var
  • use arrow functions

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 12, 2016
@lance
Copy link
Member

lance commented Dec 12, 2016

LGTM

@mscdex mscdex added the dns Issues and PRs related to the dns subsystem. label Dec 12, 2016
mscdex
mscdex reviewed Dec 12, 2016
View reviewed changes
test/internet/test-http-dns-fail.js
console.log(e.message);
assert.strictEqual(e.code, 'ENOTFOUND');
hadError++;
httpreq(count + 1);
});
}));
Copy link
Contributor

@mscdex mscdex Dec 12, 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 suggest changing this to }, 2)); and then getting rid of hadError entirely.

EDIT: Actually after further review I think we can just get rid of hadError entirely without changing this line ...

mscdex
mscdex reviewed Dec 12, 2016
View reviewed changes
test/internet/test-http-dns-fail.js

var hadError = 0;
let hadError = 0;

function httpreq(count) {
if (1 < count) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind reversing the conditional here to count > 1 while you're in this file? I personally find it much easier to understand when scanning code...

mscdex
mscdex reviewed Dec 12, 2016
View reviewed changes
test/internet/test-http-dns-fail.js
host: 'not-a-real-domain-name.nobody-would-register-this-as-a-tld',
port: 80,
path: '/',
method: 'GET'
}, common.fail);

req.on('error', function(e) {
req.on('error', common.mustCall((e) => {
console.log(e.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be removed? I still don't know if we have an official policy on this or not ...

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Dec 12, 2016
@edsadr
test: refactor test-http-dns-fail
db029f2 
* remove counter used to control function execution
* use commont.mustCall to control the function execution
* use const and let instead of var
* use arrow functions
@edsadr edsadr changed the title test: use ES6 in test-http-dns-fail test: refactor test-http-dns-fail Dec 12, 2016
@edsadr
Copy link
Member Author

edsadr commented Dec 12, 2016

@mscdex just implemented your suggestions, also removed the console.log, I was actually wondering about this...

@mscdex
Copy link
Contributor

mscdex commented Dec 12, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Dec 13, 2016

LGTM

@italoacasas
Copy link
Contributor

Landed 75ac109

Thanks for the contribution.

italoacasas pushed a commit that referenced this pull request Dec 14, 2016
@edsadr @italoacasas
test: refactor test-http-dns-fail
75ac109 
* remove counter used to control function execution
* use commont.mustCall to control the function execution
* use const and let instead of var
* use arrow functions

PR-URL: #10243
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 15, 2016
@edsadr @MylesBorins
test: refactor test-http-dns-fail
b9361ca 
* remove counter used to control function execution
* use commont.mustCall to control the function execution
* use const and let instead of var
* use arrow functions

PR-URL: #10243
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Brian White <[email protected]>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 22, 2017
@edsadr @MylesBorins
test: refactor test-http-dns-fail
a9c6dcc 
* remove counter used to control function execution
* use commont.mustCall to control the function execution
* use const and let instead of var
* use arrow functions

PR-URL: #10243
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@edsadr @MylesBorins
test: refactor test-http-dns-fail
45d1c49 
* remove counter used to control function execution
* use commont.mustCall to control the function execution
* use const and let instead of var
* use arrow functions

PR-URL: #10243
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@edsadr @MylesBorins
test: refactor test-http-dns-fail
373755c 
* remove counter used to control function execution
* use commont.mustCall to control the function execution
* use const and let instead of var
* use arrow functions

PR-URL: #10243
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Brian White <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@italoacasas italoacasas italoacasas approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
dns Issues and PRs related to the dns subsystem. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@edsadr @lance @mscdex @italoacasas @lpinca @cjihrig @MylesBorins @nodejs-github-bot