n-api: add more `int64_t` tests by kfarnung · Pull Request #19402 · nodejs/node

kfarnung · 2018-03-17T01:20:50Z

Also updated/added tests for Number and int32_t as well.

Updated N-API int64 behavior to check for all non-finite numbers
instead of just NaN.

Ref: nodejs/node-chakracore#500

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

kfarnung · 2018-03-17T01:21:11Z

/cc @boingoing @digitalinfinity @MSLaguana

kfarnung · 2018-03-17T01:22:01Z

CI: ~~https://ci.nodejs.org/job/node-test-pull-request/13713/~~
https://ci.nodejs.org/job/node-test-pull-request/13714/

Rerun linuxone: https://ci.nodejs.org/job/node-test-commit-linuxone/13701/

mhdawson · 2018-03-20T17:58:54Z

src/node_api.cc

Can you update the comment above as well. It refers to NaN and the check no longer does. Would be good if the comment explained why the broader check makes sense.

Yup, I forgot to do that.

kfarnung · 2018-03-20T18:05:52Z

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

kfarnung · 2018-03-20T22:12:29Z

@nodejs/n-api Can you please take a look?

boingoing

👍

kfarnung · 2018-03-21T18:37:54Z

@mhdawson I also made some tweaks to the documentation, any other concerns about this change? There's a somewhat open issue about what we expect the API to do when the Number value exceeds the range of int64, but I'm not addressing that directly here other than to have a test for the current behavior.

kfarnung · 2018-03-22T22:47:51Z

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

kfarnung · 2018-03-27T20:40:55Z

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

kfarnung · 2018-03-27T20:41:08Z

@nodejs/n-api Any thoughts on this PR?

gabrielschulhof · 2018-03-27T21:16:18Z

src/node_api.cc

"... values [to] match that ... "

... and perhaps "Special[-]case all ..."

kfarnung · 2018-03-27T22:12:58Z

Rebase and CI: ~~https://ci.nodejs.org/job/node-test-pull-request/13896/~~
https://ci.nodejs.org/job/node-test-pull-request/13904/

mhdawson

LGTM

mhdawson · 2018-04-04T22:16:54Z

I think a new CI is required at this point: https://ci.nodejs.org/job/node-test-pull-request/14061/

kfarnung · 2018-04-04T23:30:33Z

It looks like the Linux failure was in doc generation:

https://ci.nodejs.org/job/node-test-commit-linux/17645/nodes=ubuntu1204-64/console

And FreeBSD 10 failed a test:

https://ci.nodejs.org/job/node-test-commit-freebsd/16725/nodes=freebsd10-64/console

not ok 712 parallel/test-http-client-timeout-agent
  ---
  duration_ms: 2.124
  severity: fail
  stack: |-
    res#0 data:0
    res#0 end
    res#2 data:2
    res#2 end
    res#4 data:4
    res#4 end
    res#6 data:6
    res#6 end
    res#8 data:8
    res#8 end
    res#10 data:10
    res#10 end
    res#12 data:12
    res#12 end
    res#14 data:14
    res#14 end
    res#16 data:16
    res#16 end
    res#18 data:18
    res#18 end
    res#20 data:20
    res#20 end
    res#22 data:22
    res#22 end
    res#24 data:24
    res#24 end
    res#26 data:26
    res#26 end
    res#28 data:28
    res#28 end
    req#1 timeout
    req#3 timeout
    req#5 timeout
    req#7 timeout
    req#9 timeout
    req#11 timeout
    req#13 timeout
    req#15 timeout
    req#17 timeout
    req#19 timeout
    req#21 timeout
    req#23 timeout
    req#25 timeout
    req#27 timeout
    req#29 timeout
    req#0 timeout
    req#2 timeout
    req#4 timeout
    req#6 timeout
    req#8 timeout
    req#10 timeout
    req#12 timeout
    req#14 timeout
    req#16 timeout
    req#18 timeout
    req#20 timeout
    req#22 timeout
    req#24 timeout
    req#26 timeout
    req#28 timeout
    req#28 close
    req#26 close
    req#24 close
    req#22 close
    req#20 close
    req#18 close
    req#16 close
    req#14 close
    req#12 close
    req#10 close
    req#8 close
    req#6 close
    req#4 close
    req#2 close
    req#0 close
    req#29 error
    req#29 close
    req#27 error
    req#27 close
    req#25 error
    req#25 close
    req#23 error
    req#23 close
    req#21 error
    req#21 close
    req#19 error
    req#19 close
    req#17 error
    req#17 close
    req#15 error
    req#15 close
    req#13 error
    req#13 close
    req#11 error
    req#11 close
    req#9 error
    req#9 close
    req#7 error
    req#7 close
    req#5 error
    req#5 close
    req#3 error
    req#3 close
    req#1 error
    req#1 close
    done=45 sent=30
    assert.js:80
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: 45 strictEqual 30
        at process.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-client-timeout-agent.js:93:10)
        at process.emit (events.js:187:15)
  ...

mhdawson · 2018-04-05T23:18:10Z

Ci again: https://ci.nodejs.org/job/node-test-pull-request/14083/console

kfarnung · 2018-04-07T00:45:38Z

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

* Updated tests for `Number` and `int32_t` * Added new tests for `int64_t` * Updated N-API `int64_t` behavior to return zero for all non-finite numbers * Clarified the documentation for these calls. PR-URL: #19402 Refs: nodejs/node-chakracore#500 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

kfarnung · 2018-04-08T18:44:49Z

Landed in c529168

* Updated tests for `Number` and `int32_t` * Added new tests for `int64_t` * Updated N-API `int64_t` behavior to return zero for all non-finite numbers * Clarified the documentation for these calls. PR-URL: nodejs#19402 Refs: nodejs/node-chakracore#500 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* Updated tests for `Number` and `int32_t` * Added new tests for `int64_t` * Updated N-API `int64_t` behavior to return zero for all non-finite numbers * Clarified the documentation for these calls. PR-URL: #19402 Refs: nodejs/node-chakracore#500 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos · 2018-04-12T15:45:37Z

Landed on v9.x-staging in a63cdee with a minor change to make the test for NaN work (assert.strictEqual does not support NaN outside of master).

* Updated tests for `Number` and `int32_t` * Added new tests for `int64_t` * Updated N-API `int64_t` behavior to return zero for all non-finite numbers * Clarified the documentation for these calls. PR-URL: nodejs#19402 Refs: nodejs/node-chakracore#500 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* Updated tests for `Number` and `int32_t` * Added new tests for `int64_t` * Updated N-API `int64_t` behavior to return zero for all non-finite numbers * Clarified the documentation for these calls. Backport-PR-URL: #19447 PR-URL: #19402 Refs: nodejs/node-chakracore#500 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* Updated tests for `Number` and `int32_t` * Added new tests for `int64_t` * Updated N-API `int64_t` behavior to return zero for all non-finite numbers * Clarified the documentation for these calls. Backport-PR-URL: #19265 PR-URL: #19402 Refs: nodejs/node-chakracore#500 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

kfarnung self-assigned this Mar 17, 2018

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Mar 17, 2018

kfarnung force-pushed the napi_int64 branch from 2498968 to 1ed7b68 Compare March 17, 2018 04:43

mhdawson reviewed Mar 20, 2018

View reviewed changes

kfarnung force-pushed the napi_int64 branch from 1ed7b68 to d72d3c5 Compare March 20, 2018 18:04

boingoing approved these changes Mar 20, 2018

View reviewed changes

kfarnung force-pushed the napi_int64 branch from d72d3c5 to f95d1f9 Compare March 21, 2018 18:35

kfarnung force-pushed the napi_int64 branch from f95d1f9 to f20492a Compare March 22, 2018 22:47

gabrielschulhof reviewed Mar 27, 2018

View reviewed changes

kfarnung force-pushed the napi_int64 branch from f20492a to 1c73abf Compare March 27, 2018 22:11

kfarnung force-pushed the napi_int64 branch 2 times, most recently from 1b85199 to c5953f1 Compare March 29, 2018 18:25

mhdawson approved these changes Apr 4, 2018

View reviewed changes

jasnell approved these changes Apr 4, 2018

View reviewed changes

kfarnung closed this Apr 8, 2018

kfarnung force-pushed the napi_int64 branch from c5953f1 to c529168 Compare April 8, 2018 18:44

kfarnung deleted the napi_int64 branch April 8, 2018 18:44

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

Uh oh!

Conversation

kfarnung commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

kfarnung commented Mar 17, 2018

Uh oh!

kfarnung commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

kfarnung Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

kfarnung commented Mar 20, 2018

Uh oh!

kfarnung commented Mar 20, 2018

Uh oh!

boingoing left a comment

Choose a reason for hiding this comment

Uh oh!

kfarnung commented Mar 21, 2018

Uh oh!

kfarnung commented Mar 22, 2018

Uh oh!

kfarnung commented Mar 27, 2018

Uh oh!

kfarnung commented Mar 27, 2018

Uh oh!

gabrielschulhof Mar 27, 2018

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Mar 27, 2018

Choose a reason for hiding this comment

Uh oh!

kfarnung commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Apr 4, 2018

Uh oh!

kfarnung commented Apr 4, 2018

Uh oh!

mhdawson commented Apr 5, 2018

Uh oh!

kfarnung commented Apr 7, 2018

Uh oh!

kfarnung commented Apr 8, 2018

Uh oh!

targos commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kfarnung commented Mar 17, 2018 •

edited

Loading

kfarnung commented Mar 17, 2018 •

edited

Loading

kfarnung commented Mar 27, 2018 •

edited

Loading