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

n-api: add more int64_t tests #19402

Closed
wants to merge 0 commits into from
Closed

Conversation

kfarnung
Copy link
Contributor

@kfarnung kfarnung commented Mar 17, 2018

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 kfarnung self-assigned this Mar 17, 2018
@nodejs-github-bot 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
Copy link
Contributor Author

@kfarnung
Copy link
Contributor Author

kfarnung commented Mar 17, 2018

src/node_api.cc Outdated
if (std::isnan(doubleValue)) {
*result = 0;
} else {
if (std::isfinite(doubleValue)) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I forgot to do that.

@kfarnung
Copy link
Contributor Author

@kfarnung
Copy link
Contributor Author

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

Copy link
Contributor

@boingoing boingoing left a comment

Choose a reason for hiding this comment

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

👍

@kfarnung
Copy link
Contributor Author

@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
Copy link
Contributor Author

@kfarnung
Copy link
Contributor Author

@kfarnung
Copy link
Contributor Author

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

src/node_api.cc Outdated
// v8::Value::Int32Value() that converts NaN to 0. So special-case NaN here.
// v8::Value::IntegerValue() converts NaN, +Inf, and -Inf to INT64_MIN,
// inconsistent with v8::Value::Int32Value() which converts those values to 0.
// Special case all non-finite values match that behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@kfarnung
Copy link
Contributor Author

kfarnung commented Mar 27, 2018

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

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

mhdawson commented Apr 4, 2018

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

@kfarnung
Copy link
Contributor Author

kfarnung commented Apr 4, 2018

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
Copy link
Member

mhdawson commented Apr 5, 2018

@kfarnung
Copy link
Contributor Author

kfarnung commented Apr 7, 2018

kfarnung added a commit that referenced this pull request Apr 8, 2018
* 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@kfarnung kfarnung closed this Apr 8, 2018
@kfarnung
Copy link
Contributor Author

kfarnung commented Apr 8, 2018

Landed in c529168

@kfarnung kfarnung deleted the napi_int64 branch April 8, 2018 18:44
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
* 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2018
* 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos
Copy link
Member

targos commented Apr 12, 2018

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

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
* 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
* 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
MylesBorins pushed a commit that referenced this pull request May 1, 2018
* 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants