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 Servo integration via --browser=servo #92

Merged
merged 4 commits into from
May 11, 2024

Conversation

sagudev
Copy link
Collaborator

@sagudev sagudev commented Apr 16, 2024

Split off #80 with #80 (comment) taken into account. This PR only contains minimal work to acknowledge servo's existence.

It works on servo (if also used with sagudev@dddc19d), but I do not have m-c clone to test with.

@ErichDonGubler

This comment was marked as outdated.

@sagudev

This comment was marked as outdated.

Copy link
Owner

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

LGTM with some nitpicks and issues that I've resolved with fixup! commits. LMK what you think before I merge things; I've tried to leave good notes in the commits themselves, but I've translated feedback to conversations here, too.

moz-webgpu-cts/src/shared.rs Outdated Show resolved Hide resolved
moz-webgpu-cts/src/shared.rs Outdated Show resolved Hide resolved
Comment on lines +441 to +442
const SCOPE_DIR_SERVO_PUBLIC_STR: &str = "tests/wpt/webgpu";
const SCOPE_DIR_SERVO_PUBLIC_COMPONENTS: &[&str] = &["tests", "wpt", "webgpu"];
Copy link
Owner

Choose a reason for hiding this comment

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

issue(non-blocking): Hmm, I think trying to use a "public"/"private" scope model for Servo reveals some Mozilla-centricity in my thinking when I proposed TestVisibility. Looking at Servo's current layout of the tests/wpt folder, lemme make a table to demonstrate what I think current mental models are:

TestVisibility variant Files used in Firefox Files Erich thinks Servo's WebGPU team uses based on this PR Files Erich assumed Servo used before this PR
Private testing/web-platform/mozilla/{meta,tests} (_mozilla manifest/route prefix). tests/wpt/webgpu/{meta,tests} (_webgpu manifest/route prefix) tests/wpt/mozilla/{meta,tests} (_mozilla manifest/route prefix)
Public testing/web-platform/{meta,tests} (upstream manifest, no route prefix) Unused? tests/wpt/{meta,tests} (upstream manifest, no route prefix)

The reason that moz-webgpu-cts cares about distinguishing Public from Private tests really for debugging and making some of the formatting code more consistent. It seems that Servo's test runs only produce reports with WebGPU tests under the _webgpu route prefix? If that's the case, I think it'd be more sensible to make TestScope an enum again, with different models of scopes between browsers, rather than trying to impose TestVisibility on everything. That way, Servo can have a single variant for its WebGPU paths, and be done.

Copy link
Owner

Choose a reason for hiding this comment

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

I have a resolution for this with #103, which we can save for after this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What does private/public mean in this context? Are WebGPU CTS tests meant to be private or public (I considered them as public), but maybe public is meant for upstream WPT tests?

In servo _webgpu is alias to tests/wpt/webgpu/{meta,tests} which contains WebGPU CTS. An that's it, those are only WebGPU tests we have.

Copy link
Owner

Choose a reason for hiding this comment

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

@sagudev: Yep, "public" was supposed to mean "upstream WPT". 😅 I definitely wasn't using the right vocabulary; hopefully the table and the PR clarify the details here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why exactly would upstream path be needed for moz-webgpu-cts? Are there any webgpu test there, or is this some kind of future-proofing for when CTS lands in upstream WPT (or moz-webgpu-cts takes over all expectation updating)?

Copy link
Owner

Choose a reason for hiding this comment

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

We now have the right filtering in place for Firefox CI. So, we don't generate reports with unrelated entries anymore. At the time, though, it was easy to work around it with git add <scope>/meta/webgpu && git restore . locally. Even now, we don't fully automate the committing of expected properties updated via moz-webgpu-cts; there's still a human that looks at things that changed. I don't foresee this changing, as we slowly validate what parts of the CTS are stable for us in Firefox.

But "public" results/files are not just skipped? They are also read and formatted, IIRC?

Sorry, I didn't expect you to be interested in details, so I've been pretty terse. 😅 You're right; report results for any tests outside <scope>/meta/webgpu will still get processed and written out by moz-webgpu-cts as with tests it's intended to be used for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

report results for any tests outside /meta/webgpu will still get processed and written out by moz-webgpu-cts as with tests it's intended to be used for.

That's what's been confusing me the most. In servo there are a lot of "invalid" and not formatted expectations files, so that's why we cannot have moz-webgpu-cts try to load them.

Copy link
Owner

Choose a reason for hiding this comment

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

Is the Servo project actually interested in using moz-webgpu-cts in a broader scope? Or is that merely a pain point that's forced you to narrow what metadata moz-webgpu-cts is incidentally exposed to? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is the Servo project actually interested in using moz-webgpu-cts in a broader scope?

Formatting would be useful as I do not think any exists for expectations (or at least we aren't using any), but there is currently nothing planned.

Or is that merely a pain point that's forced you to narrow what metadata moz-webgpu-cts is incidentally exposed to? 🤔

This is exactly what happened.

Copy link
Owner

Choose a reason for hiding this comment

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

Formatting would be useful as I do not think any exists for expectations (or at least we aren't using any), but there is currently nothing planned.

I suspect that a general-purpose formatter would be very difficult to get consensus on, as opposed to the very opinionated approach here (CC @jgraham). I don't see any upstream issues in wpt or rfcs for it. However, you could use whippit now to create your own parser, and then a custom emitter like format_file to enforce things like property order and spacing. AIUI that's really the full breadth of things you can control, with the metadata language.

moz-webgpu-cts/src/main.rs Outdated Show resolved Hide resolved
moz-webgpu-cts/src/shared.rs Outdated Show resolved Hide resolved
moz-webgpu-cts/src/shared.rs Show resolved Hide resolved
moz-webgpu-cts/src/shared.rs Show resolved Hide resolved
moz-webgpu-cts/src/shared.rs Show resolved Hide resolved
moz-webgpu-cts/src/shared.rs Show resolved Hide resolved
moz-webgpu-cts/src/main.rs Show resolved Hide resolved
@ErichDonGubler ErichDonGubler changed the title Add servo as browser option Add Servo integration via --browser=servo May 10, 2024
@ErichDonGubler

This comment was marked as resolved.

@sagudev

This comment was marked as resolved.

@ErichDonGubler

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler merged commit b176b59 into ErichDonGubler:main May 11, 2024
25 checks passed
@ErichDonGubler

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-webgpu-cts enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants