Test new URLSearchParams constructor features #4523

annevk · 2017-01-11T10:07:00Z

The main thing here is new tests for
whatwg/url#175 but I also found issues
with some existing tests that I’m attempting to fix at the same time.

The current tests assumed the constructor argument was mandatory, but
it’s in fact optional. The other issue was that the current test assume
stringifying DOMException.prototype throws, but I don’t think that’s
the case.

The main thing here is new tests for https://github.com/whatwg/url/pull/175/files but I also found issues with some existing tests that I’m attempting to fix at the same time. The current tests assumed the constructor argument was mandatory, but it’s in fact optional. The other issue was that the current test assume stringifying DOMException.prototype throws, but I don’t think that’s the case.

wpt-pr-bot · 2017-01-11T10:07:01Z

Notifying @Sebmaster, @domenic, @mikewest, @rubys, @sideshowbarker, @smola, @tomalec, @xiaojunwu, and @zcorpan. (Learn how reviewing works.)

wpt-stability-bot · 2017-01-11T10:13:57Z

Chrome

Testing revision df3a098
Starting 10 test iterations
All results were stable

All results

/url/urlsearchparams-constructor.html

Subtest	Results
	OK
`Custom [Symbol.iterator]`	FAIL
`URLSearchParams constructor, {} as argument`	FAIL
`Parse space`	PASS
`Parse +`	PASS
`URLSearchParams constructor, no arguments`	PASS
`Parse 💩`	PASS
`URLSearchParams constructor, empty string as argument`	PASS
`Parse %00`	PASS
`Parse %f0%9f%92%a9`	PASS
`URLSearchParams constructor, string.`	PASS
`Parse %e2%8e%84`	PASS
`Basic URLSearchParams construction`	PASS
`Parse ⎄`	PASS
`Construct with array with two keys`	FAIL
`Parse %20`	PASS
`URLSearchParams constructor, DOMException.prototype as argument`	FAIL
`Construct with object with +`	FAIL
`Parse \0`	PASS
`Construct with object with two keys`	FAIL
`URLSearchParams constructor, object.`	PASS

wpt-stability-bot · 2017-01-11T10:14:07Z

Firefox

Testing revision df3a098
Starting 10 test iterations
All results were stable

All results

/url/urlsearchparams-constructor.html

Subtest	Results
	OK
`Custom [Symbol.iterator]`	FAIL
`URLSearchParams constructor, {} as argument`	FAIL
`Parse space`	PASS
`Parse +`	PASS
`URLSearchParams constructor, no arguments`	PASS
`Parse 💩`	PASS
`URLSearchParams constructor, empty string as argument`	PASS
`Parse %00`	PASS
`Parse %f0%9f%92%a9`	PASS
`URLSearchParams constructor, string.`	PASS
`Parse %e2%8e%84`	PASS
`Basic URLSearchParams construction`	PASS
`Parse ⎄`	PASS
`Construct with array with two keys`	FAIL
`Parse %20`	PASS
`URLSearchParams constructor, DOMException.prototype as argument`	PASS
`Construct with object with +`	FAIL
`Parse \0`	PASS
`Construct with object with two keys`	FAIL
`URLSearchParams constructor, object.`	PASS

domenic · 2017-01-11T13:37:42Z

url/urlsearchparams-constructor.html

    assert_true(params != null, 'constructor returned non-null value.');
    assert_equals(params.__proto__, URLSearchParams.prototype, 'expected URLSearchParams.prototype as prototype.');
    params = new URLSearchParams({});
    assert_equals(params + '', '%5Bobject+Object%5D=');
-}, 'URLSearchParams constructor, empty.');
+}, 'URLSearchParams constructor, empty and unusual input.');


Separate tests per input will make failures clearer.

Done this as well.

domenic · 2017-01-11T13:41:46Z

url/urlsearchparams-constructor.html

+  let params3 = new URLSearchParams(params2)
+  assert_equals(params3.get("a"), params1.get("a"))
+  assert_equals(params3.get("a"), "b")
+}, "Evil constructor test")


I don't think this works as expected. params1[Symbol.iterator] === params2[Symbol.iterator] === URLSearchParams.prototype[Symbol.iterator]. The assignment is a no-op. The iterator will look up the private data on the instance it's invoked on, so iterating over params2 will always give no params, so params3 should be empty.

Oh, shit. I based that on #4240 which I also wrote. How would I create a custom iterator? Would assigning an array be sufficient?

The easiest way is to use a generator:

params3[Symbol.iterator] = function *() { yield [a, b]; yield [c, d]; };

Thanks, used.

Per feedback in #4523 the original test would never be able to pass.

domenic · 2017-01-12T00:04:00Z

url/urlsearchparams-constructor.html

    assert_true(params != null, 'constructor returned non-null value.');
    assert_equals(params.__proto__, URLSearchParams.prototype, 'expected URLSearchParams.prototype as prototype.');
+}, "URLSearchParams constructor, empty string as argument")
+
+test(() => {
    params = new URLSearchParams({});
    assert_equals(params + '', '%5Bobject+Object%5D=');


I think this should now be equivalent to no arguments per record semantics. So just empty string.

domenic · 2017-01-12T00:04:29Z

url/urlsearchparams-constructor.html

+  }
+  let params2 = new URLSearchParams(params)
+  assert_equals(params2.get("a"), "b")
+}, "Evil constructor test")


Maybe "overridden iterator test" is now more accurate.

annevk · 2017-01-12T06:35:27Z

Thanks!

Tests: web-platform-tests/wpt#4523. Fixes #27.

cdumez · 2017-01-20T02:52:10Z

url/urlsearchparams-constructor.html

+
+test(() => {
+    params = new URLSearchParams(DOMException.prototype);
+    assert_equals(params.toString(), "Error=")


Can you clarify why this is the expected result here. My interpretation of the Web IDL spec [1] is that we should go to step 11.1 and treat the input as a sequence (not a String).

[1] https://heycam.github.io/webidl/#es-union

Yes but method is undefined so it then continues on with the rest of the steps.

How about 11.4 ? We do have a record in there.

Yeah you're right. I think this is indeed incorrect.

#4581 has the fix. I also commented on the WebKit bug.

TimothyGu · 2017-01-28T19:20:45Z

url/urlsearchparams-constructor.html

@@ -124,6 +134,30 @@
    params = new URLSearchParams('a%f0%9f%92%a9b=c');
    assert_equals(params.get('a\uD83D\uDCA9b'), 'c');
 }, 'Parse %f0%9f%92%a9');  // Unicode Character 'PILE OF POO' (U+1F4A9)
+
+;[
+  { "input": {"+": "%C2"}, "output": [[" ", "\uFFFD"]], "name": "object with +" },


@annevk, I'm in the process of implementing the URL spec change in Node.js. Where does it say in the URL Standard that keys and values of a record must be unescaped?

That's a good question, it doesn't and I'm not sure it should. I filed whatwg/url#220.

Fixes: nodejs#10635 Refs: whatwg/url#175 Refs: web-platform-tests/wpt#4523

PR-URL: #11060 Fixes: #10635 Ref: whatwg/url#175 Ref: web-platform-tests/wpt#4523 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs#11060 Fixes: nodejs#10635 Ref: whatwg/url#175 Ref: web-platform-tests/wpt#4523 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

wpt-pr-bot added url wg-webplatform labels Jan 11, 2017

domenic requested changes Jan 11, 2017

View reviewed changes

address review comments

76ac826

annevk added a commit that referenced this pull request Jan 11, 2017

Correct Headers.prototype[Symbol.iterator] test

cbc2f44

Per feedback in #4523 the original test would never be able to pass.

annevk mentioned this pull request Jan 11, 2017

Correct Headers.prototype[Symbol.iterator] test #4524

Merged

jdm pushed a commit that referenced this pull request Jan 11, 2017

Correct Headers.prototype[Symbol.iterator] test

ee7ffe3

Per feedback in #4523 the original test would never be able to pass.

domenic reviewed Jan 12, 2017

View reviewed changes

more excellent review feedback

697198f

domenic approved these changes Jan 12, 2017

View reviewed changes

annevk merged commit 74a75c9 into master Jan 12, 2017

annevk deleted the annevk/urlsearchparams-constructor branch January 12, 2017 16:12

annevk added a commit to whatwg/url that referenced this pull request Jan 12, 2017

Simplify constructing URLSearchParams

31ddc5b

Tests: web-platform-tests/wpt#4523. Fixes #27.

cdumez reviewed Jan 20, 2017

View reviewed changes

TimothyGu reviewed Jan 28, 2017

View reviewed changes

TimothyGu added a commit to TimothyGu/node that referenced this pull request Jan 28, 2017

url: extend URLSearchParams constructor

2300a51

Fixes: nodejs#10635 Refs: whatwg/url#175 Refs: web-platform-tests/wpt#4523

TimothyGu mentioned this pull request Jan 28, 2017

url: extend URLSearchParams constructor nodejs/node#11060

Closed

4 tasks

annevk mentioned this pull request Jan 29, 2017

URLSearchParams constructor test has a bug whatwg/url#220

Closed

CYBAI mentioned this pull request Dec 25, 2018

Construct URLSearchParams from array or object servo/servo#22554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test new URLSearchParams constructor features #4523

Test new URLSearchParams constructor features #4523

annevk commented Jan 11, 2017 •

edited

Loading

wpt-pr-bot commented Jan 11, 2017

wpt-stability-bot commented Jan 11, 2017 •

edited

Loading

wpt-stability-bot commented Jan 11, 2017 •

edited

Loading

domenic Jan 11, 2017

annevk Jan 11, 2017

domenic Jan 11, 2017

annevk Jan 11, 2017

domenic Jan 11, 2017

annevk Jan 11, 2017

domenic Jan 12, 2017

domenic Jan 12, 2017

annevk commented Jan 12, 2017

cdumez Jan 20, 2017

domenic Jan 20, 2017

cdumez Jan 20, 2017

domenic Jan 20, 2017

annevk Jan 20, 2017

TimothyGu Jan 28, 2017

annevk Jan 29, 2017

Test new URLSearchParams constructor features #4523

Test new URLSearchParams constructor features #4523

Conversation

annevk commented Jan 11, 2017 • edited Loading

wpt-pr-bot commented Jan 11, 2017

wpt-stability-bot commented Jan 11, 2017 • edited Loading

Chrome

All results

wpt-stability-bot commented Jan 11, 2017 • edited Loading

Firefox

All results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Jan 11, 2017 •

edited

Loading

wpt-stability-bot commented Jan 11, 2017 •

edited

Loading

wpt-stability-bot commented Jan 11, 2017 •

edited

Loading