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.

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

url: fix url spec compliance issues #46547

Closed
wants to merge 3 commits into from
Closed

url: fix url spec compliance issues #46547

wants to merge 3 commits into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Feb 7, 2023 •
edited
Loading

This pull request updates web-platform tests for URL, fixes issues that comes with it.

cc @nodejs/url

@anonrig anonrig added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Feb 7, 2023
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Feb 7, 2023
@anonrig anonrig changed the title Bump wpt url spec tests test: update web-platform tests for url Feb 7, 2023
@anonrig anonrig added the blocked PRs that are blocked by other issues or PRs. label Feb 7, 2023
@panva panva changed the title test: update web-platform tests for url url: fix url spec compliance issues Feb 7, 2023
@anonrig anonrig force-pushed the bump-wpt-url-spec-tests branch from 0d8b53a to b612773 Compare February 7, 2023 20:51
TimothyGu
TimothyGu reviewed Feb 9, 2023
View reviewed changes
lib/internal/url.js Outdated Show resolved Hide resolved
src/node_url.cc Outdated Show resolved Hide resolved
lib/internal/url.js Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the bump-wpt-url-spec-tests branch from b612773 to 632af80 Compare February 14, 2023 19:10
@anonrig
Copy link
Member Author

anonrig commented Feb 14, 2023

I added a small optimization just as fun. Please review again, @jasnell @TimothyGu

@anonrig anonrig requested review from jasnell and TimothyGu February 14, 2023 20:34
@anonrig anonrig force-pushed the bump-wpt-url-spec-tests branch from 2694506 to 2bf8e35 Compare February 14, 2023 20:57
TimothyGu
TimothyGu reviewed Feb 16, 2023
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

The tests seem to be failing

src/node_url.cc Outdated Show resolved Hide resolved
@anonrig
Copy link
Member Author

anonrig commented Feb 16, 2023

It is failing because Ada's latest version isn't merged in to main yet.

@anonrig anonrig force-pushed the bump-wpt-url-spec-tests branch 3 times, most recently from 15f8033 to 150c720 Compare February 17, 2023 16:04
@anonrig anonrig removed the blocked PRs that are blocked by other issues or PRs. label Feb 17, 2023
panva
panva reviewed Feb 17, 2023
View reviewed changes
test/wpt/test-url.js Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the bump-wpt-url-spec-tests branch from 150c720 to 8441676 Compare February 17, 2023 16:13
@anonrig anonrig requested review from panva and TimothyGu February 17, 2023 16:14
@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Feb 19, 2023
Open
Open
@joyeecheung joyeecheung mentioned this pull request Feb 20, 2023
Closed
targos pushed a commit that referenced this pull request Mar 13, 2023
@anonrig @targos
test: update web-platform tests for url
95bbd0f 
PR-URL: #46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Mar 13, 2023
@anonrig @lemire @targos
url: fix url spec compliance issues
9d55a5e 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: #46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Mar 13, 2023
@anonrig @targos
url: improve performance by removing host
a524055 
PR-URL: #46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Mar 14, 2023
Merged
@danielleadams
Copy link
Contributor

Blocked by #46410

@danielleadams danielleadams added the backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. label Apr 3, 2023
anonrig added a commit to anonrig/node that referenced this pull request Apr 5, 2023
@anonrig
test: update web-platform tests for url
11fceac 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 5, 2023
@anonrig @lemire
url: fix url spec compliance issues
1645bf6 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 5, 2023
@anonrig
url: improve performance by removing host
9c1fbcd 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@anonrig anonrig mentioned this pull request Apr 5, 2023
Closed
@anonrig anonrig added backport-open-v18.x Indicate that the PR has an open backport. and removed backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. labels Apr 5, 2023
anonrig added a commit to anonrig/node that referenced this pull request Apr 5, 2023
@anonrig
test: update web-platform tests for url
6a39d42 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 5, 2023
@anonrig @lemire
url: fix url spec compliance issues
cb14fc6 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 5, 2023
@anonrig
url: improve performance by removing host
c94ca85 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 11, 2023
@anonrig
test: update web-platform tests for url
4ce29c4 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 11, 2023
@anonrig @lemire
url: fix url spec compliance issues
1d597a4 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 11, 2023
@anonrig
url: improve performance by removing host
1be4f6c 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 11, 2023
@anonrig
test: update web-platform tests for url
78b3490 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 11, 2023
@anonrig @lemire
url: fix url spec compliance issues
205678a 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anonrig added a commit to anonrig/node that referenced this pull request Apr 11, 2023
@anonrig
url: improve performance by removing host
b2289da 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @danielleadams
test: update web-platform tests for url
972bdee 
PR-URL: #46547
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @lemire @danielleadams
url: fix url spec compliance issues
0c67a7a 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: #46547
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @danielleadams
url: improve performance by removing host
01b6525 
PR-URL: #46547
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@danielleadams danielleadams added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels Apr 12, 2023
@danielleadams danielleadams mentioned this pull request Apr 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@Trott Trott Trott approved these changes

@panva panva panva approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. 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
@anonrig @nodejs-github-bot @richardlau @danielleadams @panva @jasnell @Trott @TimothyGu