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

Flaky js-native-api/test_cannot_run_js/test #48180

Closed
kvakil opened this issue May 25, 2023 · 3 comments · Fixed by #48181 or #51898
Closed

Flaky js-native-api/test_cannot_run_js/test #48180

kvakil opened this issue May 25, 2023 · 3 comments · Fixed by #48181 or #51898
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. linux Issues and PRs related to the Linux platform. macos Issues and PRs related to the macOS platform / OSX. node-api Issues and PRs related to the Node-API.

Comments

@kvakil
Copy link
Contributor

kvakil commented May 25, 2023

Test

js-native-api/test_cannot_run_js/test

Platform

Linux ARM64, Linux x64, macOS ARM64, macOS x64

Console output

crashed (-6)
---
duration_ms: 509.947
exitcode: -6
severity: crashed
...

Build links

See reliability report: nodejs/reliability#576

Here's one example: https://ci.nodejs.org/job/node-test-binary-armv7l/5897/RUN_SUBSET=native,nodes=ubuntu2004-armv7l/testReport/junit/js-native-api/test_cannot_run_js/test/

Additional information

Newly added in #47986

Probably comes from this abort statement:

Could not reproduce locally with test.py. Weirdly it reproduces every time when run without test.py:

./node test/js-native-api/test_cannot_run_js/test.js 
@kvakil kvakil added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label May 25, 2023
@github-actions github-actions bot added linux Issues and PRs related to the Linux platform. macos Issues and PRs related to the macOS platform / OSX. labels May 25, 2023
kvakil added a commit to kvakil/node that referenced this issue May 25, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the nodejs#1 failing JS test in the most recent reliability
report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
kvakil added a commit to kvakil/node that referenced this issue May 25, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
nodejs-github-bot pushed a commit that referenced this issue May 27, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: #48180
Refs: #47986
Refs: nodejs/reliability#576
PR-URL: #48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this issue May 30, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: #48180
Refs: #47986
Refs: nodejs/reliability#576
PR-URL: #48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: #48180
Refs: #47986
Refs: nodejs/reliability#576
PR-URL: #48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
PR-URL: nodejs#48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
PR-URL: nodejs#48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
PR-URL: nodejs#48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@richardlau richardlau reopened this Feb 26, 2024
@richardlau
Copy link
Member

This issue should not have been closed -- #48181 merely marked the test as flaky.

@abmusse
Copy link
Contributor

abmusse commented Feb 27, 2024

I've added a log to check what the actual and expected values are before we abort.

This was done on IBM i:

if (napi_get_global(env, &global) != napi_ok) abort();
if (napi_get_named_property(env, global, "setTimeout", &set_timeout) !=
expected_status)
abort();
free(ref);

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 c495f8780d..3ed3ee97fb 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
@@ -14,9 +14,12 @@ static void Finalize(napi_env env, void* data, void* hint) {
 
   if (napi_delete_reference(env, *ref) != napi_ok) abort();
   if (napi_get_global(env, &global) != napi_ok) abort();
-  if (napi_get_named_property(env, global, "setTimeout", &set_timeout) !=
-      expected_status)
+
+  napi_status actual_status = napi_get_named_property(env, global, "setTimeout", &set_timeout);
+  if (actual_status != expected_status){
+    fprintf(stderr, "actual_status (%d) != expected_status(%d)\n", actual_status, expected_status);
     abort();
+  }
   free(ref);
 }
$ ./out/Release/node test/js-native-api/test_cannot_run_js/test.js 
actual_status (0) != expected_status(10)
IOT/Abort trap (core dumped)

Looks like the actual napi_status value is 0 but the expected status is 10. 🤔

From the docs napi_status is a enum an 0 should map to -> napi_ok and 10 would map to -> napi_pending_exception

ref: https://nodejs.org/api/n-api.html#napi_status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. linux Issues and PRs related to the Linux platform. macos Issues and PRs related to the macOS platform / OSX. node-api Issues and PRs related to the Node-API.
Projects
None yet
5 participants
@richardlau @legendecas @kvakil @abmusse and others