-
Notifications
You must be signed in to change notification settings - Fork 561
adding a Wasm test environment #4308
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
94d1fcc to
c618a88
Compare
djspiewak
left a 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.
I'm definitely in favor of adding a WASM environment to test in but there's a lot to address here. Several things were changed unnecessarily (notably scalatest).
Additionally, this would need to be reflected in a new GHA matrix entry in order to be tested in CI
|
@djspiewak thanks for suggestions, i'll make sure to fix issues based upon your suggestions. My current motivation is to make sure that every tests atleast work on wasm then i can move forward to change based on the projects requirements.
in my attempt to port i am facing this issue for |
We don't know either, because Wasm is new and experimental :) instead of rewriting tests to try and fix stuff, it's okay to push code with tests that don't work because then we can investigate together why they are broken. |
|
To add to what Arman said, it's very plausible that the exception you're seeing is coming from a bug in the WASM support in Scala.js itself. The only way to figure that out is to get the code shared amongst all of us, along with the stack trace, so we can try peeling back the veil and seeing if the compiler is actually just doing the wrong thing. Popping back up again, like I said, the right way to do this is by adding a sealed abstract class JSEnv
object JSEnv {
case object Firefox extends JSEnv
case object Chrome extends JSEnv
case object NodeJS extends JSEnv
}Add a case for |
this is what i was looking into, in my trial to port and fix the log errors for that issue. I came across some tests inside cats-effect repo which just exists (like even if they are not causing an issue in the repo they have 0 actual tests to pass they are just existing) for an also thanks for the explanation @djspiewak i appreciate your efforts, i'll make sure to try |
build.sbt
Outdated
| ThisBuild / jsEnv := { | ||
| useJSEnv.value match { | ||
| case NodeJS => new NodeJSEnv(NodeJSEnv.Config().withSourceMap(true)) | ||
| case NodeJS => nodeJSWasmEnv |
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.
I am not sure about this, if it is valid (shall we not change this parameter?)
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.
I definitely would not do it this way. NodeJS without WASM is still a valid environment. You should add an additional member of the JsEnv ADT to represent WASM and then add the appropriate cases here and elsewhere.
|
@qxrein are you able to keep working on this PR? thanks! |
|
yes, I am working on this and thankfully most tests are green now. I am trying my best to finish this before v3.7. |
Cool! When you have a chance, please push your changes, thanks! |
|
made progress with a this PR to enable mostly all tests to pass over (WASM) for JS test suite, overcoming
|
fixed |
|
merge conflict is cursed |
|
making a new draft PR |
|
As you already gone in to the ScalaJS direction not sure if this would help you.. I stumbled upon this talk https://www.youtube.com/watch?v=Z2SWSIThHXY which uses Graal to compile to WASM. There are still things in the works like threading, etc .. |
|
Thanks for the reference @Fristi i believe the approach i am currently using provides more mature and complete WASM support today, especially for our specific needs around fibers and asynchronous operations, CE relies a lot on threading and i’m keeping the port as minimal as possible to make maintenance easy. That said, GraalVM’s WASM backend is moving fast-once they sort out threading and a few other features, it could be a good option. Sorry for reopening the PR, accidentally hit the button on the gh mobile app. |
Cats over Wasm Implementation
Current Status:
Wasmtest environment.To fix:
testsJS/testends withRPCCore$ClosedException