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: various test improvements #8468

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 9, 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

Various test improvements

  • Favor strictEqual
  • Use const where appropriate
  • Modernize where possible

@jasnell jasnell added the test Issues and PRs related to the tests. label Sep 9, 2016
assert.equal(4, addressType);
});
dns.lookup(null, common.mustCall((error, result, addressType) => {
if (error) throw error;
Copy link
Contributor

Choose a reason for hiding this comment

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

assert.ifError(error)?

@cjihrig
Copy link
Contributor

cjihrig commented Sep 9, 2016

Is there any motivation for the changes from function to () =>? It doesn't look like any self variables were saved or anything. Other than that, and the comments I left, LGTM.

`"${process.execPath}"`,
'-e',
'"console.error(process.argv)"',
'foo', 'bar'].join(' ');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: there is one element per line now, what about putting 'bar' on a new line?

@lpinca
Copy link
Member

lpinca commented Sep 10, 2016

LGTM once if (error) throw error; is replaced with assert.ifError(error); as per @cjihrig comment.

@jasnell
Copy link
Member Author

jasnell commented Sep 27, 2016

Updated! PTAL

@jasnell
Copy link
Member Author

jasnell commented Sep 27, 2016


var dns = require('dns');
const dns = require('dns');

// Stub `getaddrinfo` to *always* error.
cares.getaddrinfo = function() {
Copy link
Member

@lpinca lpinca Sep 27, 2016

Choose a reason for hiding this comment

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

Nit: use an arrow function?

@lpinca
Copy link
Member

lpinca commented Sep 27, 2016

LGTM if CI is green.

* Favor strictEqual
* Use const where appropriate
* Modernize where possible
@jasnell
Copy link
Member Author

jasnell commented Oct 7, 2016

@jasnell
Copy link
Member Author

jasnell commented Oct 8, 2016

New New CI: https://ci.nodejs.org/job/node-test-pull-request/4434/ (since the last one had to be partially aborted)

@lpinca
Copy link
Member

lpinca commented Oct 9, 2016

Failures seem to be unrelated to these changes.

jasnell added a commit that referenced this pull request Oct 10, 2016
* Favor strictEqual
* Use const where appropriate
* Modernize where possible

PR-URL: #8468
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Oct 10, 2016

Landed in 07b54ec.

@jasnell jasnell closed this Oct 10, 2016
jasnell added a commit that referenced this pull request Oct 10, 2016
* Favor strictEqual
* Use const where appropriate
* Modernize where possible

PR-URL: #8468
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
* Favor strictEqual
* Use const where appropriate
* Modernize where possible

PR-URL: #8468
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins
Copy link
Contributor

Setting as don't land for v4.x

@jasnell please feel free to manually backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants