Skip to content

WHATWG URL origin cleanup + tests #11691

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 9, 2017
Merged

WHATWG URL origin cleanup + tests #11691

merged 2 commits into from
Mar 9, 2017

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Mar 5, 2017 •
edited
Loading

'file' should have been 'file:' but since the WHATWG URL spec suggests using an opaque origin (which is what was already being used for file URLs), we'll just keep using that, making this merely a cleanup.

Additionally, some more origin tests are added.

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

/cc @nodejs/url

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)
  • url-whatwg

@mscdex mscdex added test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 5, 2017
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 5, 2017
@mscdex mscdex changed the title Url whatwg origin tests WHATWG URL origin cleanup + tests Mar 5, 2017
joyeecheung
joyeecheung approved these changes Mar 5, 2017
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Cleanup LTGM, the additional tests seem to be duplicates

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

// Test special origins
[
{ expected: 'https://whatwg.org',
Copy link
Member

Choose a reason for hiding this comment

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

I think most tests here are already in url-tests.js, therefore tested by test-whatwg-url-origin?

Copy link
Contributor Author

@mscdex mscdex Mar 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Specifically the file: origin test is not covered by the WPT tests, which would have uncovered the 'bug.' I just added other origin tests for completeness.

mscdex added 2 commits March 9, 2017 08:56
@mscdex
url: remove invalid file protocol check
7afc70d 
'file' should have been 'file:' but since the WHATWG URL spec
suggests using an opaque origin (which is what was already being
used for file URLs), we'll just keep using that, making this merely
a cleanup.

PR-URL: nodejs#11691
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mscdex
test: add more WHATWG URL origin tests
e4fbd8e 
PR-URL: nodejs#11691
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mscdex mscdex force-pushed the url-whatwg-origin-tests branch from 7a0dd17 to e4fbd8e Compare March 9, 2017 13:59
@mscdex mscdex merged commit e4fbd8e into nodejs:master Mar 9, 2017
@mscdex mscdex deleted the url-whatwg-origin-tests branch March 9, 2017 14:01
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 13, 2017
@mscdex
url: remove invalid file protocol check
c5874d1 
'file' should have been 'file:' but since the WHATWG URL spec
suggests using an opaque origin (which is what was already being
used for file URLs), we'll just keep using that, making this merely
a cleanup.

PR-URL: nodejs#11691
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 13, 2017
@mscdex
test: add more WHATWG URL origin tests
3d55cf0 
PR-URL: nodejs#11691
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Mar 13, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 14, 2017
2017-03-14, Version 7.7.3 (Current)
9c68a69 
Notable changes:

* module: The [module loading global fallback]
(https://nodejs.org/dist/latest-v6.x/docs/api/modules.html#modules_loading_from_the_global_folders)
to the Node executable's directory now works correctly on Windows.
(Richard Lau) [nodejs#9283](nodejs#9283)

* net: `Socket.prototype.connect` now once again functions without
a callback. (Juwan Yoo) [nodejs#11762](nodejs#11762)

* url: `URL.prototype.origin` now properly specified an opaque return of
`'null'` for `file://` URLs. (Brian White)
[nodejs#11691](nodejs#11691)

PR-URL: nodejs#11831
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 14, 2017
2017-03-14, Version 7.7.3 (Current)
bc26c62 
Notable changes:

* module: The [module loading global fallback]
(https://nodejs.org/dist/latest-v6.x/docs/api/modules.html#modules_loading_from_the_global_folders)
to the Node executable's directory now works correctly on Windows.
(Richard Lau) [nodejs#9283](nodejs#9283)

* net: `Socket.prototype.connect` now once again functions without
a callback. (Juwan Yoo) [nodejs#11762](nodejs#11762)

* url: `URL.prototype.origin` now properly specified an opaque return of
`'null'` for `file://` URLs. (Brian White)
[nodejs#11691](nodejs#11691)

PR-URL: nodejs#11831
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 16, 2017
@imyller
2017-03-14, Version 7.7.3 (Current)
78018dc 
    Notable changes:

    * module: The [module loading global fallback]
    (https://nodejs.org/dist/latest-v6.x/docs/api/modules.html#modules_loading_from_the_global_folders)
    to the Node executable's directory now works correctly on Windows.
    (Richard Lau) [#9283](nodejs/node#9283)

    * net: `Socket.prototype.connect` now once again functions without
    a callback. (Juwan Yoo) [#11762](nodejs/node#11762)

    * url: `URL.prototype.origin` now properly specified an opaque return of
    `'null'` for `file://` URLs. (Brian White)
    [#11691](nodejs/node#11691)

    PR-URL: nodejs/node#11831

Signed-off-by: Ilkka Myller <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@mscdex @jungx098
url: remove invalid file protocol check
d86f859 
'file' should have been 'file:' but since the WHATWG URL spec
suggests using an opaque origin (which is what was already being
used for file URLs), we'll just keep using that, making this merely
a cleanup.

PR-URL: nodejs#11691
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@mscdex @jungx098
test: add more WHATWG URL origin tests
f77dd3d 
PR-URL: nodejs#11691
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@jungx098
2017-03-14, Version 7.7.3 (Current)
67320f2 
Notable changes:

* module: The [module loading global fallback]
(https://nodejs.org/dist/latest-v6.x/docs/api/modules.html#modules_loading_from_the_global_folders)
to the Node executable's directory now works correctly on Windows.
(Richard Lau) [nodejs#9283](nodejs#9283)

* net: `Socket.prototype.connect` now once again functions without
a callback. (Juwan Yoo) [nodejs#11762](nodejs#11762)

* url: `URL.prototype.origin` now properly specified an opaque return of
`'null'` for `file://` URLs. (Brian White)
[nodejs#11691](nodejs#11691)

PR-URL: nodejs#11831
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+2 more reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@watilde watilde watilde approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mscdex @jasnell @lpinca @TimothyGu @watilde @cjihrig @joyeecheung @nodejs-github-bot