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

JSPI: Pass all unit tests, remove stale PHP builds #1876

Merged
merged 5 commits into from
Oct 9, 2024

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Oct 9, 2024

Fixes the JSPI details overlooked in the original JSPI PR (#1867):

  • Correctly define the PLAYGROUND_JSPI constant to ensure EM_ASYNC_JS is used in JSPI builds. It wasn't picked up before.
  • Provide -sJSPI_IMPORTS during the build to ensure WebAssembly waits for any promises returned by js_open_process and other JavaScript functions.
  • Runs the CI tests using Node v23 nightly
  • Solves a few small issues – look around the diff for more details

Testing instructions

  • CI checks should pass
  • All unit tests should pass locally in Node.js v23 nightly:
$ node-v23.0.0-nightly2024100909d10b50dc-darwin-x64/bin/node --experimental-wasm-jspi --experimental-wasm-stack-switching --expose-gc node_modules/nx/bin/nx run php-wasm-node:test

Fixes JSPI details overlooked in the original JSPI PR (#1867):

* Correctly define the PLAYGROUND_JSPI constant to ensure EM_ASYNC_JS is
  used in JSPI builds. It wasn't picked up before.
* Provide -sJSPI_IMPORTS during the build to ensure WebAssembly waits
  for any promises returned by js_open_process and other JavaScript
  functions.
* Runs the CI tests using Node v23 nightly
* Solves a few small issues – look around the diff for more details

 ## Testing instructions

CI unit tests should pass
@adamziel adamziel changed the title JSPI: Pass all unit tests, run them in CI JSPI: Pass all unit tests, run them in CI, remove stale PHP builds Oct 9, 2024
@adamziel adamziel mentioned this pull request Oct 9, 2024
@adamziel
Copy link
Collaborator Author

adamziel commented Oct 9, 2024

I got the failures down to the ones below. The test process crashes in CI for some reason so I won't enable the automated JSPI test runner yet.

 ❯ src/test/php-networking.spec.ts  (63 tests | 4 failed) 29341ms
   ❯ src/test/php-networking.spec.ts > PHP 8.3 > cURL > should support HTTPS requests
     → Test timed out in 1000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".
   ❯ src/test/php-networking.spec.ts > PHP 8.2 > cURL > should support HTTPS requests
     → Test timed out in 1000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".
   ❯ src/test/php-networking.spec.ts > PHP 8.0 > cURL > should support HTTPS requests
     → Test timed out in 1000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".
   ❯ src/test/php-networking.spec.ts > PHP 7.1 > cURL > should support HTTPS requests
     → Test timed out in 1000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".
stderr | src/test/php.spec.ts > PHP 8.3 > onMessage > should return null when JS message handler throws an error
Failure!
 ✓ src/test/php-request-handler.spec.ts  (470 tests) 90174ms
 ❯ src/test/php-crash.spec.ts  (27 tests | 6 failed) 128512ms
   ❯ src/test/php-crash.spec.ts > PHP 7.0 – process crash > Does not crash due to an unhandled Asyncify error
     → php.run should have thrown an error
   ❯ src/test/php-crash.spec.ts > PHP 7.1 – process crash > Does not crash due to an unhandled Asyncify error
     → php.run should have thrown an error
   ❯ src/test/php-crash.spec.ts > PHP 7.3 – process crash > Does not crash due to an unhandled Asyncify error
     → php.run should have thrown an error
   ❯ src/test/php-crash.spec.ts > PHP 7.4 – process crash > Does not crash due to an unhandled Asyncify error
     → php.run should have thrown an error
   ❯ src/test/php-crash.spec.ts > PHP 8.0 – process crash > Does not crash due to an unhandled Asyncify error
     → php.run should have thrown an error
   ❯ src/test/php-crash.spec.ts > PHP 8.1 – process crash > Does not crash due to an unhandled Asyncify error
     → php.run should have thrown an error

@adamziel adamziel changed the title JSPI: Pass all unit tests, run them in CI, remove stale PHP builds JSPI: Pass most unit tests, remove stale PHP builds Oct 9, 2024
@adamziel
Copy link
Collaborator Author

adamziel commented Oct 9, 2024

All JSPI unit tests pass now, although the CI runner crashes so I'll keep it disabled.

@adamziel adamziel changed the title JSPI: Pass most unit tests, remove stale PHP builds JSPI: Pass all unit tests, remove stale PHP builds Oct 9, 2024
@adamziel adamziel merged commit 0385459 into trunk Oct 9, 2024
5 checks passed
@adamziel adamziel deleted the jspi-async-imports branch October 9, 2024 17:26
@brandonpayton
Copy link
Member

@adamziel, I am still getting the asyncify failure-related errors when running via command line.

⎯⎯⎯⎯⎯⎯⎯ Failed Tests 6 ⎯⎯⎯⎯⎯⎯⎯
 FAIL  src/test/php-crash.spec.ts > PHP 7.0 – process crash > Does not crash due to an unhandled Asyncify error 
 FAIL  src/test/php-crash.spec.ts > PHP 7.1 – process crash > Does not crash due to an unhandled Asyncify error 
 FAIL  src/test/php-crash.spec.ts > PHP 7.3 – process crash > Does not crash due to an unhandled Asyncify error 
 FAIL  src/test/php-crash.spec.ts > PHP 7.4 – process crash > Does not crash due to an unhandled Asyncify error 
 FAIL  src/test/php-crash.spec.ts > PHP 8.0 – process crash > Does not crash due to an unhandled Asyncify error 
 FAIL  src/test/php-crash.spec.ts > PHP 8.1 – process crash > Does not crash due to an unhandled Asyncify error 
AssertionError: php.run should have thrown an error 

This is with a local build of nodejs v23, and the command is the same as the one in the test instructions. Any thoughts on this? I'm not sure how that test is supposed to pass when using JSPI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants