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

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

Closed
wants to merge 1 commit into from

Conversation

watilde
Copy link
Member

@watilde watilde commented Feb 16, 2017

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:

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 16, 2017
@mscdex mscdex added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 16, 2017
@watilde
Copy link
Member Author

watilde commented Feb 16, 2017

I did minor tweaks to make the diff less.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Also these new data can be upstreamed to WPT

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

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 21, 2017
watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 21, 2017
@addaleax
Copy link
Member

@watilde looks like this needs a rebase?

@watilde
Copy link
Member Author

watilde commented Feb 21, 2017

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

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 21, 2017
watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 21, 2017
@joyeecheung
Copy link
Member

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 :).

watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 22, 2017
watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 22, 2017
watilde added a commit to watilde/web-platform-tests that referenced this pull request Feb 22, 2017
jgraham pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 23, 2017
@watilde
Copy link
Member Author

watilde commented Feb 23, 2017

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

@@ -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'));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@watilde watilde Feb 27, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@watilde watilde Mar 3, 2017

Choose a reason for hiding this comment

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

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
Copy link
Member

Still LGTM

@watilde
Copy link
Member Author

watilde commented Mar 5, 2017

@watilde
Copy link
Member Author

watilde commented Mar 5, 2017

CI failures are unrelated.

@watilde
Copy link
Member Author

watilde commented Mar 5, 2017

Landed in 39f7e72

@watilde watilde closed this Mar 5, 2017
@watilde watilde deleted the test-originFor branch March 5, 2017 15:45
watilde added a commit that referenced this pull request Mar 5, 2017
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 <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Mar 5, 2017
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 <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
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