GeezerVison

Copy link

Copy Markdown

Member

joyeecheung force-pushed the fix-test-cannot-run branch from 525308f to e3b6d32 Compare February 27, 2024 15:22

targos approved these changes Feb 27, 2024

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung force-pushed the fix-test-cannot-run branch from e3b6d32 to 4165dca Compare February 27, 2024 15:36

richardlau approved these changes Feb 27, 2024

View reviewed changes

marco-ippolito approved these changes Feb 27, 2024

View reviewed changes

17:10:21 Failed to build addon in c:\workspace\node-test-binary-windows-native-suites\node\test\js-native-api\test_cannot_run_js:
17:10:21 Building the projects in this solution one at a time. To enable parallel build, please add the "-m" switch.

17:10:21   test_cannot_run_js.c

17:10:21 c:\workspace\node-test-binary-windows-native-suites\node\test\js-native-api\test_cannot_run_js\test_cannot_run_js.c(28,1): error C2121: '#': invalid character: possibly the result of a macro expansion [c:\workspace\node-test-binary-windows-native-suites\node\test\js-native-api\test_cannot_run_js\build\test_cannot_run_js.vcxproj]

legendecas approved these changes Feb 29, 2024

View reviewed changes

diff --git a/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
index 925a7117d06..19cc7ae9058 100644
--- a/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
+++ b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
@@ -21,12 +21,13 @@ static void Finalize(napi_env env, void* data, void* hint) {
   // or during environment shutdown (where napi_cannot_run_js or
   // napi_pending_exception is returned). This is not deterministic from
   // the point of view of the addon.
-  NODE_API_NOGC_ASSERT_RETURN_VOID(
 #ifdef NAPI_EXPERIMENTAL
+  NODE_API_NOGC_ASSERT_RETURN_VOID(
       result == napi_cannot_run_js || result == napi_ok,
       "getting named property from global in finalizer should succeed "
       "or return napi_cannot_run_js");
 #else
+  NODE_API_NOGC_ASSERT_RETURN_VOID(
       result == napi_pending_exception || result == napi_ok,
       "getting named property from global in finalizer should succeed "
       "or return napi_pending_exception");

richardlau approved these changes Mar 1, 2024

View reviewed changes

Previously the test assumes that when the queued finalizer is run,
it must be run at a point where env->can_call_into_js() is false
(typically, during Environment shutdown), which is not certain.
If GC kicks in early and the second pass finalizer is queued before
the event loop runs the check callbacks, the finalizer would then
be called in check callbacks (via native immediates), where
the finalizer can still call into JS. Essentially, addons can't
make assumptions about where the queued finalizer would be called.
This patch updates the assertions in the test to account for that.

PR-URL: #51898
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>

Previously the test assumes that when the queued finalizer is run,
it must be run at a point where env->can_call_into_js() is false
(typically, during Environment shutdown), which is not certain.
If GC kicks in early and the second pass finalizer is queued before
the event loop runs the check callbacks, the finalizer would then
be called in check callbacks (via native immediates), where
the finalizer can still call into JS. Essentially, addons can't
make assumptions about where the queued finalizer would be called.
This patch updates the assertions in the test to account for that.

PR-URL: #51898
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>

Previously the test assumes that when the queued finalizer is run,
it must be run at a point where env->can_call_into_js() is false
(typically, during Environment shutdown), which is not certain.
If GC kicks in early and the second pass finalizer is queued before
the event loop runs the check callbacks, the finalizer would then
be called in check callbacks (via native immediates), where
the finalizer can still call into JS. Essentially, addons can't
make assumptions about where the queued finalizer would be called.
This patch updates the assertions in the test to account for that.

PR-URL: #51898
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>