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.

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

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

Closed
wants to merge 1 commit into from
Closed

Conversation

edsadr
Copy link
Member

@edsadr edsadr commented Dec 12, 2016

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
console.log(e.message);
assert.strictEqual(e.code, 'ENOTFOUND');
hadError++;
httpreq(count + 1);
});
}));
Copy link
Contributor

@mscdex mscdex Dec 12, 2016

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


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

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
* 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

@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
* 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 <me@italoacasas.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Dec 15, 2016
* 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 <me@italoacasas.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
MylesBorins pushed a commit that referenced this pull request Jan 22, 2017
* 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 <me@italoacasas.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
* 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 <me@italoacasas.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
* 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 <me@italoacasas.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

8 participants