First time contribution from NodeConf EU #16821

paulashfield · 2017-11-06T15:38:44Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

apapirovski

Thanks @paulashfield — just some minor feedback below.

apapirovski · 2017-11-06T15:56:43Z

test/addons-napi/test_general/testNapiRun.js

-assert.strictEqual(addon.testNapiRun('(41.92 + 0.08);'), 42,
-                   'napi_run_script() works correctly');
-assert.throws(() => addon.testNapiRun({ abc: 'def' }), /string was expected/);
+const args = [41.92, 0.08];


This would be a bit nicer as just, say:

const val1 = 41.92; const val2 = 0.08;

apapirovski · 2017-11-06T15:57:41Z

test/addons-napi/test_general/testNapiRun.js

+const args = [41.92, 0.08];
+const testCase = '(' + args.join(' + ') + ');';
+const expected = 43;
+const actual = addon.testNapiRun(testCase);


This could be then rewritten as:

const actual = addon.testNapiRun(`(${val1} + ${val2});`);

apapirovski · 2017-11-06T15:58:03Z

test/addons-napi/test_general/testNapiRun.js

-assert.throws(() => addon.testNapiRun({ abc: 'def' }), /string was expected/);
+const args = [41.92, 0.08];
+const testCase = '(' + args.join(' + ') + ');';
+const expected = 43;


This could then be const expected = val1 + val2;

apapirovski · 2017-11-06T15:59:12Z

test/addons-napi/test_general/testNapiRun.js

+const expected = 43;
+const actual = addon.testNapiRun(testCase);
+
+assert.strictEqual(actual, expected, `testNapiRun${testCase} gets ${actual} not expected ${expected}`);


Here you could remove the third argument (the error message) as the default message communicates the same thing.

gireeshpunathil · 2017-11-06T16:21:28Z

@paulashfield -
looks like you pushed changed without:

#git config --global user.name
#git config --global user.email

on your system - implication of which is that this commit will not be associated with your profile. Can you set them up and push once again?

vsemozhetbyt · 2017-11-06T17:39:40Z

@paulashfield See also the last note in this chapter: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#setting-up-your-local-environment

vsemozhetbyt · 2017-11-06T17:41:18Z

test/addons-napi/test_general/testNapiRun.js

+const actual = addon.testNapiRun(testCase);
+
+assert.strictEqual(actual, expected, `testNapiRun${testCase} gets ${actual} not expected ${expected}`);
+assert.throws(() => addon.testNapiRun({abc: 'def'}), /string was expected/);


A nit: the last line seems lacking the last line break.

paulashfield · 2017-11-07T10:44:12Z

Many thanks all for feedback - really helped. I have amended and recommitted, hope that looks better.

Trott · 2017-11-07T23:45:34Z

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

Trott · 2017-11-08T00:51:40Z

Hi @paulashfield! Welcome and thanks for the PR! If you run make lint-js (or vcbuild lint-js if you're on Windows), you will see a report of a linting error on line 15. Could you fix that up?

Trott

Needs spacing flagged by linter fixed...

…nd Gireesh - thanks all

Trott

Because the lint issue was just about adding two spaces, I went ahead and did it myself. Hope that's OK.

Trott · 2017-11-08T12:30:37Z

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

PR-URL: nodejs#16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Trott · 2017-11-08T13:59:42Z

Landed in 3ee524b.
Thanks for the contribution! 🎉

PR-URL: #16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

gibfahn · 2017-12-13T16:45:44Z

@paulashfield just an FYI, this commit isn't associated with your Github account. You need to go to https://github.com/settings/emails and add paul.ashfield@intercity.technology. Then all your commits will be associated with you.

That's why there's a ? next to your name on the commit: 3ee524b

PR-URL: #16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: nodejs#16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Backport-PR-URL: #19447 PR-URL: #16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

nodejs-github-bot added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Nov 6, 2017

apapirovski reviewed Nov 6, 2017

View reviewed changes

apapirovski added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2017

vsemozhetbyt reviewed Nov 6, 2017

View reviewed changes

gireeshpunathil approved these changes Nov 7, 2017

View reviewed changes

Trott approved these changes Nov 7, 2017

View reviewed changes

Trott requested changes Nov 8, 2017

View reviewed changes

Paul Ashfield and others added 3 commits November 8, 2017 12:28

Nodeconf is awesome first change

81cfdc6

Updated test case based on feedback from @apapirovski @vsemozhetbyt a…

a9d7c3d

…nd Gireesh - thanks all

squash: lint fix

2b1ad0a

Trott force-pushed the master branch from ee21e80 to 2b1ad0a Compare November 8, 2017 12:29

Trott approved these changes Nov 8, 2017

View reviewed changes

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 8, 2017

test: improve error emssage reporting in testNapiRun.js

3ee524b

PR-URL: nodejs#16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Trott closed this Nov 8, 2017

evanlucas pushed a commit that referenced this pull request Nov 13, 2017

test: improve error emssage reporting in testNapiRun.js

5d3a4ad

PR-URL: #16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

evanlucas mentioned this pull request Nov 13, 2017

v9.2.0 proposal #16992

Merged

LJNeon mentioned this pull request Nov 15, 2017

Typo in the 9.2.0 changelog. #17061

Closed

gibfahn added the land-on-v8.x label Dec 13, 2017

gibfahn pushed a commit that referenced this pull request Dec 13, 2017

test: improve error emssage reporting in testNapiRun.js

25ff7bf

PR-URL: #16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

This was referenced Dec 20, 2017

v8.9.4 proposal #17772

Closed

v8.9.4 proposal #17774

Merged

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018

test: improve error emssage reporting in testNapiRun.js

ab95aa4

PR-URL: nodejs#16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

MylesBorins pushed a commit that referenced this pull request Apr 16, 2018

test: improve error emssage reporting in testNapiRun.js

2476ab9

Backport-PR-URL: #19447 PR-URL: #16821 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First time contribution from NodeConf EU #16821

First time contribution from NodeConf EU #16821

paulashfield commented Nov 6, 2017

apapirovski left a comment

apapirovski Nov 6, 2017

apapirovski Nov 6, 2017

apapirovski Nov 6, 2017

apapirovski Nov 6, 2017

gireeshpunathil commented Nov 6, 2017

vsemozhetbyt commented Nov 6, 2017

vsemozhetbyt Nov 6, 2017

paulashfield commented Nov 7, 2017

Trott commented Nov 7, 2017

Trott commented Nov 8, 2017

Trott left a comment

Trott left a comment

Trott commented Nov 8, 2017

Trott commented Nov 8, 2017

gibfahn commented Dec 13, 2017

First time contribution from NodeConf EU #16821

First time contribution from NodeConf EU #16821

Conversation

paulashfield commented Nov 6, 2017

Checklist

Affected core subsystem(s)

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Nov 6, 2017

Choose a reason for hiding this comment

apapirovski Nov 6, 2017

Choose a reason for hiding this comment

apapirovski Nov 6, 2017

Choose a reason for hiding this comment

apapirovski Nov 6, 2017

Choose a reason for hiding this comment

gireeshpunathil commented Nov 6, 2017

vsemozhetbyt commented Nov 6, 2017

vsemozhetbyt Nov 6, 2017

Choose a reason for hiding this comment

paulashfield commented Nov 7, 2017

Trott commented Nov 7, 2017

Trott commented Nov 8, 2017

Trott left a comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

Trott commented Nov 8, 2017

Trott commented Nov 8, 2017

gibfahn commented Dec 13, 2017