n-api: Fix test-addons-napi failures #249

jasongin · 2017-05-18T19:26:17Z

Reimplement napi_make_callback() to call node::MakeCallback() using V8 (chakrashim) types. We always knew the direct call to JsCallFunction() was incorrect there.
Re-enable the two tests that were failing due to napi_make_callback()

Fixes: #246

Checklist

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

Affected core subsystem(s)

n-api

boingoing

Looks right to me, nice work on the test cases

digitalinfinity

LGTM too, although I wonder if the test updates should just be PR-ed to nodejs/node instead? I'd like it if we kept the test differences between node and node-chakracore to a minimum

jasongin · 2017-05-18T19:38:28Z

Oh, I need to update this PR to remove the skips that were put in for the failing tests.

I wonder if the test updates should just be PR-ed to nodejs/node instead?

I was thinking we would do that later. But instead we could just keep this test skipped until then.

- Reimplement napi_make_callback to call node::MakeCallback using V8 (chakrashim) types. We always knew the direct call to JsCallFunction() was incorrect there. - Re-enable the two tests that were failing due to napi_make_callback Fixes: nodejs#246

jasongin · 2017-05-18T20:02:59Z

I amended the PR to avoid changing the test JS file. I'll create a PR to node/master instead.

Also I removed the skips from the 2 tests that are now passing after the napi_make_callback() fix.

kunalspathak · 2017-05-18T22:46:31Z

- Reimplement napi_make_callback to call node::MakeCallback using V8 (chakrashim) types. We always knew the direct call to JsCallFunction() was incorrect there. - Re-enable the two tests that were failing due to napi_make_callback Fixes: nodejs#246

@dilijev

[1.8>1.9] [MERGE #4651 @dilijev] OS#14101048: Fixes #249: built-in functions should not have caller/arguments (and related fixes) Merge pull request #4651 from dilijev:func-restricted-props Replaces #4639 Several tests and test baselines needed to be updated to reflect the change in behavior. This change improves caller/arguments behavior without regressing any previous Chakra behavior. Chakra still differs from the spec and other engines on some aspects of caller/arguments. See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes #249 Fixes OS#14101048 --- Test262 results strictly improved. Using test262 commit tc39/test262@03da228: Previously 44130 passed, now 44138 passed out of 57102. (Duplicates below are strict and non-strict versions of the tests.) ``` +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js ``` Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>

@dilijev

[1.9>master] [1.8>1.9] [MERGE #4651 @dilijev] OS#14101048: Fixes #249: built-in functions should not have caller/arguments (and related fixes) Merge pull request #4651 from dilijev:func-restricted-props Replaces #4639 Several tests and test baselines needed to be updated to reflect the change in behavior. This change improves caller/arguments behavior without regressing any previous Chakra behavior. Chakra still differs from the spec and other engines on some aspects of caller/arguments. See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes #249 Fixes OS#14101048 --- Test262 results strictly improved. Using test262 commit tc39/test262@03da228: Previously 44130 passed, now 44138 passed out of 57102. (Duplicates below are strict and non-strict versions of the tests.) ``` +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js ``` Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>

@dilijev

[1.8>1.9] [MERGE #4651 @dilijev] OS#14101048: Fixes nodejs#249: built-in functions should not have caller/arguments (and related fixes) Merge pull request #4651 from dilijev:func-restricted-props Replaces #4639 Several tests and test baselines needed to be updated to reflect the change in behavior. This change improves caller/arguments behavior without regressing any previous Chakra behavior. Chakra still differs from the spec and other engines on some aspects of caller/arguments. See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes nodejs#249 Fixes OS#14101048 --- Test262 results strictly improved. Using test262 commit tc39/test262@03da228: Previously 44130 passed, now 44138 passed out of 57102. (Duplicates below are strict and non-strict versions of the tests.) ``` +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js ``` Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>

@dilijev

[1.8>1.9] [MERGE #4651 @dilijev] OS#14101048: Fixes nodejs#249: built-in functions should not have caller/arguments (and related fixes) Merge pull request #4651 from dilijev:func-restricted-props Replaces #4639 Several tests and test baselines needed to be updated to reflect the change in behavior. This change improves caller/arguments behavior without regressing any previous Chakra behavior. Chakra still differs from the spec and other engines on some aspects of caller/arguments. See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes nodejs#249 Fixes OS#14101048 --- Test262 results strictly improved. Using test262 commit tc39/test262@03da228: Previously 44130 passed, now 44138 passed out of 57102. (Duplicates below are strict and non-strict versions of the tests.) ``` +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js ``` Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>

@dilijev

[1.8>1.9] [MERGE #4651 @dilijev] OS#14101048: Fixes nodejs#249: built-in functions should not have caller/arguments (and related fixes) Merge pull request #4651 from dilijev:func-restricted-props Replaces #4639 Several tests and test baselines needed to be updated to reflect the change in behavior. This change improves caller/arguments behavior without regressing any previous Chakra behavior. Chakra still differs from the spec and other engines on some aspects of caller/arguments. See: https://tc39.github.io/ecma262/#sec-forbidden-extensions Fixes nodejs#249 Fixes OS#14101048 --- Test262 results strictly improved. Using test262 commit tc39/test262@03da228: Previously 44130 passed, now 44138 passed out of 57102. (Duplicates below are strict and non-strict versions of the tests.) ``` +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-arguments.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/Function/prototype/restricted-property-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-arguments.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js +PASS e:/dd/test262/test262/test/built-ins/ThrowTypeError/forbidden-caller.js ``` Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>

jasongin requested a review from boingoing May 18, 2017 19:26

boingoing approved these changes May 18, 2017

View reviewed changes

digitalinfinity approved these changes May 18, 2017

View reviewed changes

n-api: Fix test-addons-napi failures

fbfab4c

- Reimplement napi_make_callback to call node::MakeCallback using V8 (chakrashim) types. We always knew the direct call to JsCallFunction() was incorrect there. - Re-enable the two tests that were failing due to napi_make_callback Fixes: nodejs#246

jasongin force-pushed the napi_fixes branch from f557b39 to fbfab4c Compare May 18, 2017 20:01

kfarnung approved these changes May 18, 2017

View reviewed changes

jasongin merged commit ff42f9d into nodejs:xplat May 18, 2017

jasongin deleted the napi_fixes branch May 18, 2017 23:08

kfarnung mentioned this pull request May 30, 2017

node-chakracore-8.0.0-pre3 release proposal #272

Closed

3 tasks

kfarnung mentioned this pull request Jun 19, 2017

node-chakracore-8.1.2-pre release proposal #312

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: Fix test-addons-napi failures #249

n-api: Fix test-addons-napi failures #249

jasongin commented May 18, 2017 •

edited

Loading

boingoing left a comment

digitalinfinity left a comment

jasongin commented May 18, 2017

jasongin commented May 18, 2017

kunalspathak commented May 18, 2017

n-api: Fix test-addons-napi failures #249

n-api: Fix test-addons-napi failures #249

Conversation

jasongin commented May 18, 2017 • edited Loading

Checklist

Affected core subsystem(s)

boingoing left a comment

Choose a reason for hiding this comment

digitalinfinity left a comment

Choose a reason for hiding this comment

jasongin commented May 18, 2017

jasongin commented May 18, 2017

kunalspathak commented May 18, 2017

jasongin commented May 18, 2017 •

edited

Loading