Skip to content

URL: fix setters tests #38150

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

Merged
merged 2 commits into from
Jan 26, 2023
Merged

URL: fix setters tests #38150

merged 2 commits into from
Jan 26, 2023

Conversation

rmisev
Copy link
Member

@rmisev rmisev commented Jan 24, 2023

These tests were introduced in the #38032

According to the URL specification U+000D is a newline character and must be removed before further processing.

These tests were introduced in the web-platform-tests#38032

According to the URL specification U+000D is a newline character and must be removed before further processing.
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

jsdom/whatwg-url passes both before and after this change, so, not sure what that says... should you just include new tests instead of changing the existing ones?

@annevk
Copy link
Member

annevk commented Jan 25, 2023

I restored the trailing U+000D test with changed expectations. If you still have a 100% pass rate there's something wrong with the test harness maybe?

@anonrig
Copy link
Member

anonrig commented Jan 26, 2023

jsdom/whatwg-url passes both before and after this change, so, not sure what that says..

The spec says that:

The protocol setter steps are to basic URL parse the given value, followed by U+003A (:), with this’s URL as url and scheme start state as state override.

The changes requested in this pull request are correct. It shouldn't have passed for jsdom/whatwg-url package since all newline characters are removed before the scheme start state. Therefore, whether or not the input includes ASCII tab and/or newline shouldn't have made a difference.

@annevk annevk merged commit f1ade79 into web-platform-tests:master Jan 26, 2023
@rmisev rmisev deleted the patch-2 branch January 26, 2023 09:54
@domenic
Copy link
Member

domenic commented Mar 25, 2025

The mystery here was almost certainly caused by the use of var in the test harness, fixed in #51369.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants