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: add failing url parse tests as known_issue #5885

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 24, 2016

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 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
console.log(`\u2717 - ${test.href} - ${err.message}`);
failed++;
}
console.log(` Input: ${test.input}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

cjihrig commented Mar 24, 2016

LGTM

@jasnell
Copy link
Member Author

jasnell commented Mar 24, 2016

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

@sam-github
Copy link
Contributor

@jasnell is there a list of defects?

@jasnell
Copy link
Member Author

jasnell commented Apr 1, 2016

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

@sam-github
Copy link
Contributor

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

@jasnell
Copy link
Member Author

jasnell commented Apr 1, 2016

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 jasnell added the wip Issues and PRs that are still a work in progress. label Apr 22, 2016
@jasnell
Copy link
Member Author

jasnell commented May 1, 2016

Updated. /cc @cjihrig @mscdex

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

mscdex commented May 1, 2016

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented May 2, 2016

LGTM

jasnell added a commit that referenced this pull request May 2, 2016
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
Copy link
Member Author

jasnell commented May 2, 2016

Landed in fa542eb. Thanks @mscdex and @cjihrig !

@jasnell jasnell closed this May 2, 2016
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
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>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
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
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.

5 participants