-
Notifications
You must be signed in to change notification settings - Fork 76
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
ci: add job to test with Emscripten #1272
Conversation
I think this is starting to be in good shape, so it would be nice to get some feedback. There are quite a few things that changed since my initial comment.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks! After merging this, I'll add "Test build / pyodide-build" to the list of required tests.
My only comment about the code: sys.platform == "emscripten"
appears in multiple places; can this become a global boolean in _util.py? There aren't (currently) any examples in Uproot, but here are some examples of this in Awkward:
This is useful because there can sometimes be multiple ways to check which platform something is on (sys.platform
versus os.name
, maybe others), and some method may be found to be better in the future. Using a global boolean makes this a one-time fix.
I'm glad this is using Selenium and a separate tests-wasm directory. ✓
Please merge this PR when you're done and remind me to make the new test required if I forget. Thanks!
Thank you Jim, that's a great suggestion. I introduced
|
This PR resumes the work started in #1026. The package is installed and used with
pyodide
, just how it was done in #1026. I considered switching to usingemscripten-forge
andxeus-python
since there are some significant advantages for deployment, but currently it is much easier to set up these tests withpyodide
.The main difficulty with the tests are network requests and thread spawning. Network issues don't end up being as bad for deployments since often files are hosted on the same server/website (and there's ways to get around CORS protections), so I'll see if I can get some very simple http test working. As for thread spawning, it's more of an issue since there are multiple functions that try to spawn threads without being explicit that they are doing so. Currently WASM doesn't support multiple threads, so various things don't work.
I hand-picked a few test files that work, but probably the best approach will be to introduce a new
pytest
marker to indicate which tests should be attempted. And maybe it would be good to decide which subset of the functionality should be available in WASM and write some simple tests for it to make it more explicit.