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

Rework TestScope model to meet Servo's needs #103

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ErichDonGubler
Copy link
Owner

@ErichDonGubler ErichDonGubler commented May 10, 2024

In #92, I had suggested a new representation for TestScope where Browser and TestVisibility were two dimensions on which other products could integrate with moz-webgpu-cts. However, that model is wrong; Servo only really has one set of paths and runner URLs that it cares about, currently. One can observe this with the todo!()s that I merged in that PR. 😅 So, I'm fixing my mistake!

Throw away TestVisibility and make TestScope mirror Browser variants with additional, independent structure per-browser. Concretely, make TestScope use either a FirefoxTestScope or a ServoTestScope, the latter of which only has a WebGpu variant (for now).

@ErichDonGubler ErichDonGubler added bug Something isn't working A-webgpu-cts labels May 10, 2024
@ErichDonGubler ErichDonGubler self-assigned this May 10, 2024
@ErichDonGubler ErichDonGubler changed the base branch from servo-init to main May 11, 2024 14:48
@ErichDonGubler ErichDonGubler marked this pull request as ready for review May 11, 2024 14:52
@ErichDonGubler

This comment was marked as outdated.

sagudev

This comment was marked as resolved.

@ErichDonGubler

This comment was marked as resolved.

browser: Browser,
visibility: TestVisibility,
pub(crate) enum ServoTestScope {
WebGpu,
Copy link
Owner Author

Choose a reason for hiding this comment

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

From @sagudev on the commit (original link):

Maybe add some rustdoc comment that would describe variant. Something about that this is WebGPU CTS directory.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@sagudev: How about daa3f73?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is nitpicking but source sounds wired to me in this context, maybe Servo tree or Servo repo would be more appropriate?

Disclaimer: I am NOT a native speaker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-webgpu-cts bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants