remove var and use let and const #9903

duylebao · 2016-12-01T16:22:19Z

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

remove var and use let and const
remove assert.equals and use assert.strictEqual
new unit test to handle case when requiring invalid path to package.json

cjihrig · 2016-12-03T16:15:46Z

test/parallel/test-require-invalid-package.js

+require('../common');
+const assert = require('assert');
+
+// should be invalid package path


Can you capitalize and punctuate the comment please.

cjihrig · 2016-12-03T16:16:04Z

test/parallel/test-require-invalid-package.js

+const assert = require('assert');
+
+// should be invalid package path
+try {


This should use assert.throws().

duylebao · 2016-12-05T23:21:36Z

@cjihrig made the changes as suggested

cjihrig

Thanks. LGTM

duylebao · 2016-12-06T00:50:25Z

thank you for approving, I don't think I have access to merge the pull request, I assume one of you will be merging this then?

jasnell · 2016-12-06T01:36:22Z

Once one of us has had an opportunity to run it through CI, assuming that comes up good, one of the committers will get it landed. The CI has been worked pretty hard today to I'm saving up a batch of tests for tomorrow.

fhinkel · 2016-12-06T15:09:32Z

Thanks. LGTM. Can you squash the commits into one and change the commit message, please? It should start with the subsystem, i.e., test:

jasnell · 2016-12-06T15:37:48Z

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

@duylebao : ~~can you squash the commits in this down to a single commit?~~ ... ha! just saw that @fhinkel had just asked... guess I should refresh before posting :-)

duylebao · 2016-12-07T00:38:21Z

never had to use squash before, tried the following:

git rebase -i HEAD~4
change all the choose to be s
try to save the file
get the following error: Cannot 'squash' without a previous commit

anyone know what's going on?

cjihrig · 2016-12-07T00:40:28Z

You'll want to keep one of the four :-)

remove assert.equal to use assert.strictEqual new unit test for requiring an invalid package

duylebao · 2016-12-07T00:49:01Z

thanks, commits have been squashed.

Trott · 2016-12-08T22:52:26Z

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

Trott · 2016-12-09T03:29:13Z

CI issues unrelated to these changes. Let's run again.

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

* var -> const * assert.equal() -> assert.strictEqual() PR-URL: nodejs#9903 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Add test for requiriing an invalid package path. PR-URL: nodejs#9903 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

* var -> const/let * assert.equal() -> assert.strictEqual() PR-URL: nodejs#9903 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Trott · 2016-12-21T23:13:41Z

This probably should have been three separate PRs, one for each file (or at least two separate PRs, one for the refactors and one for the new test). So I split it into separate commits.

Landed in 576cf4b, 283321e, and 3c8a3a5.

Thanks for the contribution! 🎉

* var -> const * assert.equal() -> assert.strictEqual() PR-URL: #9903 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>