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

Jump to bottom

Extract Location object tests from query-encoding/ #10891

Merged
merged 1 commit into from
May 9, 2018
Merged

Extract Location object tests from query-encoding/ #10891

merged 1 commit into from
May 9, 2018
+49 −51

Conversation

annevk
Copy link
Member

@annevk annevk commented May 8, 2018 •
edited by wpt-pr-bot
Loading

Helps with #4934.

@wpt-pr-bot wpt-pr-bot added the html label May 8, 2018
@wpt-pr-bot wpt-pr-bot requested review from ayg, jdm, jgraham and zqzhang May 8, 2018 11:51
@annevk
Copy link
Member Author

annevk commented May 8, 2018

Bugs:

@annevk
Copy link
Member Author

annevk commented May 8, 2018

@hsivonen this relies on defaulting to windows-1252. Is that a reasonable thing to do in a test suite?

@annevk annevk mentioned this pull request May 8, 2018
Open
@annevk annevk changed the title Extra Location object tests from query-encoding/ Extract Location object tests from query-encoding/ May 9, 2018
@annevk annevk requested a review from foolip May 9, 2018 07:01
@hsivonen
Copy link
Member

hsivonen commented May 9, 2018

I think it's reasonable enough. Ideally, the instance of the test suite available to ad hoc running by users would be hosted on a non-com/org/net domain that's also not affiliated with an non-windows-1252 encoding. E.g. a .io (of to avoid .io, any of the new vanity TLDs) would work better than the current .org.

annevk
annevk commented May 9, 2018
View reviewed changes
html/infrastructure/urls/resolving-urls/query-encoding/location.sub.html
function expected(encoding) {
return "?" + {
"UTF-8": "%C3%BF",
"windows-1251": "&%23255;",
Copy link
Member Author

Choose a reason for hiding this comment

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

Either change this to align with whatwg/url#386 before landing or rebase #10915 and change it there.

@foolip
Copy link
Member

foolip commented May 9, 2018

Interesting, I came across this part of the test suite in web-platform-tests/wpt.fyi#132 by a completely different way of looking for problems. @zcorpan FYI.

foolip
foolip approved these changes May 9, 2018
View reviewed changes
Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

LGTM assuming the tests still work :)

@annevk annevk mentioned this pull request May 9, 2018
Merged
@annevk
Copy link
Member Author

annevk commented May 9, 2018

@zcorpan could you quickly skim this as well?

@annevk annevk mentioned this pull request May 9, 2018
Closed
@zcorpan
Copy link
Member

zcorpan commented May 9, 2018

Looks good to me.

@annevk annevk merged commit 42006d1 into master May 9, 2018
@annevk annevk deleted the annevk/query-encoding-location branch May 9, 2018 14:31
@annevk annevk mentioned this pull request Sep 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foolip foolip foolip approved these changes

@ayg ayg Awaiting requested review from ayg

@jdm jdm Awaiting requested review from jdm

@jgraham jgraham Awaiting requested review from jgraham

@zqzhang zqzhang Awaiting requested review from zqzhang

Assignees
No one assigned
Labels
html
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@annevk @hsivonen @foolip @zcorpan @wpt-pr-bot