Skip to content

[ php-wasm ] Add test-unit-jspi in CI workflow #2285

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Jul 4, 2025
Merged

[ php-wasm ] Add test-unit-jspi in CI workflow #2285

merged 13 commits into from
Jul 4, 2025

Conversation

mho22
Copy link
Collaborator

@mho22 mho22 commented Jun 18, 2025 •
edited
Loading

Motivation for the change, related issues

Based on this comment

This enables the JSPI mode tests alongside test-unit-asyncify in CI workflow.

Implementation details

Duplication of the test-unit-asyncify tests. With two major differences :

  1. JSPI Unit Tests need Node 23 to use JSPI mode.
  2. JSPI Unit Tests need a dedicated Vite config file to add the JSPI options:
execArgv: [
	'--expose-gc',
	'--experimental-wasm-stack-switching',
	'--experimental-wasm-jspi',
],
  1. Dispose php runtime with php.exit() after each test.

Testing Instructions (or ideally a Blueprint)

CI

@mho22
Copy link
Collaborator Author

mho22 commented Jun 18, 2025

Well, only two test-unit-jspi failed tests. I didn't expect that.

@adamziel
Copy link
Collaborator

PHP 8.1 > Startup sequence > Should have access to raw request data via the php://input stream 90ms
→ expected 'Dӷ\u0000Dӷ\u0000"bar"}' to deeply equal '{"foo": "bar"}'

this is a genuine failure

PHP 7.3 – process crash > Does not crash due to an unhandled Asyncify error 613ms
→ php.run should have thrown an error or caused an unhandled rejection

This can be disabled when we're running JSPI

@mho22
Copy link
Collaborator Author

mho22 commented Jun 19, 2025

On it.

@adamziel
Copy link
Collaborator

Is anything missing here?

@mho22
Copy link
Collaborator Author

mho22 commented Jun 20, 2025 •
edited
Loading

I'm a bit skeptical since the pull request was resolved unusually smoothly, with only two minor issues.

But, I think everything is in place. 😄

@adamziel
Copy link
Collaborator

Haha sometimes things just work :) sort out the description and merge

@mho22
Copy link
Collaborator Author

mho22 commented Jun 23, 2025

Well, I guess now we are talking about errors.

adamziel
adamziel reviewed Jun 24, 2025
View reviewed changes
@adamziel
Copy link
Collaborator

I'd guess the asyncify tests in trunk were somehow adjusted to the multi-worker setup and JSPI tests in this branch were not. Resolving this may potentially be as easy as updating a config flag somewhere.

@mho22
Copy link
Collaborator Author

mho22 commented Jun 24, 2025 •
edited
Loading

@brandonpayton I certainly don't understand everything here so I wanted to know your opinion on this. I see in the php/Dockerfile that the phpwasm-emscripten-library-file-locking-for-node.js file is specific to JSPI mode.

if [ "$EMSCRIPTEN_ENVIRONMENT" = "node" ] && [ "$WITH_JSPI" = "yes" ]; then \
	PLATFORM_SPECIFIC_ARGS="$PLATFORM_SPECIFIC_ARGS -DPHP_WASM_FILE_LOCKING_SUPPORT=1"; \
	PLATFORM_SPECIFIC_ARGS="$PLATFORM_SPECIFIC_ARGS --js-library /root/phpwasm-emscripten-library-file-locking-for-node.js"; \
fi; \

This is probably why when I run the tests in jspi mode some tests are failing. But does it mean that the asyncify tests are successful because they don't have that library specified ?

I also don't know where the file-lock-manager-for-node.spec.ts test file is run.

@adamziel adamziel mentioned this pull request Jun 24, 2025
Merged
adamziel added a commit that referenced this pull request Jun 25, 2025
@adamziel
PHP Node: Only consider NODEFS to be a shared filesystem (#2300)
bdf820e 
## Motivation for the change, related issues

#2231 overrides
`FS.hashAddNode` with `function hashAddNodeIfNotSharedFS(node)` where
additional logic applies if `is_shared_fs_node(node)` is true. Only
NODEFS nodes were supposed to be considered as coming from a shared fs.
Unfortunately, the internal logic of `is_shared_fs_node()` also returned
true for MEMFS nodes. This caused a FS error 44 for the following
operation where `runtime2` attempts to create a directory in a
`/wordpress` directory mounted from `runtime1`:

```ts
import { loadNodeRuntime } from "@php-wasm/node";
import { getLoadedRuntime } from "@php-wasm/universal";

const opts = {
	emscriptenOptions: { ENV: { DOCROOT: '/wordpress' } }
};
const runtime1 = getLoadedRuntime(await loadNodeRuntime('8.3', opts));
runtime1.FS.mkdir("/wordpress");

const runtime2 = getLoadedRuntime(await loadNodeRuntime('8.3', opts));
runtime2.FS.mkdir("/wordpress");

runtime2.FS.mount(
	runtime2.PROXYFS,
	{ root: '/wordpress', fs: runtime1.FS },
	'/wordpress'
);

// This works:
// runtime1.FS.mkdir("/wordpress/wp-content");

// This doesn't:
runtime2.FS.mkdir("/wordpress/wp-content");
```

Specifically, the FS error 44 was triggered inside `is_shared_fs_node()`
when calling NODEFS operations on these non-NODEFS nodes.

## Implementation details

Adds a check confirming the shared node comes from NODEFS.

## Testing Instructions (or ideally a Blueprint)

* Confirm the reproduction above works without errors.
* Once #2285 lands, we'll be able to add a unit test

cc @brandonpayton
@mho22 mho22 mentioned this pull request Jul 1, 2025
Merged
@mho22 mho22 changed the title [ php-wasm] Add test-unit-jspi in CI workflow [ php-wasm ] Add test-unit-jspi in CI workflow Jul 1, 2025
@mho22 mho22 force-pushed the test-unit-jspi branch 3 times, most recently from e035bc0 to eac6b14 Compare July 2, 2025 12:53
@mho22 mho22 force-pushed the test-unit-jspi branch 14 times, most recently from afbf20f to 9f02bfc Compare July 2, 2025 21:50
@mho22 mho22 force-pushed the test-unit-jspi branch from 9f02bfc to fb04769 Compare July 2, 2025 22:10
@mho22 mho22 force-pushed the test-unit-jspi branch from 9f7298a to eab674c Compare July 3, 2025 11:08
@adamziel
Copy link
Collaborator

adamziel commented Jul 4, 2025

This looks good, it only needs a rebase

@mho22
Copy link
Collaborator Author

mho22 commented Jul 4, 2025

Yep! I am on it.

I've separated the global tests from the php-wasm-node tests. Due to the specific nature of the following tests:

test-php-file-get-contents
test-php-fopen
test-php-fsockopen
test-php-gethostbyname
test-php-mysqli
test-php-sqlite3

They now have their own dedicated job.
Additionally, disposing of the PHP runtime after each test helped me clean up a lot of code and significantly reduce the test file size.

@mho22
Copy link
Collaborator Author

mho22 commented Jul 4, 2025

@adamziel You approved this pull request two weeks ago, before many changes. Do you still approve it? Can I squash and merge?

@adamziel adamziel merged commit 5f296a6 into WordPress:trunk Jul 4, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamziel adamziel adamziel approved these changes

Assignees
No one assigned
Labels
[Feature] GitHub integration [Package][@php-wasm] Node Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mho22 @adamziel