-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add Servo integration via --browser=servo
#92
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
acba05f
to
3267b9e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this 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.
const SCOPE_DIR_SERVO_PUBLIC_STR: &str = "tests/wpt/webgpu"; | ||
const SCOPE_DIR_SERVO_PUBLIC_COMPONENTS: &[&str] = &["tests", "wpt", "webgpu"]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
--browser=servo
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
132510d
to
30e61d7
Compare
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.