url: ensure host setter matches parse for file url

[jasnell]

Technically, file URLs are not permitted to have a port. There is
currently an ambiguity in the URL spec. In the current spec
having a port in a file URL is undefined behavior. In the current
implementation, the port is ignored and handled as if it were part
of the host name. This will be changing once the ambiguity is
resolved in the spec. The spec change may involve either ignoring the
port if specified or throwing with an Invalid URL error if the port
is specified. For now, this test verifies the currently implemented
behavior.

Fixes: #10608

/cc @domenic

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

url

[domenic]

It'd be ideal if these tests were in the urltestdata.json and url_setters.json files (or in some kind of urltestdata-to-upstream.json supplement) so they could be upstreamed and shared among all implementations.

[jasnell]

Yeah, I'd considered that but wanted to hold off just a bit before doing so until there was more clarity on what the expected spec changes will be. Validating the current underspecified behavior does not seem too worthwhile since there are variances among the implementations on how to handle it

[jasnell]

That said, there's no reason not to add them to our own copies of those files ;-)

[domenic]

Yeah, in jsdom/whatwg-url we have a to-upstream/urltestdata.json which we run, in addition to the official urltestdata.json (which we download in the pretest script from a known hash). Then we periodically go through and send PRs to web-platform-tests, and when they're accepted, empty out to-upstream/urltestdata.json.

[jasnell]

Ok, added a commit that adds the additional tests to the files. The file names are different but the patch should apply clean once adjusted for the file path differences.

[TimothyGu]

cc @nodejs/url

[joyeecheung]

Is this ready to be reviewed? Or do we wait until the spec changes land?

[jasnell]

Waiting for spec changes. This pr is incomplete

…

On Fri, Jan 13, 2017 at 10:41 PM Joyee Cheung ***@***.***> wrote: Is this ready to be reviewed? Or do we wait until the spec changes land? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10615 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2efe_JCeJ87p4awLyG-LOp4Vjeu2gks5rSG4TgaJpZM4La624> .

[watilde]

@jasnell Could you please rebase this to review?

[jasnell]

@watilde ... rebased!

[TimothyGu]

This should be made obsoleted by #12523, which adds similar logic in the C++ layer.

[jasnell]

@TimothyGu ... happy to close this! Looking forward to #12523 landing!