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

Convert all remaining wasm worker tests to btest_exit. NFC #22776

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 22, 2024

Amongst other things this allows all these test to be run under node.

Amongst other things this allows all these test to be run under node.
@sbc100 sbc100 requested review from kripken and juj October 22, 2024 20:29
@@ -91,6 +96,7 @@ int main() {
#ifdef __EMSCRIPTEN_WASM_WORKERS__
emscripten_wasm_worker_t worker = emscripten_create_wasm_worker(stack, sizeof(stack));
emscripten_wasm_worker_post_function_v(worker, worker_main);
emscripten_runtime_keepalive_push();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slightly worry about converting all the tests to this form, as our users may be writing in the simpler way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I can make this part implicit by having post_function itself keep the runtime alive.

The other part of replacing REPORT_RESULT with normal exit I hope/think is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually post_function doesn't track when job is done so that don't work.

I think the benefit here of making these test runnable under node, along with using a more normal exit-status-based test setup outweighs the downside here of having code that a user might not write. Note that emscripten_runtime_keepalive_push simply does nothing in default build without -sEXIT_RUNTIME since the runtime always stays alive regardless in the default mode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, maybe we can land just a small subset for now? #22777

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be ok landing the whole thing, so long as we keep one test in the old form (something like a "wasm workers without exit runtime" test).

sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 22, 2024
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 22, 2024
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 24, 2024
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 25, 2024
sbc100 added a commit that referenced this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants