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

Add CI job for checking if our WASM compilation succeeds #1186

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jakzale
Copy link
Contributor

@jakzale jakzale commented Feb 2, 2024

This PR expands on our WASM demo by

  • fleshing it more as an NPM package,
  • adding TypeScript support,
  • adding Playwright tests for testing WASM in major browsers, and
  • adding a GitHub CI job to run it all.

@vivekvpandya
Copy link
Contributor

May need something like livepeer/catalyst#811

@matthiasgoergens
Copy link
Collaborator

Can we make the tests pass?

@jakzale
Copy link
Contributor Author

jakzale commented Feb 15, 2024

Can we make the tests pass?

@matthiasgoergens, this is blocked by #1242.

with:
name: playwright-report
path: wasm-demo/playwright-report/
retention-days: 30
Copy link
Contributor

@codeblooded1729 codeblooded1729 Feb 16, 2024

Choose a reason for hiding this comment

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

So this test suite is intended to be triggered only on push to main branch, since its slow. The slowness comes from

  • need to install playwright
  • The test test-slow which runs test on multiple browsers

👍

@@ -2,3 +2,4 @@
channel = "nightly-2024-01-23"
components = ["rustfmt", "rust-src", "rust-analyzer", "clippy"]
profile = "minimal"
targets = ["wasm32-unknown-unknown", "wasm32-wasi"]
Copy link
Contributor

Choose a reason for hiding this comment

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

wasm32-wasi target has been added to access OS services, since previously we had issues with, for example obtaining time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also use wasm32-wasi to able to quickly test our WASM build by running it with wasmtime. Also, wasm32-wasi tends to give more meaningful stack traces in case of an exception.

"test": "wasmtime ./dist/demo.wasm",
"test-slow": "playwright test"
}
}
Copy link
Contributor

@codeblooded1729 codeblooded1729 Feb 16, 2024

Choose a reason for hiding this comment

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

So this runs

  • wasm-pack to generate .js files which run the wasm code.
  • cargo build to generate target for wasm32-wasi
  • webpack to bundle stuff generated by wasm-pack into pkg/wasm_demo.js

test is for testing the wasm32-wasi target through wasmtime as runtime environment
test-slow actually automates human interaction through browser where he would execute commands to run the wasm targets. It checks both targets on three major browsers.

await page.getByText("Loading wasm32-wasi version of demo...").waitFor();
await page.getByText("Proving :true").waitFor();
await page.getByText("wasm32-wasi demo completed.").waitFor();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Simulating human interaction 👍

// For some reason this is incompatible
wasi.start(inst as any);

term.writeln("wasm32-wasi demo completed.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Runs wasm32-wasi target on browser.

wasm_demo(99, 99);

term.writeln("wasm32-unknown-unknown demo completed.");
});
Copy link
Contributor

@codeblooded1729 codeblooded1729 Feb 16, 2024

Choose a reason for hiding this comment

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

runs wasm_demo generated through wasm-pack

@jakzale
Copy link
Contributor Author

jakzale commented Feb 20, 2024

This is probably blocked by #1270, so we can have nix back.

@jakzale
Copy link
Contributor Author

jakzale commented Feb 21, 2024

@matthiasgoergens @codeblooded1729, I can try splitting this PR into smaller ones. Would that help?

@matthiasgoergens
Copy link
Collaborator

@jakzale What's the status of this?

@jakzale
Copy link
Contributor Author

jakzale commented Mar 25, 2024

@matthiasgoergens, it ended up being too big to get it reviewed properly. I need to split it into smaller chunks, but I want to finish work on debugging first.

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.

4 participants