Add tests for URL setters #2830

SimonSapin · 2016-04-13T19:30:22Z

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 · 2016-04-13T19:30:24Z

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

hoppipolla-critic-bot · 2016-04-13T19:30:26Z

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 · 2016-04-14T09:41:45Z

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

SimonSapin · 2016-04-14T12:15:51Z

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

domenic · 2016-04-15T21:59:05Z

url/url-setters.html

+        for (var attr in test_case.expected) {
+          assert_equals(url[attr], test_case.expected[attr])
+        }
+      }, "Setting <" + test_case.href + ">." + attr + " = '" + test_case.new_value + "' " + test_case.comment)


Don't append comment if it's undefined

domenic · 2016-04-15T22:06:48Z

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://me@example.net>.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 · 2016-04-15T22:13:30Z

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.

…ent)

domenic · 2016-04-15T22:49:52Z

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://me@example.net>.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 · 2016-04-16T00:40:49Z

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 · 2016-04-16T02:28:23Z

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 · 2016-04-16T02:39:09Z

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 · 2016-04-16T02:44:39Z

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".

See #2830 (comment)

SimonSapin · 2016-04-17T14:23:29Z

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

I’ve added a separate commit to do this.

domenic · 2016-04-26T00:10:33Z

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.

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.)

See #2830 (comment). This can be un-reverted when the following are fixed in the spec: - whatwg/url#113 - whatwg/url#104

As discussed in web-platform-tests/wpt#2830 (comment). This makes the setter tests pass (by having most of them ignore invalid input instead of throwing). It should also make parsing failures during construction throw correctly; previously they were checking a nonexistant urlRecord.failure property. This changes the public API by making the URL-record-returning functions return either a URL record or the string "failure", instead of returning either a URL record or throwing.

See web-platform-tests#2830 (comment). This can be un-reverted when the following are fixed in the spec: - whatwg/url#113 - whatwg/url#104

wpt-pr-bot added url wg-webapps labels Apr 13, 2016

domenic mentioned this pull request Apr 15, 2016

hostname setter does not work because of wrong state override. jsdom/whatwg-url#51

Closed

domenic reviewed Apr 15, 2016
View reviewed changes

domenic added a commit to jsdom/whatwg-url that referenced this pull request Apr 15, 2016

try/catch, but it's suboptimal; see web-platform-tests/wpt#2830 (comm…

440c867

…ent)

SimonSapin force-pushed the url-setters branch from 7d202a7 to 1df8fa8 Compare April 16, 2016 00:39

SimonSapin added a commit that referenced this pull request Apr 17, 2016

Revert spec deviations for now.

b37554a

See #2830 (comment)

zcorpan assigned SimonSapin Apr 26, 2016

SimonSapin added 2 commits May 10, 2016 23:42

Add tests for URL setters

9dddf35

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.)

Revert URL setter test spec deviations for now

4ec7252

See #2830 (comment). This can be un-reverted when the following are fixed in the spec: - whatwg/url#113 - whatwg/url#104

domenic force-pushed the url-setters branch from b37554a to 4ec7252 Compare May 11, 2016 03:45

domenic merged commit 4ec7252 into master May 11, 2016

domenic deleted the url-setters branch May 11, 2016 03:46

ivanzr pushed a commit to ivanzr/web-platform-tests that referenced this pull request Jun 29, 2016

Revert URL setter test spec deviations for now

82451ce

See web-platform-tests#2830 (comment). This can be un-reverted when the following are fixed in the spec: - whatwg/url#113 - whatwg/url#104

annevk mentioned this pull request Feb 15, 2017

URL setters test #1408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for URL setters #2830

Add tests for URL setters #2830

SimonSapin commented Apr 13, 2016

wpt-pr-bot commented Apr 13, 2016

hoppipolla-critic-bot commented Apr 13, 2016

zcorpan commented Apr 14, 2016

SimonSapin commented Apr 14, 2016

domenic Apr 15, 2016

domenic commented Apr 15, 2016 •

edited

Loading

domenic commented Apr 15, 2016

domenic commented Apr 15, 2016

SimonSapin commented Apr 16, 2016

domenic commented Apr 16, 2016

Sebmaster commented Apr 16, 2016 •

edited

Loading

SimonSapin commented Apr 16, 2016

SimonSapin commented Apr 17, 2016

domenic commented Apr 26, 2016

Add tests for URL setters #2830

Add tests for URL setters #2830

Conversation

SimonSapin commented Apr 13, 2016

wpt-pr-bot commented Apr 13, 2016

hoppipolla-critic-bot commented Apr 13, 2016

zcorpan commented Apr 14, 2016

SimonSapin commented Apr 14, 2016

domenic Apr 15, 2016

Choose a reason for hiding this comment

domenic commented Apr 15, 2016 • edited Loading

domenic commented Apr 15, 2016

domenic commented Apr 15, 2016

SimonSapin commented Apr 16, 2016

domenic commented Apr 16, 2016

Sebmaster commented Apr 16, 2016 • edited Loading

SimonSapin commented Apr 16, 2016

SimonSapin commented Apr 17, 2016

domenic commented Apr 26, 2016

domenic commented Apr 15, 2016 •

edited

Loading

Sebmaster commented Apr 16, 2016 •

edited

Loading