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 core WebAssembly tests to WPT #49277

Merged
merged 3 commits into from
Dec 20, 2024
Merged

Add core WebAssembly tests to WPT #49277

merged 3 commits into from
Dec 20, 2024

Conversation

past
Copy link
Member

@past past commented Nov 19, 2024

The Interop 2024 WebAssembly Testing Investigation intends to get WebAssembly core tests running on WPT.fyi (alongside the existing JS API and Web API tests).

This PR adds a GitHub Actions workflow that pulls the latest tests from the WebAssembly/spec repo on a regular schedule (or when run manually) and creates a PR with any new changes.

This mimics the existing process for updating WebIDL tests, which is low risk and low friction. Creating a PR that needs to be reviewed removes the risk of the workflow messing up the wpt repo at the cost of needing manual review of the changes (which is mostly rubber-stamping as in the WebIDL case). I also considered a push model from the spec repo, which could reduce CPU costs and achieve lower latency to propagate changes, but with a substantially more complex solution.

My WPT repo fork has some successful runs that you can look at:

  • The merged PRs that show up in the repo at wasm/core
  • A testing PR to verify that the action handles additions, removals and modifications.
  • A test run to ensure the scheduled action succeeds even when no changes exist in WebAssembly/spec

This should match the MVP that the investigation is pursuing and could subsequently be expanded to cover tentative tests from WebAssembly/testsuite or elsewhere. This should let the investigation get to a 67% score in the Interop dashboard.

/CC @jgraham, @gsnedders

Copy link

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

spelling nits.

.github/workflows/update-wasm-tests.yml Outdated Show resolved Hide resolved
.github/workflows/update-wasm-tests.yml Outdated Show resolved Hide resolved
.github/workflows/update-wasm-tests.yml Outdated Show resolved Hide resolved
.github/workflows/update-wasm-tests.yml Outdated Show resolved Hide resolved
.github/workflows/update-wasm-tests.yml Outdated Show resolved Hide resolved
rm -rf main/wasm/core
cp -r wasm-spec/out/ main/wasm/core/
find main/wasm/core/ -type f -name '*.html' -exec sed -i 's/\.\/js\/harness\/testharness/\/resources\/testharness/' {} \;
- name: Commit changes
Copy link

Choose a reason for hiding this comment

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

If you use the peter-evans/create-pull-request action as the WebIDL workflow does, you don't need to manually commit the changes or do anything with git itself.

Copy link

Choose a reason for hiding this comment

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

Another example is what we do in emscripten to create a PR, request a review from a github team, and set the PR to auto-merge (after approval and passing tests)

Copy link
Member Author

Choose a reason for hiding this comment

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

We do something similar in the wpt-metadata repo, but I'm always wary of using 3rd-party libraries for security reasons. However, I see the author of this action is now working for GitHub, so maybe this is less of a concern.

Copy link

Choose a reason for hiding this comment

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

Yeah, that makes sense. I'm fine either way, up to you.

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

Low-confidence (in my ability to review) approval. Can you add a codeowners file to make sure you're tagged on any accidental manual changes?

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Generally I guess this is OK, just a few small suggestions.

.github/workflows/update-wasm-tests.yml Outdated Show resolved Hide resolved
.github/workflows/update-wasm-tests.yml Outdated Show resolved Hide resolved
dschuff added a commit to dschuff/spec that referenced this pull request Dec 20, 2024
When converting tests to HTML for use in WPT, use the path to
the test harness in WPT's resources/ directory. This is also
important because the WPT test runner users this path inclusion
to discover the tests.

There are several more things I want to simplify here but I'm
starting with just this because it matches the sed transform
that @past originally wrote in
web-platform-tests/wpt#49277
dschuff added a commit to WebAssembly/spec that referenced this pull request Dec 20, 2024
When converting tests to HTML for use in WPT, use the path to
the test harness in WPT's resources/ directory. This is also
important because the WPT test runner users this path inclusion
to discover the tests.

There are several more things I want to simplify here but I'm
starting with just this because it matches the sed transform
that @past originally wrote in
web-platform-tests/wpt#49277
@past
Copy link
Member Author

past commented Dec 20, 2024

Low-confidence (in my ability to review) approval. Can you add a codeowners file to make sure you're tagged on any accidental manual changes?

I assume you meant to add myself to .github/META.yml so that I can be tagged for review, not to add this workflow and myself as the sole reviewer in CODEOWNERS? We don't have any other workflow blocked on core/admin review and this one doesn't seem more critical than the rest.

- Add myself as a reviewer for .github/
- Remove the path fix up step as it's now done by build.py
- Use  better name for the wpt repo directory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants