-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Map streams tests to web-features #49829
base: master
Are you sure you want to change the base?
Map streams tests to web-features #49829
Conversation
files: "**" | ||
- name: async-iterable-streams | ||
files: | ||
- async-iterator.any.js |
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.
async-iterator.any.js will be under both streams and async-iterable-streams. Is that intentional?
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.
Yes, that's intentional. Async iterable streams is a subfeature of streams.
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.
Thanks for responding.
I'm asking this because the way we organize features in the web-features repository is a bit different from how they might be conceptually related. While async iterable streams are a sub-feature of streams in general, we treat all features as top-level. We then use groups to categorize related features. (This can be a little confusing since some features have the same name as their group.)
It's important to remember that the wpt
repository doesn't recognize these groups. This means we need to explicitly tag each test with the specific feature name it belongs to. We also need to avoid including tests that belong to multiple features within the same group. (Otherwise, this is a new pattern we can discuss in step 3 below).
Here are some examples of how we've handled this in the past:
grid
andsubgrid
: Subgrid is clearly a sub-feature of grid. However, because theweb-features
repository doesn't have a hierarchy for sub-features, we keep thesubgrid
tests separate from thegrid
tests. (Both are mapped to a group namedgrid
.) You can see this in thewpt
repository: grid and subgrid. You can see the tests that are mapped to grid vs subgrid.custom-elements
group: This group contains features likeautonomous-custom-elements
andcustomized-built-in-elements
. While not a parent-child relationship, this example illustrates how we use the!
exclusion pattern in the file to ensure tests are only associated with one feature.
By including async-iterator.any.js
in streams
, you're implying that the completeness score for the streams
web feature (which includes these APIs) depends on assertions in async-iterator.any.js
. However, I suspect async-iterator.any.js
only tests the APIs defined in the async-iterable-streams
feature (see the API list here).
To ensure we're mapping these tests correctly, I propose the following steps:
- Verify API Coverage: Check if the APIs tested in
async-iterator.any.js
align with the BCD keys in bothstreams.yml
andasync-iterable-streams.yml
. If they don't, please update the YAML file accordingly. If they do, move on to step 2. - Reorganize Tests (If Possible): If there's overlap in the APIs tested by
async-iterator.any.js
, let's try to reorganize the test so it focuses on assertions for a single feature. If that's not feasible, proceed to step 3. - File an Issue: If we can't cleanly separate the tests due to unavoidable overlap between the features, we should file an issue in the
web-features
repository to document this.
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.
Thanks for clarifying.
If I understand correctly, there is the streams
group which contains the streams
feature and the async-iterable-streams
feature. The streams
feature is supposed to be the "base" feature, whereas the async-iterable-streams
feature is a later addition on top of that base feature. Following that logic, I agree that the streams for async iterable streams should not overlap with those of the base feature, so I'll add an exclusion for async-iterator.any.js
so it's not part of the streams
feature.
I still have some questions though. (If this isn't the right place to ask them, I can also repost them as issues over at web-features.)
- Many JavaScript APIs on the web (including Streams) have a Web IDL interface definition, and we use idlharness tests to automatically test the API's input validation. These tests are auto-generated from the IDL definition, and they all live inside a single file (for streams: idlharness.any.js). So whenever we add a new property or method to the IDL, the IDL harness tests will automatically be extended with new tests for that property or method. Should we keep these IDL tests associated with the base
streams
feature (although the tests also cover later additions), or exclude them entirely? - There are a quite a few additions to the Streams API that are not yet covered by
web-features
(see below). Should I add separate subfeatures for these? It looks like there was already some prior discussion in Addstreams
web-platform-dx/web-features#990 and Add more streams keys/features web-platform-dx/web-features#2358, but I'm not really sure where to go from there.- Transferable streams, added to the standard in August 2020 and shipped in Chrome 87 and Firefox 103
ReadableStream.from(asyncIterable)
, added to the standard in June 2023 and shipped in Firefox 117Transformer.cancel()
, added to the standard in September 2023 but not yet shipping in any browserReadableStreamBYOBReader.read(view, {min})
, added to the standard in November 2023 and shipped in Firefox 134
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 opened a PR to add more subfeatures for streams: web-platform-dx/web-features#2491
No description provided.