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

Unreviewed scope increase in URL since Interop 2023 launch #338

Closed
foolip opened this issue May 17, 2023 · 15 comments
Closed

Unreviewed scope increase in URL since Interop 2023 launch #338

foolip opened this issue May 17, 2023 · 15 comments
Labels
focus area: URL test-change-proposal Proposal to add or remove tests for an interop area

Comments

@foolip
Copy link
Member

foolip commented May 17, 2023

Test List

https://wpt.fyi/results/url/idlharness.any.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop
https://wpt.fyi/results/url/idlharness.any.worker.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop
https://wpt.fyi/results/url/urlencoded-parser.any.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop
https://wpt.fyi/results/url/urlencoded-parser.any.worker.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop
https://wpt.fyi/results/url/urlsearchparams-delete.any.html?label=experimental&label=master&product=chrome&product=firefox&product=safari&aligned&view=interop
https://wpt.fyi/results/url/urlsearchparams-delete.any.worker.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop
https://wpt.fyi/results/url/urlsearchparams-has.any.html?label=experimental&label=master&product=chrome&product=firefox&product=safari&aligned&view=interop
https://wpt.fyi/results/url/urlsearchparams-has.any.worker.html?label=experimental&label=master&product=chrome&product=firefox&product=safari&aligned&view=interop

Rationale

There are new failing subtests in these tests due to changes post-launch.

web-platform-tests/wpt#38953 was written for https://bugs.chromium.org/p/chromium/issues/detail?id=633153, but wasn't part of the scope originally.

web-platform-tests/wpt#39865 is for whatwg/url#735, which is a new feature. (It might be trivial to implement, I don't know.)

If we don't have support from everyone to include these, we should exclude these subtests from Interop 2023.

@foolip foolip added the test-change-proposal Proposal to add or remove tests for an interop area label May 17, 2023
@foolip
Copy link
Member Author

foolip commented May 17, 2023

cc @annevk

@annevk
Copy link
Member

annevk commented May 17, 2023

Thanks @foolip. To be clear, I didn't mean to increase the scope of Interop 2023. And I'd much rather have other browsers focus on their URL parser than these API extensions.

@foolip
Copy link
Member Author

foolip commented May 17, 2023

No problem, there's nothing preventing changing the tests that are labeled for Interop 2023, so things like this are bound to happen.

Unfortunately we have no mechanism for targeting or filtering out subtests, so I don't know how we'd go about this. We could just hack something into the scoring script, and try to generalize if we have more cases like this.

@foolip
Copy link
Member Author

foolip commented May 17, 2023

@jgraham can you review this from the Gecko side?

@jgraham
Copy link
Contributor

jgraham commented May 17, 2023

I think we're reluctant to add additional scope to the URL focus area. New API features seem like they should be considered for Interop 2024 rather than added to the existing tests.

@foolip
Copy link
Member Author

foolip commented May 17, 2023

I found one additional change to the idlharness tests that came via web-platform-tests/wpt#38345, the canParse() static function.

@foolip
Copy link
Member Author

foolip commented May 17, 2023

To make the suggestion concrete:

  • Drop the idlharness.js tests entirely. These tests were 100% passing at the beginning of the year, and the API surface isn't the main point of this focus area, parsing is. Dropping it is less work than trying to filter out tests.
  • Add a hack to the scoring script to filter out subtests with the string "%EF%BF%BF=%EF%BF%BF", double checking it removes exactly as many tests as we think it will.

In parallel I'll check if a fix for the "%EF%BF%BF=%EF%BF%BF" test would be trivial, in which case we can perhaps just fix it. No promises though.

@annevk
Copy link
Member

annevk commented May 17, 2023

Sounds good. I think it would be okay to exclude the URLSearchParams tests as well, with the same rationale. (In retrospect I wish we would have excluded more to put the actual interoperability issue more into focus. It's kinda obscured at the moment due to all the passing tests.)

@foolip
Copy link
Member Author

foolip commented May 17, 2023

That would make sense, and leaves urlencoded-parser.any.js covering the parsing behavior.

foolip added a commit to web-platform-tests/wpt-metadata that referenced this issue May 23, 2023
@foolip
Copy link
Member Author

foolip commented May 23, 2023

I've created web-platform-tests/wpt-metadata#4244 to show what the test list change would be. @annevk do you need anything more to make a call on this? I haven't updated up the scoring script yet, waiting to hear back about the feasibility of fixing the "%EF%BF%BF=%EF%BF%BF" cases.

@annevk
Copy link
Member

annevk commented May 23, 2023

For WebKit that seems fine. I'll flag it internally just in case, but I don't foresee any issue.

@foolip
Copy link
Member Author

foolip commented May 23, 2023

Thanks @annevk! @jgraham are you OK dropping the tests?

@zcorpan
Copy link
Member

zcorpan commented May 24, 2023

cc @valenting

@valenting
Copy link

I'm OK with dropping them, but I don't feel strongly about it.

@foolip
Copy link
Member Author

foolip commented Jun 8, 2023

Good news regarding the "" tests. @hayatoito has a fix for this in progress in https://chromium-review.googlesource.com/c/chromium/src/+/4567772 so we're OK keeping that in scope.

That means web-platform-tests/wpt-metadata#4244 is the complete change being proposed. We have support from Gecko (@valenting) and WebKit (@annevk) on this issue, so I'll land the PR and close this issue.

foolip added a commit to web-platform-tests/wpt-metadata that referenced this issue Jun 8, 2023
@foolip foolip closed this as completed Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus area: URL test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Development

No branches or pull requests

5 participants