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, test: synchronize web-platform-test url tests #11079

Closed
wants to merge 7 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 31, 2017

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 31, 2017
@joyeecheung joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 31, 2017
@joyeecheung joyeecheung force-pushed the wpt-url branch 5 times, most recently from 54cea70 to 701e92b Compare January 31, 2017 17:23
@joyeecheung 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
Copy link
Member Author

joyeecheung commented Jan 31, 2017

Completed. Request review from @nodejs/url

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

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

@joyeecheung
Copy link
Member Author

Force pushed because of filename mistakes.

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a couple nits

https://github.com/w3c/web-platform-tests/blob/8791bed/url/url-origin.html
*/
function runURLOriginTests() {
// var setup = async_test("Loading data…")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps remove the commented out code sections?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test(function() {
var url = bURL(expected.input, expected.base)
assert_equals(url.origin, expected.origin, "origin")
}, "Origin parsing: <" + expected.input + "> against <" + expected.base + ">")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: template string may be better here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's fine then

@@ -0,0 +1,87 @@
module.exports = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

assert_equals(url.port, expected.port, "port")
assert_equals(url.pathname, expected.pathname, "pathname")
assert_equals(url.search, expected.search, "search")
// if ("searchParams" in expected) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"??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)

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

test/common.js Outdated

// https://github.com/w3c/testharness.js/blob/master/testharness.js
exports.WPT = {
test: (fn) => fn(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Thanks

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 1, 2017

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

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -1,52 +1,58 @@
'use strict';

require('../common');
const common = require('../common');
const assert = require('assert');
const url = require('url');
const URL = url.URL;
const URLSearchParams = url.URLSearchParams;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can condense these three lines into

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

test/common.js Outdated
}, desc);
},
assert_array_equals: assert.deepStrictEqual,
assert_unreached: assert.fail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}`);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@joyeecheung
Copy link
Member Author

@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?

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

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

@joyeecheung
Copy link
Member Author

@joaocgreis
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

Landed in 10b687b

@jasnell jasnell closed this Feb 2, 2017
italoacasas pushed a commit that referenced this pull request Feb 3, 2017
* 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 <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
TimothyGu pushed a commit to TimothyGu/node that referenced this pull request Feb 18, 2017
* 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
@joyeecheung joyeecheung deleted the wpt-url branch February 19, 2017 17:42
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017
* 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
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
* 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
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
* 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 <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants