Skip to content
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

url: Stop passing DOMException.prototype to URLSearchParams's constructor. #6552

Merged
merged 1 commit into from
Jul 14, 2017
Merged

url: Stop passing DOMException.prototype to URLSearchParams's constructor. #6552

merged 1 commit into from
Jul 14, 2017

Conversation

rakuco
Copy link
Member

@rakuco rakuco commented Jul 14, 2017

As of whatwg/webidl#378, both

new URLSearchParams(DOMException.prototype)
DOMException.prototype.toString()

are supposed to throw an exception due to brand checks in properties such as
"name" and "message", which meant compliant implementations were always
failing one of the tests here.

Fix it by passing a DOMException instead: it has all the constants we need
and passes the required property checks. Also assert that the previous
behavior throws a TypeError.

@annevk
Copy link
Member

annevk commented Jul 14, 2017

Safari TP does pass this test. Is that https://bugs.webkit.org/show_bug.cgi?id=174216 then? @youennf thoughts?

@ghost
Copy link

ghost commented Jul 14, 2017

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision 7cfea82
Using browser at version BuildID 20170713100259; SourceStamp 30ea2905130e85f9e1d8d56fa3097901eec6514b
Starting 10 test iterations
All results were stable

All results

1 test ran
/url/urlsearchparams-constructor.html
Subtest Results Messages
OK
Basic URLSearchParams construction PASS
URLSearchParams constructor, no arguments PASS
URLSearchParams constructor, DOMException as argument PASS
URLSearchParams constructor, empty string as argument PASS
URLSearchParams constructor, {} as argument PASS
URLSearchParams constructor, string. PASS
URLSearchParams constructor, object. PASS
Parse + PASS
Parse encoded + PASS
Parse space PASS
Parse %20 PASS
Parse \0 PASS
Parse %00 PASS
Parse � PASS
Parse %e2%8e%84 PASS
Parse � PASS
Parse %f0%9f%92%a9 PASS
Constructor with sequence of sequences of strings PASS
Construct with object with + PASS
Construct with object with two keys PASS
Construct with array with two keys PASS
Construct with object with NULL, non-ASCII, and surrogate keys PASS
Custom [Symbol.iterator] PASS

@ghost
Copy link

ghost commented Jul 14, 2017

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision 7cfea82
Using browser at version 10.0
Starting 10 test iterations
All results were stable

All results

1 test ran
/url/urlsearchparams-constructor.html
Subtest Results Messages
OK
Basic URLSearchParams construction FAIL Can't find variable: URLSearchParams
URLSearchParams constructor, no arguments FAIL Can't find variable: URLSearchParams
URLSearchParams constructor, DOMException as argument FAIL Can't find variable: URLSearchParams
URLSearchParams constructor, empty string as argument FAIL Can't find variable: URLSearchParams
URLSearchParams constructor, {} as argument FAIL Can't find variable: URLSearchParams
URLSearchParams constructor, string. FAIL Can't find variable: URLSearchParams
URLSearchParams constructor, object. FAIL Can't find variable: URLSearchParams
Parse + FAIL Can't find variable: URLSearchParams
Parse encoded + FAIL Can't find variable: URLSearchParams
Parse space FAIL Can't find variable: URLSearchParams
Parse %20 FAIL Can't find variable: URLSearchParams
Parse \0 FAIL Can't find variable: URLSearchParams
Parse %00 FAIL Can't find variable: URLSearchParams
Parse � FAIL Can't find variable: URLSearchParams
Parse %e2%8e%84 FAIL Can't find variable: URLSearchParams
Parse � FAIL Can't find variable: URLSearchParams
Parse %f0%9f%92%a9 FAIL Can't find variable: URLSearchParams
Constructor with sequence of sequences of strings FAIL Can't find variable: URLSearchParams
Construct with object with + FAIL Can't find variable: URLSearchParams
Construct with object with two keys FAIL Can't find variable: URLSearchParams
Construct with array with two keys FAIL Can't find variable: URLSearchParams
Construct with object with NULL, non-ASCII, and surrogate keys FAIL Can't find variable: URLSearchParams
Custom [Symbol.iterator] FAIL Can't find variable: URLSearchParams

@ghost
Copy link

ghost commented Jul 14, 2017

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision 7cfea82
Using browser at version 61.0.3153.4 dev
Starting 10 test iterations
All results were stable

All results

1 test ran
/url/urlsearchparams-constructor.html
Subtest Results Messages
OK
Basic URLSearchParams construction PASS
URLSearchParams constructor, no arguments PASS
URLSearchParams constructor, DOMException as argument PASS
URLSearchParams constructor, empty string as argument PASS
URLSearchParams constructor, {} as argument PASS
URLSearchParams constructor, string. PASS
URLSearchParams constructor, object. PASS
Parse + PASS
Parse encoded + PASS
Parse space PASS
Parse %20 PASS
Parse \0 PASS
Parse %00 PASS
Parse ⎄ PASS
Parse %e2%8e%84 PASS
Parse 💩 PASS
Parse %f0%9f%92%a9 PASS
Constructor with sequence of sequences of strings PASS
Construct with object with + PASS
Construct with object with two keys PASS
Construct with array with two keys PASS
Construct with object with NULL, non-ASCII, and surrogate keys PASS
Custom [Symbol.iterator] PASS

@ghost
Copy link

ghost commented Jul 14, 2017

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision 7cfea82
Using browser at version 14.14393
Starting 10 test iterations
All results were stable

All results

1 test ran
/url/urlsearchparams-constructor.html
Subtest Results Messages
OK
Basic URLSearchParams construction FAIL 'URLSearchParams' is undefined
URLSearchParams constructor, no arguments FAIL 'URLSearchParams' is undefined
URLSearchParams constructor, DOMException as argument FAIL 'URLSearchParams' is undefined
URLSearchParams constructor, empty string as argument FAIL 'URLSearchParams' is undefined
URLSearchParams constructor, {} as argument FAIL 'URLSearchParams' is undefined
URLSearchParams constructor, string. FAIL 'URLSearchParams' is undefined
URLSearchParams constructor, object. FAIL 'URLSearchParams' is undefined
Parse + FAIL 'URLSearchParams' is undefined
Parse encoded + FAIL 'URLSearchParams' is undefined
Parse space FAIL 'URLSearchParams' is undefined
Parse %20 FAIL 'URLSearchParams' is undefined
Parse \0 FAIL 'URLSearchParams' is undefined
Parse %00 FAIL 'URLSearchParams' is undefined
Parse ⎄ FAIL 'URLSearchParams' is undefined
Parse %e2%8e%84 FAIL 'URLSearchParams' is undefined
Parse 💩 FAIL 'URLSearchParams' is undefined
Parse %f0%9f%92%a9 FAIL 'URLSearchParams' is undefined
Constructor with sequence of sequences of strings FAIL 'URLSearchParams' is undefined
Construct with object with + FAIL 'URLSearchParams' is undefined
Construct with object with two keys FAIL 'URLSearchParams' is undefined
Construct with array with two keys FAIL 'URLSearchParams' is undefined
Construct with object with NULL, non-ASCII, and surrogate keys FAIL 'URLSearchParams' is undefined
Custom [Symbol.iterator] FAIL 'URLSearchParams' is undefined

@rakuco
Copy link
Member Author

rakuco commented Jul 14, 2017

Safari TP does pass this test. Is that https://bugs.webkit.org/show_bug.cgi?id=174216 then?

I think so, Domenic filed that one after the spec change was merged.

@annevk
Copy link
Member

annevk commented Jul 14, 2017

Maybe also add another test that ensures DOMException.prototype throws the correct exception.

@rakuco
Copy link
Member Author

rakuco commented Jul 14, 2017

Isn't that already covered by #6361?

@annevk
Copy link
Member

annevk commented Jul 14, 2017

Generally yes, but not the specific binding for this feature. It's always a little bit unclear to me how much we should test the binding on a per-feature basis, but some spot testing doesn't hurt I think.

@rakuco
Copy link
Member Author

rakuco commented Jul 14, 2017

OK, so you mean specifically testing that new URLSearchParams(DOMException.prototype) throws a TypeError? I can do that, though personally I don't see anything specific to this, i.e. it doesn't seem different than asserting that record<K,V> conversions with prototype objects with attributes throw anywhere else.

@annevk
Copy link
Member

annevk commented Jul 14, 2017

Yeah, that's what I meant. I think it's in the same realm as testing that DOMException itself stringifies here (and how). Maybe we shouldn't test either though, but I think it does help catch bugs if the bindings end up being implemented differently at some point.

…ctor.

As of whatwg/webidl#378, both

    new URLSearchParams(DOMException.prototype)
    DOMException.prototype.toString()

are supposed to throw an exception due to brand checks in properties such as
"name" and "message", which meant compliant implementations were always
failing one of the tests here.

Fix it by passing a `DOMException` instead: it has all the constants we need
and passes the required property checks. Also assert that the previous
behavior throws a TypeError.
@rakuco
Copy link
Member Author

rakuco commented Jul 14, 2017

I've added an assert_throws there to cover DOMException.prototype, please take another look.

@annevk annevk merged commit 125950d into web-platform-tests:master Jul 14, 2017
@rakuco rakuco deleted the urlsearchparams-constructor-domexception branch July 14, 2017 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants