test, url: check the origin of the blob URLs #11426

watilde · 2017-02-16T19:21:20Z

In the getter of the origin in URL, the URL that has blob protocol will be parsed in a function called "originFor". Add test cases to the url-tests-additional fixture to test that.

This test increases the coverage of internal/url.js:

https://coverage.nodejs.org/coverage-3429991d8b43474c/root/internal/url.js.html

The following lines will be covered:

node/lib/internal/url.js

Lines 1064 to 1073 in a196895

    
           case 'blob:': 
        
             if (url[context].path && url[context].path.length > 0) { 
        
               try { 
        
                 return (new URL(url[context].path[0])).origin; 
        
               } catch (err) { 
        
                 // fall through... do nothing 
        
               } 
        
             } 
        
             origin = new OpaqueOrigin(); 
        
             break;

Checklist

make -j4 test passes
tests are included
commit message follows commit guidelines

Affected core subsystem(s)

test, url

watilde · 2017-02-16T22:55:11Z

I did minor tweaks to make the diff less.

joyeecheung · 2017-02-18T19:21:17Z

test/parallel/test-whatwg-url-origin.js

@@ -42,7 +45,7 @@ function runURLTests(urltests) {
    var expected = urltests[i]
    if (typeof expected === "string" || !("origin" in expected)) continue
    test(function() {
-      var url = bURL(expected.input, expected.base)
+      var url = bURL(expected.input || expected.url, expected.base)


This part is not in WPT, and the test data format is different actually. I think we can simply move it to the bottom and leave the WPT part alone for the ease of syncing/upstreaming.

// Tests below are not from WPT. const tests = require(path.join(common.fixturesDir, 'url-tests-additional.js')); for (const test of tests) { if (typeof test === 'object' && 'origin' in test) { const url = new URL(test.url); assert.strictEqual(url.origin, test.origin); } }

Also these new data can be upstreamed to WPT, no need to block this PR though, we can remove the duplicates in url-tests-additional.js if it's upstreamed and synced into url-tests.json later.

That's right, like the some of the other tests. I will update it, thanks!

Yes, I agreed on it. I will make a patch to WPT then.

Also these new data can be upstreamed to WPT

joyeecheung · 2017-02-19T08:31:59Z

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

Related link: + nodejs/node#11426

addaleax · 2017-02-21T17:09:30Z

@watilde looks like this needs a rebase?

watilde · 2017-02-21T17:12:30Z

@addaleax Oh yeah, I was just doing that xD

Also, I made a patch to WPT at web-platform-tests/wpt#4941 now. Once we get a merge the tests, I migrate them into node.

Related link: + nodejs/node#11426

joyeecheung · 2017-02-22T05:07:57Z

Just to be clear, I don't think we have to wait for the upstream PR to be merged to land our own, but this one touches a unsettled section in the spec so I think better wait until that one is merged :).

Related link: + nodejs/node#11426

watilde · 2017-02-23T18:18:36Z

The upstream PR was merged, and I've picked them into this PR

TimothyGu · 2017-02-25T22:22:15Z

test/parallel/test-whatwg-url-origin.js

@@ -50,3 +51,12 @@ function runURLTests(urltests) {

 runURLOriginTests()
 /* eslint-enable */
+
+// Tests below are not from WPT.
+const tests = require(path.join(common.fixturesDir, 'url-tests-additional.js'));


This doesn't seem to be applicable anymore, now that the tests are moved to url-tests.js

I think it's good to keep it since potentially a case can be added into url-tests-addition sometimes and some test codes actually are still having it.
e.g.

https://github.com/nodejs/node/blob/master/test/parallel//test-whatwg-url-parsing.js#L27-L43

https://github.com/nodejs/node/blob/master/test/parallel/test-whatwg-url-inspect.js#L17-L18

Should we add a test into the other URL tests as well to use url-tests-addition? We also can separate the patch from this PR :)

Maybe just wait until there are new tests added into url-tests-addition? I am fine either way though.

Ok, I will get rid of this part from this PR since the scope can be different

In the getter of the origin in URL, the URL that has blob protocol will be parsed in a function called "originFor". Add test cases into the url-tests-additional fixture to test that. Refs: web-platform-tests/wpt#4941

joyeecheung · 2017-03-05T14:38:15Z

Still LGTM

watilde · 2017-03-05T14:48:55Z

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

watilde · 2017-03-05T15:39:00Z

CI failures are unrelated.

watilde · 2017-03-05T15:44:55Z

Landed in 39f7e72

In the getter of the origin in URL, the URL that has blob protocol will be parsed in a function called "originFor". Add test cases into the url-tests-additional fixture to test that. Refs: web-platform-tests/wpt#4941 PR-URL: #11426 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

jasnell approved these changes Feb 16, 2017

View reviewed changes

mscdex added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 16, 2017

watilde force-pushed the test-originFor branch from 9ef8e3d to 546005b Compare February 16, 2017 22:54

joyeecheung reviewed Feb 18, 2017

View reviewed changes

watilde force-pushed the test-originFor branch 3 times, most recently from fbf17a3 to 5c43539 Compare February 18, 2017 19:56

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 21, 2017

Add blob URLs into urltestdata.json

ed71fa2

Related link: + nodejs/node#11426

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 21, 2017

Add blob URLs into urltestdata.json

fea90e4

Related link: + nodejs/node#11426

watilde mentioned this pull request Feb 21, 2017

Add blob URLs into urltestdata.json web-platform-tests/wpt#4941

Merged

watilde force-pushed the test-originFor branch from 5c43539 to b310388 Compare February 21, 2017 17:10

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 21, 2017

Add blob URLs into urltestdata.json

11f0c49

Related link: + nodejs/node#11426

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 21, 2017

Add blob URLs into urltestdata.json

9980731

Related link: + nodejs/node#11426

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 22, 2017

Add blob URLs into urltestdata.json

87f615e

Related link: + nodejs/node#11426

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 22, 2017

Add blob URLs into urltestdata.json

e4cb43f

Related link: + nodejs/node#11426

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 22, 2017

Add blob URLs into urltestdata.json

c0cb991

Related link: + nodejs/node#11426

jgraham pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 23, 2017

Add blob URLs into urltestdata.json

e32ff14

Related link: + nodejs/node#11426

watilde force-pushed the test-originFor branch from b310388 to 84aab33 Compare February 23, 2017 18:09

joyeecheung approved these changes Feb 25, 2017

View reviewed changes

TimothyGu reviewed Feb 25, 2017

View reviewed changes

test: check the origin of the blob URLs

03d30cb

In the getter of the origin in URL, the URL that has blob protocol will be parsed in a function called "originFor". Add test cases into the url-tests-additional fixture to test that. Refs: web-platform-tests/wpt#4941

watilde force-pushed the test-originFor branch from 84aab33 to 03d30cb Compare March 3, 2017 10:19

watilde closed this Mar 5, 2017

watilde deleted the test-originFor branch March 5, 2017 15:45

evanlucas mentioned this pull request Mar 8, 2017

v7.7.2 proposal #11745

Merged

MylesBorins added the dont-land-on-v6.x label Apr 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test, url: check the origin of the blob URLs #11426

test, url: check the origin of the blob URLs #11426

watilde commented Feb 16, 2017

watilde commented Feb 16, 2017

joyeecheung Feb 18, 2017

joyeecheung Feb 18, 2017

watilde Feb 18, 2017

watilde Feb 18, 2017

joyeecheung commented Feb 19, 2017

addaleax commented Feb 21, 2017

watilde commented Feb 21, 2017 •

edited

Loading

joyeecheung commented Feb 22, 2017

watilde commented Feb 23, 2017

TimothyGu Feb 25, 2017

watilde Feb 27, 2017 •

edited

Loading

watilde Mar 2, 2017

joyeecheung Mar 2, 2017

watilde Mar 3, 2017 •

edited

Loading

joyeecheung commented Mar 5, 2017

watilde commented Mar 5, 2017

watilde commented Mar 5, 2017

watilde commented Mar 5, 2017

	case 'blob:':
	if (url[context].path && url[context].path.length > 0) {
	try {
	return (new URL(url[context].path[0])).origin;
	} catch (err) {
	// fall through... do nothing
	}
	}
	origin = new OpaqueOrigin();
	break;

test, url: check the origin of the blob URLs #11426

test, url: check the origin of the blob URLs #11426

Conversation

watilde commented Feb 16, 2017

Checklist

Affected core subsystem(s)

watilde commented Feb 16, 2017

joyeecheung Feb 18, 2017

Choose a reason for hiding this comment

joyeecheung Feb 18, 2017

Choose a reason for hiding this comment

watilde Feb 18, 2017

Choose a reason for hiding this comment

watilde Feb 18, 2017

Choose a reason for hiding this comment

joyeecheung commented Feb 19, 2017

addaleax commented Feb 21, 2017

watilde commented Feb 21, 2017 • edited Loading

joyeecheung commented Feb 22, 2017

watilde commented Feb 23, 2017

TimothyGu Feb 25, 2017

Choose a reason for hiding this comment

watilde Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

watilde Mar 2, 2017

Choose a reason for hiding this comment

joyeecheung Mar 2, 2017

Choose a reason for hiding this comment

watilde Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

joyeecheung commented Mar 5, 2017

watilde commented Mar 5, 2017

watilde commented Mar 5, 2017

watilde commented Mar 5, 2017

watilde commented Feb 21, 2017 •

edited

Loading

watilde Feb 27, 2017 •

edited

Loading

watilde Mar 3, 2017 •

edited

Loading