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

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.

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

/cc @boingoing @digitalinfinity @MSLaguana

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/

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.

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

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

👍

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

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

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

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

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

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

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

LGTM

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

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

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

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>

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>

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

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

Uh oh!

Uh oh!

n-api: add more int64_t tests - #19402

n-api: add more int64_t tests#19402kfarnung wants to merge 0 commit intonodejs:masternodejs/node:masterfrom kfarnung:napi_int64kfarnung/node:napi_int64Copy head branch name to clipboard

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

n-api: add more int64_t tests#19402
kfarnung wants to merge 0 commit into
nodejs:masterfrom
kfarnung:napi_int64

kfarnung commented Mar 17, 2018 •
edited
Loading

kfarnung commented Mar 17, 2018 •
edited
Loading

kfarnung commented Mar 27, 2018 •
edited
Loading