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

RFC #163: Add web_feature metadata file to web-platform-tests #163

Merged
merged 10 commits into from
Oct 23, 2023

Conversation

jcscottiii
Copy link
Contributor

Replacement of #157 that uses the feedback from the last WPT meeting.

Rendered

@jcscottiii jcscottiii changed the title # RFC #: Add feature_set metadata file to web-platform-tests RFC #163: Add feature_set metadata file to web-platform-tests Aug 22, 2023
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.

This seems reasonable

rfcs/feature-set-metadata-file-to-web-platform-tests.md Outdated Show resolved Hide resolved
rfcs/feature-set-metadata-file-to-web-platform-tests.md Outdated Show resolved Hide resolved
rfcs/feature-set-metadata-file-to-web-platform-tests.md Outdated Show resolved Hide resolved
rfcs/feature-set-metadata-file-to-web-platform-tests.md Outdated Show resolved Hide resolved

# Proposed change

The proposed change includes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably there's also a step where this data is integrated in wpt.fyi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will come as a separate RFC. Now that this RFC is moving to separate things (as opposed to #157), there might be a different way to integrate this into wpt.fyi instead of just modifying the current metadata ingestion path.

@jcscottiii jcscottiii force-pushed the Add-feature_set-metadata-file-to-web-platform-tests branch from 2561022 to 35bcbe3 Compare August 23, 2023 23:36
@foolip
Copy link
Member

foolip commented Sep 5, 2023

https://github.com/web-platform-dx/web-features has been renamed to match the name of the NPM package, so I suggest renaming this to web_features or features.

@jcscottiii jcscottiii changed the title RFC #163: Add feature_set metadata file to web-platform-tests RFC #163: Add web_feature metadata file to web-platform-tests Sep 5, 2023
@jcscottiii
Copy link
Contributor Author

Thanks for pointing that out @foolip ! I have renamed things.

@foolip
Copy link
Member

foolip commented Sep 15, 2023

@jcscottiii did you forget to push changes? There's a lot of "feature-set" in the text still.

@jcscottiii
Copy link
Contributor Author

@foolip that's quite embarrassing of me 😅. Thanks for catching that.

I fixed that.

@jcscottiii
Copy link
Contributor Author

@jgraham I added the lint suggestion that you mentioned during the https://github.com/web-platform-tests/wpt-notes/blob/master/minutes/2023-09-05.md

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Thanks @jcscottiii!

@foolip
Copy link
Member

foolip commented Sep 26, 2023

@Ms2ger are you OK with the wpt.fyi integration being out of scope? You have a "requested changes" review if you'd like to give it another spin :)

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.

Not blocking

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.

So generally this seems OK, but I'd at least like to understand the requirements that drove the inheritance design.

Longer term, I'm worried about the duplication between this and wpt-metadata. wpt.fyi seems like the obvious UI to add feature metadata, even though (like interop labels) they don't correspond to test results. Indeed the lowest-risk way of prototyping this work and exploring requirements would be just adding labels to tests like feature-subgrid.

rfcs/feature-set-metadata-file-to-web-platform-tests.md Outdated Show resolved Hide resolved
rfcs/feature-set-metadata-file-to-web-platform-tests.md Outdated Show resolved Hide resolved
@foolip
Copy link
Member

foolip commented Oct 3, 2023

@jcscottiii now that the file can list multiple features, can you rename it from WEB_FEATURE.yml to plural WEB_FEATURES.yml? This aligns with the package name web-features too :)

@jcscottiii
Copy link
Contributor Author

@foolip Thanks for the suggestion. Done!

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Thanks @jcscottiii! I've looked this over once more. There are lots of ways that one could do the "exclude features from parent directories" part, and this approach seems pragmatic to me. (You could also imagine a system of includes/excludes, but then you need to both exclude tests from a parent feature and include them into a child feature, which isn't a big deal, but also not an improvement IMHO.)

@jcscottiii
Copy link
Contributor Author

jcscottiii commented Oct 11, 2023

@jgraham any thoughts on the latest version

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.

I think this looks good enough to be worth prototyping.

@jcscottiii jcscottiii merged commit 33a1c2d into master Oct 23, 2023
@jcscottiii jcscottiii deleted the Add-feature_set-metadata-file-to-web-platform-tests branch October 23, 2023 14:43
@foolip
Copy link
Member

foolip commented Oct 25, 2023

A first WEB_FEATURES.yml file was added in web-platform-tests/wpt#42709. Thanks @jcscottiii for driving this RFC!

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.

None yet

4 participants