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 flag #80

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Add servo flag #80

wants to merge 8 commits into from

Conversation

sagudev
Copy link
Collaborator

@sagudev sagudev commented Feb 15, 2024

Currently moz-webgpu-cts --servo process-reports --preset merge ../../Downloads/wpute/* works on servo if all disabled: comment are replaced with disabled: true

@ErichDonGubler
Copy link
Collaborator

ErichDonGubler commented Feb 27, 2024

This hasn't actually been submitted for review, but 2 design notes on the current contents that, if changed, I could see getting upstreamed:

  1. I think it would be a closer/more honest data model to reorganize the TestScope enum into two enum fields of a struct instead. One would indicate the ecosystem being worked with (Firefox or Servo), and another would indicate whether the Public or Private directories in the chosen ecosystem. These can be combined into a reformed struct TestScope as separate fields:

    enum Browser {
        Firefox,
        Servo,
    }
    
    enum TestVisibility {
        Public,
        Private,
    }
    
    struct TestScope {
        visibility: TestVisibility,
        browser: Browser,
    }

    With all this, we could probably rename TestPath::from_fx_metadata_test to TestPath::from_metadata_test, and require a Browser to be specified as an argument.

  2. After the last point, I'd like to see servo: bool become a usage of the proposed enum Browser.1 Then, the variant can be passed to the proposed/renamed TestPath::from_metadata_test constructor from CLI.

Footnotes

  1. Perhaps defaulting to --browser=firefox, to preserve the existing CLI experience?

@sagudev sagudev mentioned this pull request May 30, 2024
2 tasks
@sagudev sagudev mentioned this pull request Jul 31, 2024
@ErichDonGubler
Copy link
Collaborator

@sagudev: RE: the disabled property not accepting anything but true: Does #143 help?

@sagudev
Copy link
Collaborator Author

sagudev commented Aug 2, 2024

@sagudev: RE: the disabled property not accepting anything but true: Does #143 help?

Looks like it should (although we rewrite all disabled with true values), but I didn't tried it yet.

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