Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

Latest test-addon-napi test failing with node-chakracore #246

Closed
kunalspathak opened this issue May 17, 2017 · 4 comments · Fixed by #301
Closed

Latest test-addon-napi test failing with node-chakracore #246

kunalspathak opened this issue May 17, 2017 · 4 comments · Fixed by #301
Assignees
@kfarnung

Comments

@kunalspathak
Copy link
Member

Below 3 addon-napi test cases are failing. This issue is to track the fix and re-enable them.

test/addons-napi/test_async/test.js
test/addons-napi/test_make_callback_recurse/test.js
test/addons-napi/test_reference/test.js
  • Version:
  • Platform:
  • Subsystem:
jasongin added a commit to jasongin/node-chakracore that referenced this issue May 17, 2017
@jasongin
n-api: Fix test-addons-napi failures
a32568a 
 - Reimplement napi_make_callback to call node::MakeCallback using V8
   (chakrashim) types. We always knew the direct call to
   JsCallFunction() was incorrect there.
 - Refactor the reference test cases so each GC call happens after a
   setImmediate callback, because the GC often will not collect values
   that were just-allocated in the same event loop iteration. (One of
   the test cases already needed this for the newer V8 GC. For the
   ChakraCore GC, more of them do.)

Fixes: nodejs#246
jasongin added a commit to jasongin/node-chakracore that referenced this issue May 18, 2017
@jasongin
n-api: Fix test-addons-napi failures
bb7eef7 
 - Reimplement napi_make_callback to call node::MakeCallback using V8
   (chakrashim) types. We always knew the direct call to
   JsCallFunction() was incorrect there.
 - Refactor the reference test cases so each GC call happens after a
   setImmediate callback, because the GC often will not collect values
   that were just-allocated in the same event loop iteration. (One of
   the test cases already needed this for the newer V8 GC. For the
   ChakraCore GC, more of them do.)

Fixes: nodejs#246
jasongin added a commit to jasongin/node-chakracore that referenced this issue May 18, 2017
@jasongin
n-api: Fix test-addons-napi failures
f557b39 
 - Reimplement napi_make_callback to call node::MakeCallback using V8
   (chakrashim) types. We always knew the direct call to
   JsCallFunction() was incorrect there.
 - Refactor the reference test cases so each GC call happens after a
   setImmediate callback, because the GC often will not collect values
   that were just-allocated in the same event loop iteration. (One of
   the test cases already needed this for the newer V8 GC. For the
   ChakraCore GC, more of them do.)

Fixes: nodejs#246
@jasongin jasongin mentioned this issue May 18, 2017
Merged
3 tasks
jasongin added a commit to jasongin/node-chakracore that referenced this issue May 18, 2017
@jasongin
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 added a commit that referenced this issue May 18, 2017
@jasongin
n-api: Fix test-addons-napi failures (#249)
ff42f9d 
- 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
@kunalspathak
Copy link
Member Author

@jasongin - I think your PR still doesn't address test/addons-napi/test_reference/test.js, right?

@jasongin jasongin reopened this May 18, 2017
@jasongin
Copy link
Member

Right, reopening to track the remaining test_reference failure. For that I'll make a PR to node/master and we should re-enable the test when we merge the fix.

@kunalspathak
Copy link
Member Author

Great, Thanks!

@jasongin jasongin mentioned this issue May 19, 2017
Merged
3 tasks
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this issue May 23, 2017
@jasongin @kfarnung
n-api: Fix test-addons-napi failures (nodejs#249)
26992ab 
- 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
kfarnung added a commit to kfarnung/node-chakracore that referenced this issue May 23, 2017
@kfarnung
test: disabling N-API test
2e118ed 
The test failure is being tracked by nodejs#246
kfarnung added a commit to kfarnung/node-chakracore that referenced this issue May 23, 2017
@kfarnung
test: disabling N-API test
245e9f5 
The test failure is being tracked by nodejs#246
kfarnung added a commit to kfarnung/node-chakracore that referenced this issue May 23, 2017
@kfarnung
test: disabling N-API test
5e26434 
The test failure is being tracked by nodejs#246
kfarnung added a commit to kfarnung/node-chakracore that referenced this issue May 23, 2017
@kfarnung
test: disabling N-API test
31ee0c4 
The test failure is being tracked by nodejs#246

PR-URL: nodejs#258
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
jasongin added a commit to jasongin/nodejs that referenced this issue May 30, 2017
@jasongin
test: Make N-API weak-ref GC tests asynchronous
bb91879 
One of the N-API weak-reference test cases already had to be made
asynchronous to handle different behavior in a newer V8 version:
nodejs#12864
When porting N-API to Node-ChakraCore, we found more of the test
cases needed similar treatment:
nodejs/node-chakracore#246 So to make
thes tests more robust (and avoid having differences in the test code
for Node-ChakraCore), I am refactoring the tests in this file to
insert a `setImmedate()` callback before every call to `gc()` and
assertions about the effects of the GC.

PR-URL: nodejs#13121
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
jasnell pushed a commit to nodejs/node that referenced this issue Jun 5, 2017
@jasongin @jasnell
test: Make N-API weak-ref GC tests asynchronous
5369359 
One of the N-API weak-reference test cases already had to be made
asynchronous to handle different behavior in a newer V8 version:
#12864
When porting N-API to Node-ChakraCore, we found more of the test
cases needed similar treatment:
nodejs/node-chakracore#246 So to make
thes tests more robust (and avoid having differences in the test code
for Node-ChakraCore), I am refactoring the tests in this file to
insert a `setImmedate()` callback before every call to `gc()` and
assertions about the effects of the GC.

PR-URL: #13121
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@kfarnung kfarnung assigned kfarnung and unassigned jasongin Jun 14, 2017
@kfarnung
Copy link
Contributor

I'll take a look.

kfarnung added a commit to kfarnung/node-chakracore that referenced this issue Jun 15, 2017
@kfarnung
test: enable addons-napi/test_reference
9d981c7 
Fixed by upstream PR nodejs/node#13121

Resolves nodejs#246
@kfarnung kfarnung mentioned this issue Jun 15, 2017
Merged
2 tasks
kfarnung added a commit to kfarnung/node-chakracore that referenced this issue Jun 15, 2017
@kfarnung
test: enable addons-napi/test_reference
2890542 
Fixed by upstream PR nodejs/node#13121

Resolves nodejs#246

PR-URL: nodejs#301
Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this issue Jun 15, 2017
@kfarnung
test: enable addons-napi/test_reference
e72523b 
Fixed by upstream PR nodejs/node#13121

Resolves nodejs#246

PR-URL: nodejs#301
Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 10, 2018
@jasongin
test: Make N-API weak-ref GC tests asynchronous
8653a1b 
One of the N-API weak-reference test cases already had to be made
asynchronous to handle different behavior in a newer V8 version:
nodejs#12864
When porting N-API to Node-ChakraCore, we found more of the test
cases needed similar treatment:
nodejs/node-chakracore#246 So to make
thes tests more robust (and avoid having differences in the test code
for Node-ChakraCore), I am refactoring the tests in this file to
insert a `setImmedate()` callback before every call to `gc()` and
assertions about the effects of the GC.

PR-URL: nodejs#13121
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Apr 16, 2018
@jasongin @MylesBorins
test: Make N-API weak-ref GC tests asynchronous
652d321 
One of the N-API weak-reference test cases already had to be made
asynchronous to handle different behavior in a newer V8 version:
#12864
When porting N-API to Node-ChakraCore, we found more of the test
cases needed similar treatment:
nodejs/node-chakracore#246 So to make
thes tests more robust (and avoid having differences in the test code
for Node-ChakraCore), I am refactoring the tests in this file to
insert a `setImmedate()` callback before every call to `gc()` and
assertions about the effects of the GC.

Backport-PR-URL: #19447
PR-URL: #13121
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@kfarnung kfarnung

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: enable addons-napi/test_reference kfarnung/node-chakracore
3 participants
@kfarnung @kunalspathak @jasongin