Skip to content

Run Emscripten tests in a browser #294

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

Conversation

mcbarton
Copy link
Collaborator

@mcbarton mcbarton commented May 1, 2025

Description

Please include a summary of changes, motivation and context for this PR.

This PR adds the ability to run the Emscripten tests in a browser

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.78%. Comparing base (53d1686) to head (98dd563).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #294   +/-   ##
=======================================
  Coverage   81.78%   81.78%           
=======================================
  Files          20       20           
  Lines         950      950           
  Branches       87       87           
=======================================
  Hits          777      777           
  Misses        173      173           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcbarton mcbarton force-pushed the Run-emscripten-tests-in-browser-as-well-as-node branch 4 times, most recently from 32c6236 to 93dd06f Compare May 1, 2025 18:03
@mcbarton
Copy link
Collaborator Author

mcbarton commented May 1, 2025

Running the tests makes use of emrun which comes with Emscripten. The documentation for emrun can be found here https://emscripten.org/docs/compiling/Running-html-files-with-emrun.html

@mcbarton mcbarton force-pushed the Run-emscripten-tests-in-browser-as-well-as-node branch from 944d734 to d72f58a Compare May 1, 2025 18:18
@mcbarton mcbarton requested review from vgvassilev and anutosh491 May 1, 2025 18:59
@mcbarton mcbarton force-pushed the Run-emscripten-tests-in-browser-as-well-as-node branch from d72f58a to 2d3ebbf Compare May 9, 2025 17:09
@anutosh491
Copy link
Collaborator

Hey @DerThorsten

I would really appreciate your expertise here. Please let us if this should be our GoTo method to address the problem statement here.

The contributor has made quite some comments describing his approach here.

Feel free to let us know if we can do better. Thanks !

@vgvassilev
Copy link
Contributor

IIRC, @mcbarton mentioned he discussed that at the emscripten discord or similar.

@mcbarton
Copy link
Collaborator Author

emscripten-core/emscripten#24273 - This is where I discussed it previously with Emscripten maintainers. I asked the Emscripten maintainers about this PR, and they said that emrun is a reasonable way to run Emscripten tests in a browser, and that my emrun command looks reasonable.

@mcbarton mcbarton force-pushed the Run-emscripten-tests-in-browser-as-well-as-node branch 2 times, most recently from dffa830 to 4912297 Compare May 21, 2025 09:28
@mcbarton
Copy link
Collaborator Author

I've added the PR comments, as comments in the files as requested here #294 (comment)

@mcbarton mcbarton force-pushed the Run-emscripten-tests-in-browser-as-well-as-node branch 4 times, most recently from a2d3dc8 to e295736 Compare May 23, 2025 12:28
@mcbarton mcbarton force-pushed the Run-emscripten-tests-in-browser-as-well-as-node branch from e295736 to 98dd563 Compare May 24, 2025 17:10
@mcbarton
Copy link
Collaborator Author

@anutosh491 pinging you on this PR, since you said it would go in by the end of today in the Discord chat with @vgvassilev

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Lgtm!

@anutosh491
Copy link
Collaborator

pinging you on this PR,

Writing my thoughts.

@mcbarton
Copy link
Collaborator Author

@vgvassilev It was my understanding that it had already been decided on our Discord group chat that this would go in today. Are you happy for me to merge this, and I can deal with any of any @anutosh491 thoughts post merge, or would you like me to wait?

@anutosh491
Copy link
Collaborator

anutosh491 commented May 24, 2025

Hey @SylvainCorlay ,

I went through this PR today and I don't have any strong opinions here (probably I'm not educated enough or falling short on time to educate myself with all going on here)

I tagged @DerThorsten couple days back but haven't heard from him.

So do you have any opinions if this is how we should test stuff in the browser ?
I'll let you move this in if you think we're good (Vassil says we're good)

Some surface level reviews

  1. Not a fan of having explanations/long writeups in the actions file. If at all I would like references/docs than explanation for some thing. If in question we can get back to the PR as to why something was done. We all probably lack experiences that a technical writer would have, so framing explanations more than a couple lines is not really what I'd prefer.

  2. Not a fan of having users follow a huge set of instructions in the readme for building/testing (1 configure command, 1 build command, 1 test command is all I would follow). If at all we want to provide a huge set of instructions for testing ....... I've seen quite some repos just put all of it in a build script and ask the user to run the build script (like ./build_xeus_cpp_test.sh)

EDIT : Also after build and install, running jupyterlite comes way below (I would like a user to try lite first, then test stuff if they really want to, let's have the complete build workflow first and then have the test workflow ?)

That being said, these are just my thoughts. If anyone thinks otherwise and can't wait on this, feel free to move it in.

@SylvainCorlay
Copy link
Collaborator

For in-browser testing, my initial thought was that we could use Galata for full integration tests with Jupyter, including visual non-regression tests, but I like this approach of running the standard C++ tests in the browser with emrun.

Galata-based integration tests could be added at a later stage, including for the native builds.

@mcbarton mcbarton merged commit 51aeae3 into compiler-research:main May 26, 2025
14 checks passed
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.

5 participants