url, test: synchronize web-platform-test url tests by joyeecheung · Pull Request #11079 · nodejs/node · GitHub

This is still a WIP, I'll update all the URLSearchParams tests first, then see how we can synchronize other tests. But I would like to get some feedback from @nodejs/url just to make sure I'm on the right direction.

This is an attempt to synchronize the tests of our WHATWG URL implementation with the tests in https://github.com/w3c/web-platform-tests/tree/master/url. #9484 ported some of the URLSearchParams tests into our code base, but it is somewhat hard to see how up-to-date we are compared with the upstream tests.

In this PR, the upstream tests are placed like this

'use strict';

const common = require('../common');
const assert = require('assert');
const URLSearchParams = require('url').URLSearchParams;

const { test, assert_equals, assert_true } = common.WPT;  // port WPT test utilities

/* eslint-disable */
// Disable eslint because WPT tests have different styles
/* WPT Refs:
   https://github.com/w3c/web-platform-tests/blob/8791bed/url/urlsearchparams-append.html
*/
// The last version of this test suite identified as compatible

// Pure copy-paste of the WPT test
// with incompatible/non-appliable tests comment out

/* eslint-enable */

// Tests below are not from WPT.
// We can also identify tests that can be merged upstream here.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test, url-whatwg

Completed. Request review from @nodejs/url

A few tests are out of sync/not sync'ed:

urlsearchparams-sort.html: see url: add sort() to URLSearchParams #10760
urlsearchparams-constructor.html: see url: extend URLSearchParams constructor (whatwg #175) #10635
urlencoded-parser.html: application/x-www-form-urlencoded parser test, doesn't apply
a-element-*: doesn't apply
historical.worker.js: duplicate of historical.html in the context of Node

The test data is out of sync too, I've updated setters_tests.json to the latest compatible version, later versions of setters_tests.json and urltestdata.json already have their tracking issues.

There are some tests commented out because we are not compatible yet, the major blocker is #10454 (PR: #10967) and the encoding of space to + in search params. The major blocker of test data jsons is #10655

Force pushed because of filename mistakes.

CI: https://ci.nodejs.org/job/node-test-pull-request/6130/

LGTM with a couple nits

Perhaps remove the commented out code sections?

I think just commenting them out would make it easier to update and easier to compare with the upstream, maybe just me though

nit: template string may be better here

Just copy-pasting the upstream. I think they don't do ES.Next features for browser compatibility?

Ok, that's fine then

Can we upstream these to web-platform-tests? I can do it if that'd be easier.

I can give it a try first :D. These test cases don't have base so is urltestdata.json the right place to go?

Yeah. No base is just a base of about:blank, so just add that.

Cool, thanks!

Why is this commented out?

"??a=b&c=d" is broken at the moment, can make another PR to skip specific test cases later I think (though the whole condition doesn't have to be commented out. I'll change that)

Nice work!

I'd rather have a more featureful test(), perhaps one like

(fn, desc) => {
  try {
    fn();
  } catch (err) {
    if (err instanceof Error)
      err.message = `In ${desc}:\n  ${err.message}`;
    throw err;
  }
}

Done! Thanks

Rebased and updated URL search parameters constructor tests to w3c/web-platform-tests/405394a now that #11060 has been merged.

EDIT: oops, I mean 405394a for search params tests

Adapted from https://github.com/joyeecheung/node/blob/d65904bc6a340d67c10efdcb9fa083b3bf20d234/test/parallel/test-whatwg-url-parsing.js; see discussion in nodejs/node#11079.

There is also the issue of attribution. According to the W3C 3-clause BSD License (the alternative does not allow modification of source), we "must retain the original copyright notice". I don't think we are doing that currently.

You can condense these three lines into

const { URL, URLSearchParams } = require('url');

The signature of assert_unreached() in WPT is (description), but assert.fail() in Node.js is (actual, expected, message, operator). Maybe

(description) => assert.fail('reached', 'unreached', `Reached unreachable code: ${description}`, 'is')

?

According to the docs

If message is falsy, the error message is set as the values of actual and expected separated by the provided operator. Otherwise, the error message is the value of message.

Probably just

    assert.fail(undefined, undefined, `Reached unreachable code: ${description}`);

@joyeecheung, yeah, that'd work as well. IMO the 2 undefineds look a bit too long to be aesthetically pleasing, but up to you.

@TimothyGu Thanks for catching the copyright notice, looks like url-tests.json is doing that with

License: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html

So I'll put this in other files too. @jasnell is it the right way to do it?

Refs: web-platform-tests/wpt#4413 Refs: whatwg/url#104

Yes, our JSON parsing does not recognize regular javascript style comments so including it in a property like this is good.

New CI: https://ci.nodejs.org/job/node-test-pull-request/6165/

@TimothyGu Does this LGTY?

Aborted CI, only arm-fanned was pending because of nodejs/build#611

New CI: https://ci.nodejs.org/job/node-test-pull-request/6167/

Landed in 10b687b

* attributon of WPT in url-setter-tests * add WPT test utilities * synchronize WPT URLSearchParams tests * synchronize WPT url tests * split whatwg-url-inspect test * port historical url tests from WPT * protocol setter and special URLs Refs: web-platform-tests/wpt#4413 Refs: whatwg/url#104 PR-URL: #11079 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

* attributon of WPT in url-setter-tests * add WPT test utilities * synchronize WPT URLSearchParams tests * synchronize WPT url tests * split whatwg-url-inspect test * port historical url tests from WPT * protocol setter and special URLs Refs: web-platform-tests/wpt#4413 Refs: whatwg/url#104 Backport-of: nodejs#11079

* attributon of WPT in url-setter-tests * add WPT test utilities * synchronize WPT URLSearchParams tests * synchronize WPT url tests * split whatwg-url-inspect test * port historical url tests from WPT * protocol setter and special URLs Refs: web-platform-tests/wpt#4413 Refs: whatwg/url#104 Backport-of: #11079

* attributon of WPT in url-setter-tests * add WPT test utilities * synchronize WPT URLSearchParams tests * synchronize WPT url tests * split whatwg-url-inspect test * port historical url tests from WPT * protocol setter and special URLs Refs: web-platform-tests/wpt#4413 Refs: whatwg/url#104 PR-URL: nodejs#11079 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 31, 2017

joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 31, 2017

joyeecheung force-pushed the wpt-url branch 5 times, most recently from 54cea70 to 701e92b Compare January 31, 2017 17:23

joyeecheung changed the title [WIP] url, test: synchronize web-platform-test url tests url, test: synchronize web-platform-test url tests Jan 31, 2017

joyeecheung force-pushed the wpt-url branch from 701e92b to 1bd46a6 Compare January 31, 2017 18:11

jasnell approved these changes Jan 31, 2017

View reviewed changes

domenic reviewed Jan 31, 2017

View reviewed changes

joyeecheung mentioned this pull request Feb 1, 2017

url, querystring: web platform tests broken by WHATWG URL implementation #11093

Closed

TimothyGu reviewed Feb 1, 2017

View reviewed changes

joyeecheung force-pushed the wpt-url branch from fb4aba7 to 8ef3b32 Compare February 1, 2017 13:18

joyeecheung force-pushed the wpt-url branch from 8ef3b32 to dee335a Compare February 1, 2017 13:21

joyeecheung mentioned this pull request Feb 1, 2017

Upstream Node.js URL test data web-platform-tests/wpt#4693

Merged

TimothyGu reviewed Feb 1, 2017

View reviewed changes

TimothyGu suggested changes Feb 1, 2017

View reviewed changes

joyeecheung added 7 commits February 2, 2017 14:15

test: attributon of WPT in url-setter-tests

e16be38

test: add WPT test utilities

0e98bbc

url, test: synchronize WPT URLSearchParams tests

971f8b9

url, test: synchronize WPT url tests

57a2937

test: split whatwg-url-inspect test

f4d972d

test: port historical url tests from WPT

38511c4

test: protocol setter and special URLs

ff0fccc

Refs: web-platform-tests/wpt#4413 Refs: whatwg/url#104

joyeecheung force-pushed the wpt-url branch from dee335a to ff0fccc Compare February 2, 2017 06:47

TimothyGu approved these changes Feb 2, 2017

View reviewed changes

jasnell closed this Feb 2, 2017

TimothyGu mentioned this pull request Feb 2, 2017

test: simplify WPT.assert_throws() #11126

Closed

2 tasks

italoacasas mentioned this pull request Feb 5, 2017

Proposal - 7.6.0 (Current) #11185

Merged

TimothyGu mentioned this pull request Feb 18, 2017

(v7.x backport) URL-related changes #11457

Closed

4 tasks

joyeecheung deleted the wpt-url branch February 19, 2017 17:42

italoacasas added the dont-land-on-v7.x label Feb 20, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jun 22, 2017

url, test: synchronize web-platform-test url tests by joyeecheung · Pull Request #11079 · nodejs/node · GitHub

Uh oh!

Uh oh!

url, test: synchronize web-platform-test url tests - #11079

url, test: synchronize web-platform-test url tests#11079joyeecheung wants to merge 7 commits intonodejs:masternodejs/node:masterfrom joyeecheung:wpt-urljoyeecheung/node:wpt-urlCopy head branch name to clipboard

Conversation

joyeecheung commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

joyeecheung commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Jan 31, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Feb 2, 2017

Uh oh!

jasnell commented Feb 2, 2017

Uh oh!

joyeecheung commented Feb 2, 2017

Uh oh!

joaocgreis commented Feb 2, 2017

Uh oh!

jasnell commented Feb 2, 2017

Uh oh!

jasnell commented Feb 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

url, test: synchronize web-platform-test url tests#11079
joyeecheung wants to merge 7 commits into
nodejs:masterfrom
joyeecheung:wpt-url

joyeecheung commented Jan 31, 2017 •
edited
Loading

joyeecheung commented Jan 31, 2017 •
edited
Loading

joyeecheung commented Feb 1, 2017 •
edited
Loading