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

Move some tests from toascii.json to urltestdata.json #38813

Merged
merged 3 commits into from
Mar 7, 2023
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Mar 4, 2023

These were introduced in e9a1061. However, these are not tests of the Unicode ToASCII algorithm. They are tests of the extra step introduced in whatwg/url@cceb435 to the URL Standard.

Keep toascii.json focused on testing the ToASCII algorithm, so that software which implements it can use that file.

/cc @rmisev

These were introduced in e9a1061. However, these are not tests of the Unicode ToASCII algorithm. They are tests of the extra step introduced in whatwg/url@cceb435 to the URL Standard.

Keep toascii.json focused on testing the ToASCII algorithm, so that software which implements it can use that file.
@rmisev
Copy link
Member

rmisev commented Mar 4, 2023

I agree with moving these tests from toascii.json, but in WPT they are also used to test host and hostname setters.
So maybe add them to the setters_tests.json as well?

@rmisev
Copy link
Member

rmisev commented Mar 5, 2023

Now an idea came to mind: I think even better to move these tests to a new file, say host_parser_tests.json and use it in WPT the same way as toascii.json. This might be useful for testing the future URLHost class.
What do you think?

/cc @annevk

@domenic
Copy link
Member Author

domenic commented Mar 6, 2023

I think that might be reasonable, but I'm quickly reaching the limit of the extra effort I'm willing to put in here myself. I'm just trying to correct a bug so that my ToASCII library can be tested properly...

},
{
"comment": "Empty host after domain to ASCII",
"input": "https://\u00AD/",
Copy link
Member

@rmisev rmisev Mar 6, 2023

Choose a reason for hiding this comment

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

Add required base: parameter, the same for remaining two tests.

Copy link
Member

@rmisev rmisev left a comment

Choose a reason for hiding this comment

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

Overall OK, you just skipped the required base: parameter in the last three tests in the urltestdata.json file.

@annevk annevk merged commit 5d5a731 into master Mar 7, 2023
@annevk annevk deleted the move-idna-tests branch March 7, 2023 17:59
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.

5 participants