FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add failing url parse tests as known_issue by jasnell · Pull Request #5885 · nodejs/node · GitHub

/ node Public

test: add failing url parse tests as known_issue - #5885

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:url-test-cases
ConversationCommits (1)ChecksFiles changed
Closed

test: add failing url parse tests as known_issue#5885
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:url-test-cases

Conversation

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

url, test

Description of change

url resolve and parse do not currently adhere to the same url spec parsing rules that browsers use, which leads to some issues. This addition to test/known_issues creates a set of tests based on the w3c/whatwg test suite from https://github.com/w3c/web-platform-tests/tree/master/url

jasnell added url Issues and PRs related to the legacy built-in url module. test Issues and PRs related to the tests. known issue test labels Mar 24, 2016
jasnell mentioned this pull request Mar 24, 2016
Closed
cjihrig reviewed Mar 24, 2016
View reviewed changes
Comment thread test/known_issues/test-url-parse-conformance.js Outdated
console.log(`\u2717 - ${test.href} - ${err.message}`);
failed++;
}
console.log(` Input: ${test.input}`);

cjihrig Mar 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does all of this need to be logged for every test? What if it was only printed for failing tests?

jasnell Mar 24, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No, it doesn't. I added that in largely to highlight the differences. I can
switch it to only output on failing tests.
On Mar 24, 2016 5:27 AM, "Colin Ihrig" notifications@github.com wrote:

In test/known_issues/test-url-parse-conformance.js
#5885 (comment):

  • username = parsed.auth ? parsed.auth.split(':', 2)[0] : '';
  • password = parsed.auth ? parsed.auth.split(':', 2)[1] : '';
  • assert.equal(test.username, username);
  • assert.equal(test.password, password);
  • assert.equal(test.host, parsed.host);
  • assert.equal(test.hostname, parsed.hostname);
  • assert.equal(+test.port, +parsed.port);
  • assert.equal(test.pathname, parsed.pathname || '/');
  • assert.equal(test.search, parsed.search || '');
  • assert.equal(test.hash, parsed.hash || '');
  • console.log(\u2713 - ${test.href});
  • } catch (err) {
  • console.log(\u2717 - ${test.href} - ${err.message});
  • failed++;
  • }
  • console.log(Input: ${test.input});

Does all of this need to be logged for every test? What if it was only
printed for failing tests?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/5885/files/e8229bd3262164bd842f00ec09fe3e533666eec6#r57309357

cjihrig commented Mar 24, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

@cjihrig .. updated, PTAL
/cc @nodejs/testing

sam-github commented Apr 1, 2016

Copy link
Copy Markdown
Contributor

@jasnell is there a list of defects?

jasnell commented Apr 1, 2016

Copy link
Copy Markdown
Member Author

#5832 is one. I don't have a compiled list tho.

sam-github commented Apr 1, 2016

Copy link
Copy Markdown
Contributor

I was wondering if you found more things than I reported.

jasnell commented Apr 1, 2016

Copy link
Copy Markdown
Member Author

Well, the test points out that there are a significant number of inconsistencies between our parsing (and relative URL resolution) than what is implemented / expected on the browser side per the WhatWG specs. This test is really meant just to highlight those. I wouldn't necessarily call them defects, per se, but closing the gaps would be good.

jasnell added the wip Issues and PRs that are still a work in progress. label Apr 22, 2016
estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

jasnell commented May 1, 2016

Copy link
Copy Markdown
Member Author

Updated. /cc @cjihrig @mscdex

test: add failing url parse tests as known_issue
3ed8f8b 
url resolve and parse do not currently adhere to the same url
spec parsing rules that browsers use, which leads to some
issues. This addition to test/known_issues creates a set of
tests based on the w3c/whatwg test suite from:

https://github.com/w3c/web-platform-tests/tree/master/url

mscdex commented May 1, 2016

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment

cjihrig commented May 2, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell added a commit that referenced this pull request May 2, 2016
test: add failing url parse tests as known_issue
fa542eb 
url resolve and parse do not currently adhere to the same url
spec parsing rules that browsers use, which leads to some
issues. This addition to test/known_issues creates a set of
tests based on the w3c/whatwg test suite from:

Refs: https://github.com/w3c/web-platform-tests/tree/master/url

PR-URL: #5885
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell commented May 2, 2016

Copy link
Copy Markdown
Member Author

Landed in fa542eb. Thanks @mscdex and @cjihrig !

jasnell closed this May 2, 2016
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
test: add failing url parse tests as known_issue
457d12a 
url resolve and parse do not currently adhere to the same url
spec parsing rules that browsers use, which leads to some
issues. This addition to test/known_issues creates a set of
tests based on the w3c/whatwg test suite from:

Refs: https://github.com/w3c/web-platform-tests/tree/master/url

PR-URL: #5885
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
This was referenced May 4, 2016
Closed
Merged
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
test: add failing url parse tests as known_issue
db2498d 
url resolve and parse do not currently adhere to the same url
spec parsing rules that browsers use, which leads to some
issues. This addition to test/known_issues creates a set of
tests based on the w3c/whatwg test suite from:

Refs: https://github.com/w3c/web-platform-tests/tree/master/url

PR-URL: nodejs#5885
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

MylesBorins commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

Added don't land as I'm assuming we would not be chasing down this problem in lts

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

test Issues and PRs related to the tests. url Issues and PRs related to the legacy built-in url module. wip Issues and PRs that are still a work in progress.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL