Skip to content

Conversation

@jeffcharles
Copy link
Contributor

While trying to add a test case for using a v2 of the Wasm API, I noticed the v1 Wasm API test case doesn't import the Wasm API and uses WASI's stdio directly. So I'm updating it here so I can use a similar test case for v2. I'm also opting to add a test-utils crate to perform the trampolining since we will need to use multiple trampoline versions to rebuild the test fixtures in the near future.

@jeffcharles jeffcharles marked this pull request as ready for review August 14, 2025 20:03
Ok(path)
});

fn download_trampoline(destination: &Path, version: &str) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reasonable alternative to hitting the network here (even if infrequently)? My main worry is that this might potentially make tests flaky. Have you considered committing the shopify-function-trampoline-<triple>.wasm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly I was copying the approach we use for the shopify_function Rust crate here.

We could commit the trampolined file. The part that kind of sucks with rebuilding the Wasm file manually is getting the correct version of the trampoline since if you grab the wrong version, the trampolining will appear to work but running it will fail with a failed to instantiate error. I could also update the test such that we check if the trampoline file exists and don't bother downloading and running the trampoline if it does and then update the instructions for rebuilding the fixtures to say delete the trampolined version and run the tests to rebuild it. Or I could write a Makefile to handle building the fixtures and that could handle downloading and running the correct trampoline version. Thoughts?

Copy link
Contributor

@saulecabrera saulecabrera Aug 18, 2025

Choose a reason for hiding this comment

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

Oh, I wrongly assumed that you were introducing this approach in this PR without realizing that it was a pre-existing approach (in some of our other repos). In that case, I believe we can leave it.

@jeffcharles jeffcharles merged commit bc1a102 into main Aug 18, 2025
10 checks passed
@jeffcharles jeffcharles deleted the jc.redo-wasm-api-test branch August 18, 2025 14:32
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