Add tests for URL setters

[SimonSapin]

A lot of them fail in browsers, but I believe they’re should pass in a spec-conforming implementation. (With a few exceptions noted in comments.)

CC @domenic

[wpt-pr-bot]

Reviewers for this pull request are: @Sebmaster, @domenic, @frewsxcv, @mikewest, @rubys, @sideshowbarker, @smola, @tomalec, @xiaojunwu, and @zcorpan.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/6391

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[zcorpan]

The deviations are only for whatwg/url#104 right?

[SimonSapin]

That and whatwg/url#113 and whatwg/url#114.

[domenic]

Don't append comment if it's undefined

[domenic]

I ran this against https://github.com/jsdom/whatwg-url and got the following failures:

<a://example.net>.protocol = "bC0+-."
<http://example.net>.protocol = "b" Spec deviation: from special scheme to not is not problematic. https://github.com/whatwg/url/issues/104
<ssh://[email protected]>.protocol = "http" Spec deviation: from non-special scheme with a host to special is not problematic. https://github.com/whatwg/url/issues/104
<http://example.net:8080>.host = "example.com:" Port number is removed if empty in the new value: https://github.com/whatwg/url/pull/113
<http://example.net>.host = "" The empty host is not valid for special schemes
<view-source+http://example.net/path>.host = "example.com\stuff" \ is not a delimiter for non-special schemes, and it’s invalid in a domain
<http://example.net/path>.host = "example.com:65536" Port numbers are 16 bit integers, overflowing is an error. Hostname is still set, though.
<http://example.net>.hostname = "" The empty host is not valid for special schemes
<view-source+http://example.net/path>.hostname = "example.com\stuff" \ is not a delimiter for non-special schemes, and it’s invalid in a domain
<http://example.net:8080>.port = "" Port number is removed if empty in the new value: https://github.com/whatwg/url/pull/113
<http://example.net:8080/path>.port = "65536" Port numbers are 16 bit integers, overflowing is an error

Some of these may be bugs in jsdom/whatwg-url; after all, we didn't have tests. Some of them are noted as spect violations already. But some of them seem like potential test bugs. For example, I cannot find any spec support for

<http://example.net:8080>.port = "" Port number is removed if empty in the new value

[domenic]

A bunch of tests were fixed by adding a try/catch around the setter line. It seems like the test format does not encode cases where the setter throws, instead just leaving them as expected = result of parsing href. I think it should instead have an explicit "expected": "failure" format similar to urltestdata.json.

[domenic]

OK, I found one bug in our scheme parsing state. The remaining list of things where either jsdom/whatwg-url has a bug or the spec is not matched, accounting for the failure to distinguish throwing setters from no-ops as mentioned above, is these four:

<http://example.net>.protocol = "b" Spec deviation: from special scheme to not is not problematic. https://github.com/whatwg/url/issues/104
<ssh://[email protected]>.protocol = "http" Spec deviation: from non-special scheme with a host to special is not problematic. https://github.com/whatwg/url/issues/104
<http://example.net:8080>.host = "example.com:" Port number is removed if empty in the new value: https://github.com/whatwg/url/pull/113
<http://example.net:8080>.port = "" Port number is removed if empty in the new value: https://github.com/whatwg/url/pull/113

I count two spec deviations plus the problem I mentioned above where I can't find where in the spec the port number is removed if empty is the new value.

[SimonSapin]

Re undefined comment and shadowing attr: done.

The url.port = "" case is also a deliberate spec deviation per whatwg/url#113. I’ve marked it as such. But maybe it’s better for this PR to not include any spec deviation, and update tests later if the spec changes?

Re exceptions, none of the setters (except href which is not tested here) throw per https://url.spec.whatwg.org/#api if I’m reading correctly. (Compare "If parsedURL is failure, throw a TypeError exception." in the href setter v.s. the lack of such a step in other setters, meaning a parser failure there is silently ignored.) So an implementation throwing should make the test fail. (Unless of course the spec is wrong and needs to be changed.)

[domenic]

But maybe it’s better for this PR to not include any spec deviation, and update tests later if the spec changes?

Yeah, probably; we could just wait to land this until the spec gets updated, of course.

So an implementation throwing should make the test fail.

Oh, very interesting. @Sebmaster any reason we implemented this as throws?

[Sebmaster]

Seems like we've been doing this wrong since jsdom/whatwg-url#36, since we started using the basicURLParse helper in setters which throws 😕

[SimonSapin]

FWIW https://url.spec.whatwg.org/#concept-basic-url-parser returns a value that is either an URL record or "failure". Algorithms in this spec don’t have a concept of exceptions or stack unwinding, error handling is based on explicit "return values".

[SimonSapin]

But maybe it’s better for this PR to not include any spec deviation?

I’ve added a separate commit to do this.

[domenic]

This LGTM then with the for-now revert. Will let you merge if you want to do any last-minute touchups or similar.

I wish we'd caught the throw vs. silent failure issue when designing the URL API. Loud failure would have been much nicer for authors. Oh well.