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

chore(wasm): add CI workflow #3503

Merged
merged 1 commit into from
Jan 23, 2024
Merged

chore(wasm): add CI workflow #3503

merged 1 commit into from
Jan 23, 2024

Conversation

TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented Dec 9, 2023

References #3475

@TalDerei
Copy link
Collaborator Author

TalDerei commented Dec 10, 2023

@grod220 Attempting to execute the workflow in headless mode seems to fail. Alternatively, the interactive mode (without the headless flag) compiles successfully, but stalls indefinitely since the CI environment can't interact with it. We may need to rethink our approach to testing the WASM crate.

Tagging @conorsch if you have any suggestions?

@conorsch
Copy link
Contributor

Nice start! Looks like the new test isn't passing yet, so I'll check back when it's green. N.B. we already have a "wasm" job inside the rust workflow—perhaps this new test should be added around here:

(I realize the parent ticket mentioned a new workflow, but I'm being persnickety. <3) Currently, the only wasm-related CI we run on this repo is "make sure the wasm builds OK." Is that check still useful if we're actually running tests? That is, doesn't running the tests involve building before running?

Holler if you want more eyes on this.

@conorsch
Copy link
Contributor

Tagging @conorsch if you have any suggestions?

Ha, I hadn't seen your comment when I wrote my message. Thanks for pinging.

Attempting to execute the workflow in headless mode seems to fail. Alternatively, the interactive mode (without the headless flag) compiles successfully, but stalls indefinitely since the CI environment can't interact with it.

Can you teach me how to run this test locally? I see the new command added to both the wasm crate's README and the CI job: wasm-pack test --chrome -- --test test_build --target wasm32-unknown-unknown --release --features "mock-database" but when I run that locally, I get stuck on:

Set timeout to 20 seconds...
Interactive browsers tests are now available at http://127.0.0.1:8000

Note that interactive mode is enabled because `NO_HEADLESS`
is specified in the environment of this process. Once you're
done with testing you'll need to kill this server with

which is odd, because I definitely don't have NO_HEADLESS exported in my environment. Are you able to run these tests locally? If so, and you can teach me to do the same, I'm sure we can make CI happy.

@TalDerei
Copy link
Collaborator Author

TalDerei commented Dec 11, 2023

You're running it properly! Since it's an interactive browser, you need to open http://127.0.0.1:8000 to start executing the test. Alternatively running it in headless mode (wasm-pack test --headless --chrome -- --test test_build --target wasm32-unknown-unknown --release --features "mock-database) fails locally.

@conorsch
Copy link
Contributor

... fails locally.

Yup, same here, that command fails. Likely we'll have to prepare a webdriver.json config or something. But my recommendation is to forget about the CI workflows until we have a testing command that executes reliably locally. For instance, this command does pass for me locally:

❯ wasm-pack test --headless --firefox -- --test test_build --target wasm32-unknown-unknown --release --features "mock-database"
[...snip...]
Set timeout to 20 seconds...
Running headless tests in Firefox on `http://127.0.0.1:42199/`
Try find `webdriver.json` for configure browser's capabilities:
Not found
running 1 test                                    

test test_build::tests::mock_build_serial_and_parallel ... ok

test result: ok. 1 passed; 0 failed; 0 ignored

That's a start, at least!

@TalDerei
Copy link
Collaborator Author

TalDerei commented Dec 24, 2023

This was backlogged in favor of more higher priority web related work. @conorsch Strangely wasm-pack test --headless --firefox -- --test test_build --target wasm32-unknown-unknown --release --features "mock-database" passes locally in the wasm-ci-cd branch, but fails in the main branch. What would the reason for that be since this branch doesn't have any changes that should affect that?

We can pick this up after the holidays ~

@TalDerei
Copy link
Collaborator Author

TalDerei commented Jan 2, 2024

tagging @grod220

@conorsch
Copy link
Contributor

conorsch commented Jan 3, 2024

Strangely wasm-pack test --headless --firefox -- --test test_build --target wasm32-unknown-unknown --release --features "mock-database" passes locally in the wasm-ci-cd branch, but fails in the main branch. What would the reason for that be since this branch doesn't have any changes that should affect that?

That's not clear to me, either, but FWIW I can definitely reproduce the behavior you describe. There are changes to the wasm tests in this diff, but even copying crates/wasm/tests/test_build.rs on main and rerunning the tests still fails.

@grod220
Copy link
Contributor

grod220 commented Jan 4, 2024

Sadly, won't be able to dive into this before my holiday given other high-priority rpc changes before we freeze those. Think we can de-prioritize this and revisit later.

@conorsch
Copy link
Contributor

conorsch commented Jan 8, 2024

fails in the main branch

As of merge of #3586, I'm able to run wasm-pack test --headless --firefox -- --test test_build --target wasm32-unknown-unknown --release --features "mock-database" on main and it passes! I'm going to rebase this branch on top of latest main and poke at the new test.

@TalDerei
Copy link
Collaborator Author

TalDerei commented Jan 8, 2024

@conorsch might need webdriver.json config to satisfy CI?

@conorsch
Copy link
Contributor

I've made sure I can run the wasm-pack headless test command on both my local workstation, and also on a truly headless setup (rented cloud box), and no problems in either place. So there's something different about the GHA context that we're overlooking. I'll dig into this more.

@conorsch conorsch force-pushed the wasm-ci-cd branch 2 times, most recently from 196a3b5 to f7d46e3 Compare January 23, 2024 18:43
@conorsch
Copy link
Contributor

Finally sorted out the weirdness between local env and CI env: the WIP CI job didn't have lfs=true, so the proving keys being included in the tests weren't real. The error was occurring on

let output_key_js: JsValue = serde_wasm_bindgen::to_value(&output_key).unwrap();
which gets data from
let output_key: &[u8] = include_bytes!("../../crypto/proof-params/src/gen/output_pk.bin");

Then simply adding lfs=true to the checkout task worked. I've since reorganized a bit, adding the headless browser tests to the existing WASM workflow. When we next discuss breaking the wasm crate out of the workspace, we can rethink how best to map the tests to the testing coverage we want. For now, the headless browser tests are running just the test_build setup, against headless Firefox, which I declare Good Enough™ for merge.

@TalDerei
Copy link
Collaborator Author

cc @grod220

@conorsch conorsch marked this pull request as ready for review January 23, 2024 19:04
Squashed the following commits by @TalDerei:

  * update wasm readme and add action workflow file
  * trigger workflow on pull-request
  * add assertion check to wasm unit test
  * modify path in ci workflow
  * attemp to pass CI
  * add wasm-pack binary to workflow
  * attempt to run in headless browser mode
  * add firefox execution environment

We struggled a bit in getting the test to pass CI, but it was as simple
as ensuring that lfs=true on the repo checkout.

Closes #3475.
@conorsch conorsch merged commit 0c54e5b into main Jan 23, 2024
7 checks passed
@conorsch conorsch deleted the wasm-ci-cd branch January 23, 2024 19:48
@grod220
Copy link
Contributor

grod220 commented Jan 23, 2024

wow! amazing work 🎊

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.

3 participants