Extend WEB_FEATURES.yml to support feature exclusions#58757
Draft
jugglinmike wants to merge 3 commits intoweb-platform-tests:masterfrom
Draft
Extend WEB_FEATURES.yml to support feature exclusions#58757jugglinmike wants to merge 3 commits intoweb-platform-tests:masterfrom
jugglinmike wants to merge 3 commits intoweb-platform-tests:masterfrom
Conversation
Contributor
Author
|
I've submitted an RFC for the change and switched this pull request to a draft while that's under review. |
Contributor
|
Thanks for this. I left a comment on the RFC. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We've classified a large number of web-features over the past few months, and that experience surfaced many opportunities for improving the maintainability of WPT's
WEB_FEATURES.ymlfiles. One of them is a shorthand for excluding one set of tests from another set.This patch prototypes one possible solution, demonstrated by the application to a single subdirectory (namely,
css/css-sizing/). To summarize: any entry for one web-feature can reference another from thefileslist by prefixing the web-feature ID with the number sign (#). The matched files can be excluded from the referencing set by further prefixing with the exclamation point (!), just as with filename patterns. Filename patterns would be distinguished with a leading sequence of./. For example:This necessarily includes a number of subjective design decisions, but they are by no means settled. In order to promote discussion, I've included the most relevant (by my estimation) design questions below, along with my rational for the answers I've selected.
Should the name of the
fileskey change (e.g.patterns,tests)?The patterns will no longer directly describe files, so "files" may not be the more accurate name.
However, file-matching continues to be their purpose. This might be worth re-visiting when/if we decide to allow for matching tests as distinct from files (e.g. matching specific expansions of
.any.jstests).Should exclusion be the default (and only) behavior?
Currently, we only have need to exclude sets. The
!prefix, borrowed from existing file-pattern syntax, insinuates non-existent "inclusion" use-case. Omitting it would avoid any such insinuation and reduce the number of characters necessary to support the desired use-case.That said, it seems somewhat confusing for the semantics of the two types of patterns to vary so widely. (This could be mitigated by placing exclusions under a separate key since. A name like "exclusions" would make it much easier to recognize the "exclude by default" semantic of these patterns. However, introducing a separate key by any name has its own drawbacks--see the next section.) There's also reason to believe that future extensions will make use of feature inclusion (namely: the ability to explicitly express intentional overlaps).
Should the exclusions be expressed under a different key?
Introducing a new key like
features(and placing all feature exclusion rules there) would negate the need for "pattern type" prefixes altogether and obviate all the design considerations which follow this one. Since we expect most patterns to describe files directly, any prefix for those entries could be considered noisy. (They will also dramatically increase the final size of this patch.)For example:
And for completeness, here's how that could look with the "exclude by default" semantic contemplated above:
My largest concern with splitting patterns across two keys, whatever their name, is the implications for pattern precedence. The sequence for applying two distinct sets of patterns isn't immediately obvious to me (especially considering that this feature may one day be used to express intentional overlaps between sets). Whatever design we might select, I'd wonder about how intuitive the behavior would be for contributors.
A more complex schema could have it both ways: a "list of maps of lists" would reduce the repetition of a "files" designation while preserving the author's control over the order of operations. For example, the rules expressed in the following sketch have a much more clear sequence:
While I think clear precedence is important (especially for future extensions), I don't feel the complexity of this structure is preferable to the noise of pattern prefixes.
Should the prefixes be more explicit (e.g.
files:andfeature:)?The shorter prefixes (
./and#) are inspired by conventions that I expect many developers will recognize (filesystem paths and HTML IDs, respectively), so I think they strike a balance between verbosity and understandability.Should the
files:prefix be omitted?The prefix is not technically necessary because tooling could assume entries are filename patterns if they lack an explicit "feature" prefix (whatever that prefix may be).
I'm not entirely comfortable with a language inconsistency like this, but that's frankly my most weakly-held opinion about this proposal. (Not for nothing: the two-character
./prefix obviates the need to apply quotes to patterns which begin with*, so we'd just be swapping characters for those1.)Should both prefixes be omitted?
Neither prefix may be necessary to disambiguate the two types of patterns. Today (and for the foreseeable future), all test filename patterns include either an asterisk character (
*) or a period character (.), and no web-feature ID includes those characters. If we could secure an explicit commitment from the WebDX community group about the makeup of web-feature IDs, then we could lean on these details to infer pattern type based on their content alone.While this disambiguation heuristic would be trivial to implement in software, it's not particularly discoverable (and even after folks learn about it, it feels cognitively burdensome to apply).
Footnotes
There are currently 212 such patterns
↩