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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Map streams tests to web-features #49829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions streams/WEB_FEATURES.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
features:
- name: streams
files: "**"
6 changes: 6 additions & 0 deletions streams/readable-streams/WEB_FEATURES.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
features:
- name: streams
files: "**"
- name: async-iterable-streams
files:
- async-iterator.any.js
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@jcscottiii jcscottiii Dec 26, 2024 •
edited
Loading

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 and subgrid: Subgrid is clearly a sub-feature of grid. However, because the web-features repository doesn't have a hierarchy for sub-features, we keep the subgrid tests separate from the grid tests. (Both are mapped to a group named grid.) You can see this in the wpt repository: grid and subgrid. You can see the tests that are mapped to grid vs subgrid.
  • custom-elements group: This group contains features like autonomous-custom-elements and customized-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:

  1. Verify API Coverage: Check if the APIs tested in async-iterator.any.js align with the BCD keys in both streams.yml and async-iterable-streams.yml. If they don't, please update the YAML file accordingly. If they do, move on to step 2.
  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.
  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.

Copy link
Contributor Author

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.)

Copy link
Contributor Author

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