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

Added browser tests using wasmtime test artifacts #42

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsoverson
Copy link
Contributor

@jsoverson jsoverson commented Oct 10, 2023

This PR adds the start of browser-based tests. It adds a playwright configuration to test across Firefox, Chromium, and Webkit and uses test artifacts from the wasmtime project to assert compatibility.

Only one test has been added so far (test-programs/preview1_path_open_read_write). It required some project changes that need review before moving forward.

To run tests:

$ npm install
$ npm run test:build  # clone wasmtime and compile test .wasm
$ npm run build       # compile project typescript
$ npm run test        # run tests

Change Notes:

  • The initialize() function was updated to only call _initialize() if it's exported. AFAICT _initialize() is optional. This lets all reactor libraries use the same shim init logic without needing the user to inspect the exports.
  • I had to pull the wasiImport types into a proper interface to keep satisfying TypeScript. This had the side effect of requiring some changes to satisfy types that would have been the source of errors before. Any change where obj[key] = undefined becomes delete obj[key] is due to this. (reverted)
  • I needed to propagate the rights flags through to the Fd implementations so that fd_fdstat_get could report correct values.
  • I added a dumb path_unlink_file implementation to Dir to satisfy the test wasm. It deletes the instance from the dir's contents and doesn't do anything more destructive.

@jsoverson
Copy link
Contributor Author

@bjorn3 I hope these changes are again in the right direction. Testing could go any one of a dozen ways, and this seemed like the best cost/value.

@bjorn3
Copy link
Owner

bjorn3 commented Oct 10, 2023

Using wasmtime's wasi tests is a great idea. Thanks for working on this. FYI I probably won't have time to review this in the next couple of days.

Part of #7

@jsoverson
Copy link
Contributor Author

@bjorn3 np, thanks for being willing and responsive!

/playwright-report/
/playwright/.cache/

wasmtime/
Copy link
Owner

Choose a reason for hiding this comment

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

nit: missing trailing newline


path_unlink_file(path: string): number {
delete this.dir.contents[path];
return wasi.ERRNO_SUCCESS;
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add a FIXME to check if the path actually exists and to return an error otherwise. Also is there no test in wasmtime's test suite for that?

if (this.file.readonly) return { ret: wasi.ERRNO_BADF, nwritten };
if (
this.file.readonly ||
(Number(this.fs_rights_base) & wasi.RIGHTS_FD_WRITE) === 0
Copy link
Owner

Choose a reason for hiding this comment

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

Would making RIGHTS_FD_WRITE a Bigint work to avoid this cast?

this.file.readonly ||
(Number(this.fs_rights_base) & wasi.RIGHTS_FD_WRITE) === 0
)
return { ret: wasi.ERRNO_BADF, nwritten };
Copy link
Owner

Choose a reason for hiding this comment

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

For please use {} for if where the action is not on the same line. That helps prevent issues like the duplicate "goto fail;" that caused a huge security issue in macOS a couple of years back.

WASMTIME_DIR=$SCRIPT_DIR/../wasmtime

if ! [[ -d wasmtime ]]; then
git clone --recurse-submodules https://github.com/bytecodealliance/wasmtime.git --depth=1 $WASMTIME_DIR;
Copy link
Owner

Choose a reason for hiding this comment

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

Are the submodules actually necessary? And if so are all of them necessary?

WASMTIME_DIR=$SCRIPT_DIR/../wasmtime

if ! [[ -d wasmtime ]]; then
git clone --recurse-submodules https://github.com/bytecodealliance/wasmtime.git --depth=1 $WASMTIME_DIR;
Copy link
Owner

Choose a reason for hiding this comment

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

Could you pin a specific commit of wasmtime for reproducibility in the future?


cargo build --manifest-path $WASMTIME_DIR/crates/test-programs/artifacts/Cargo.toml

TARGET_DIR=$(ls -dt $WASMTIME_DIR/target/debug/build/test-programs-artifacts-* | head -n 1)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
TARGET_DIR=$(ls -dt $WASMTIME_DIR/target/debug/build/test-programs-artifacts-* | head -n 1)
TARGET_DIR=$(ls -dt $WASMTIME_DIR/target/debug/build/test-programs-artifacts-*/out | head -n 1)

There is no guarantee that the first one is the one with the OUT_DIR.

@bjorn3
Copy link
Owner

bjorn3 commented Oct 17, 2023

I'm not entirely sure all fs_rights_base changes are correct. Would you mind pulling out all changes to the actual code except for fs_rights_base into a separate PR? Also could you add CI integration?

@bjorn3
Copy link
Owner

bjorn3 commented Nov 25, 2023

@jsoverson are you still interested in working on this or would you prefer if I work on splitting it up into several commits and landing it myself?

@jsoverson
Copy link
Contributor Author

@bjorn3 sorry, this fell off my radar during a few hectic weeks. I'll jump on it this coming week.

@jsoverson
Copy link
Contributor Author

Status: I'm going to fork wasmtime to create releases consisting of strictly the test wasm files. Getting CI setup to clone wasmtime to build the test artifacts is excessive.

@bjorn3
Copy link
Owner

bjorn3 commented Dec 1, 2023

You can use --filter=blob:none when cloning to tell git to only fetch the blobs that are actually necessary. For me that only takes 10s for a total download of 24MB. It can be reduced even further using a sparse checkout, but I think 10s is more than fast enough.

@jsoverson
Copy link
Contributor Author

It's more the overhead of setting up a rust environment to build wasm files that really shouldn't change. It will dramatically extend the duration of CI tests, vs. downloading a tarball of wasm artifacts.

@bjorn3
Copy link
Owner

bjorn3 commented Dec 1, 2023

I see. How about caching the wasm test files in a github actions cache after they are built. And then only setup a rust toolchain for building if the cache is missing. That should be easier to keep up to date with new wasmtime versions than building locally and checking into a repo.

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.

2 participants