Change how invalid base URLs are tested #10955
Conversation
Fixes #8707. Supercedes #10917, which it is based on. This keeps the tests inside urltestdata.json, but adds a new requirement on consumers of that file that they should also test their base URL parsing logic by reusing the data in a different way. It also rearranges the various cases from 90efdbe under several different headings.
wpt-pr-bot
requested review from
GPHemsley,
mikewest,
rubys,
Sebmaster,
smola,
TimothyGu and
xiaojunwu
url/urltestdata.json
Outdated
| @@ -6521,27 +6521,34 @@ | |||
| "search": "?a", | |||
| "hash": "#%GH" | |||
| }, | |||
| "Bad bases", | |||
| "URLs that require a non-about:blank base. (Also serve as invalid base tests.)" | |||
There was a problem hiding this comment.
This is missing a trailing comma.
There was a problem hiding this comment.
Thanks, I should actually test my changes against jsdom/whatwg-url as I do for everyone else's to this file -_-
url/urltestdata.json
Outdated
| "base": "a:b", | ||
| "failure": true | ||
| }, | ||
| "Other base URL tests, that must succeed" |
|
JSON fixed; confirmed working in jsdom/whatwg-url#116. Filed nodejs/node#20720 to alert Node.js folks. Will merge this after Travis finishes. |
|
I'd wanted to test these changes before I commented on the clarity of the readme... I'm not entirely clear on what's changing. Are you saying that the base URL needs to be parsed the same as the input? And that failing to parse the base URL should fail the whole thing? If so, I think I already do that... |
|
Sorry, I didn't realize :(. That's basically what I'm saying, yes. I imagine most people already do this, but the test cases were previously not testing it explicitly. They had a few examples of invalid base URLs, but now we're encouraging testing all invalid URLs as invalid base URLs. |
…lure When runnning test cases in urltestdata.json. Follows web-platform-tests/wpt#10955
Fixes #8707. Supercedes #10917, which it is based on.
This keeps the tests inside urltestdata.json, but adds a new requirement on consumers of that file that they should also test their base URL parsing logic by reusing the data in a different way.
It also rearranges the various cases from 90efdbe under several different headings.
/cc @GPHemsley and @TimothyGu especially to review the changes to the README to see if they're clear on how to maintain and improve coverage. Existing harnesses will lose coverage from this change, since urltestdata.json no longer tests the
a,a/, anda//base URLs directly. But if they implement the tweak suggested in the README, they will gain coverage, testing not only those URLs as bases but also lots of others already in the file.