-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add test cases for URL empty query and fragment #25829
Add test cases for URL empty query and fragment #25829
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! @domenic want to double check with whatwg-url?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are incorrect because they use the about:blank
base. What we really need to test is the no base case.
Since the only place no-base is testable is in new URL()
, I think urltestdata.json
is probably not the right place to put this. A separate .js
file would work.
Or, if we did want to put them in urltestdata.json
, then they need to omit the "base"
key, and all the test harness code that uses urltestdata.json
needs to get updated to skip such "base"
-less test cases, unless they are testing the URL constructor.
I think we can update |
I'll add a new js file for no base URL if my updated commit doesn't look good. |
@@ -5,8 +5,7 @@ function setBase(base) { | |||
} | |||
|
|||
function bURL(url, base) { | |||
base = base || "about:blank" | |||
setBase(base) | |||
if (base) setBase(base) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work. It will leave the base URL for the document as the URL it is loaded in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we would have to skip this type of test when the base is lacking (and say we do it because you cannot unset the base URL of a document).
@@ -5,7 +5,7 @@ | |||
<div id=log></div> | |||
<script> | |||
function bURL(url, base) { | |||
return new URL(url, base || "about:blank") | |||
return base ? new URL(url, base) : new URL(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does work.
@watilde are you still planning on pushing this forward? It would be nice to see it merged. |
@guybedford Thank you for picking up this one! |
…ent tests, a=testonly Automatic update from web-platform-tests Test URLs with empty query and fragment Closes whatwg/url#539. Closes web-platform-tests/wpt#25829 by superseding it. Co-authored-by: Daijiro Wachi <daijiro.wachi@gmail.com> -- wpt-commits: 6315c7757a6675f39262167c8196ce562f2a6778 wpt-pr: 29579
…ent tests, a=testonly Automatic update from web-platform-tests Test URLs with empty query and fragment Closes whatwg/url#539. Closes web-platform-tests/wpt#25829 by superseding it. Co-authored-by: Daijiro Wachi <daijiro.wachi@gmail.com> -- wpt-commits: 6315c7757a6675f39262167c8196ce562f2a6778 wpt-pr: 29579
…ent tests, a=testonly Automatic update from web-platform-tests Test URLs with empty query and fragment Closes whatwg/url#539. Closes web-platform-tests/wpt#25829 by superseding it. Co-authored-by: Daijiro Wachi <daijiro.wachi@gmail.com> -- wpt-commits: 6315c7757a6675f39262167c8196ce562f2a6778 wpt-pr: 29579
…ent tests, a=testonly Automatic update from web-platform-tests Test URLs with empty query and fragment Closes whatwg/url#539. Closes web-platform-tests/wpt#25829 by superseding it. Co-authored-by: Daijiro Wachi <daijiro.wachi@gmail.com> -- wpt-commits: 6315c7757a6675f39262167c8196ce562f2a6778 wpt-pr: 29579
Refs: whatwg/url#539