Skip to content

test, url: update WHATWG URL parser to align with latest spec #43190

Merged
aduh95 merged 5 commits intonodejs:mainfrom
F3n67u:update-wpt-url
Jun 30, 2022
Merged

test, url: update WHATWG URL parser to align with latest spec #43190
aduh95 merged 5 commits intonodejs:mainfrom
F3n67u:update-wpt-url

Conversation

@F3n67u
Copy link
Contributor

@F3n67u F3n67u commented May 23, 2022 •
edited
Loading

The PR:

  1. update WPT test for url and resource to latest commit: automated by git node wpt
  2. remove test-whatwg-url-constructor.js, test-whatwg-url-origin.js, and test-whatwg-url-setters.js because they are now .any.js in the WPT upstream and covered by wpt test
  3. add isShadowRealm to global.GLOBAL to avoid wpt test crash: without this property test will fail with global.GLOBAL.isShadowRealm is not a function see https://github.com/web-platform-tests/wpt/blob/master/resources/testharness.js#L544
  4. update WHATWG URL parser to align with latest spec

Fix #42920
Fix #41717
Fix #42914: this issue is marked closed by #42915, but #42915 is reverted by #42940, so this issue is still not fixed

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 23, 2022
@F3n67u F3n67u changed the title test: update WPT tests for url url: update WHATWG URL parser to latest spec Jun 3, 2022
@F3n67u F3n67u force-pushed the update-wpt-url branch 2 times, most recently from 6e399ca to f59e9f5 Compare June 3, 2022 17:20
@F3n67u F3n67u marked this pull request as ready for review June 4, 2022 02:49
@F3n67u F3n67u force-pushed the update-wpt-url branch 5 times, most recently from 382f5ce to 6af706a Compare June 4, 2022 04:59
@F3n67u F3n67u changed the title url: update WHATWG URL parser to latest spec test, url: update WHATWG URL parser to latest spec Jun 4, 2022
@F3n67u F3n67u force-pushed the update-wpt-url branch 3 times, most recently from a0fbe2b to f436d5d Compare June 4, 2022 11:01
@F3n67u F3n67u changed the title test, url: update WHATWG URL parser to latest spec test, url: update WHATWG URL parser to align with latest spec Jun 4, 2022
@F3n67u F3n67u force-pushed the update-wpt-url branch 5 times, most recently from 5fd8957 to 3414a08 Compare June 6, 2022 04:32
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 7, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 7, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@Trott
Copy link
Member

Trott commented Jun 7, 2022

@nodejs/url @nodejs/testing

@tniessen tniessen requested a review from jasnell June 7, 2022 21:04
@F3n67u F3n67u added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2022
@F3n67u F3n67u added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 29, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 29, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@F3n67u F3n67u added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 29, 2022
@F3n67u
Copy link
Contributor Author

F3n67u commented Jun 29, 2022

This pr got two approvals so I think it is author ready and add the author-ready label.

@F3n67u
Copy link
Contributor Author

F3n67u commented Jun 29, 2022

Could you help to rerun just the failed ci? I am tired to rerun Jenkins ci in fresh.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@aduh95
Copy link
Contributor

aduh95 commented Jun 30, 2022

This pr got two approvals so I think it is author ready and add the author-ready label.

Adding author ready PRs that have at least one approval, no pending requests for changes, and a CI started. is definitely the right thing to do here, FYI I try to make a tour of all author ready PRs that have at least one approval, no pending requests for changes, and a CI started. PRs every once in a while to re-run CI / land them / comment if there are conflicts / genuine CI failures – and I encourage any collaborator to do the same. I wish I could walk through all the PRs that have been open for a while, that's a lot of work, it would be extra nice to have a triagger day where everyone gets assigned one long-open PR/issue and make sure it's addressed.

I am tired to rerun Jenkins ci in fresh.

I understand the frustration, spawning fresh CI is really not a good solution, pinging as you did is a much better solution indeed.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 30, 2022

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

@F3n67u
Copy link
Contributor Author

F3n67u commented Jun 30, 2022 •
edited
Loading

wow, CI is green now. could you please land this pr?

@RaisinTen
Copy link
Member

RaisinTen commented Jun 30, 2022

wow, CI is green now. could you please land this pr?

@F3n67u I think you'll be able to land this one yourself by applying the commit-queue label + (commit-queue-rebase if you want to land the PR as several commits / commit-queue-squash if you want to squash all the commits into the first commit while landing) ;)

@aduh95 aduh95 merged commit 2dccda2 into nodejs:main Jun 30, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jun 30, 2022

Landed in 2dccda2

@F3n67u F3n67u deleted the update-wpt-url branch June 30, 2022 22:58
@F3n67u
Copy link
Contributor Author

F3n67u commented Jun 30, 2022

Landed in 2dccda2

@aduh95 many thanks.

@F3n67u
Copy link
Contributor Author

F3n67u commented Jun 30, 2022

wow, CI is green now. could you please land this pr?

@F3n67u I think you'll be able to land this one yourself by applying the commit-queue label + (commit-queue-rebase if you want to land the PR as several commits / commit-queue-squash if you want to squash all the commits into the first commit while landing) ;)

Thanks. I will try next time.

@github-actions github-actions bot mentioned this pull request Jul 4, 2022
Open
23 tasks
targos pushed a commit that referenced this pull request Jul 12, 2022
@F3n67u @targos
url: update WHATWG URL parser to align with latest spec
a1f1d3a 
PR-URL: #43190
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@F3n67u @targos
url: update WHATWG URL parser to align with latest spec
da09d8a 
PR-URL: #43190
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@F3n67u @targos
url: update WHATWG URL parser to align with latest spec
8cda415 
PR-URL: #43190
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@F3n67u @guangwong
url: update WHATWG URL parser to align with latest spec
f67adfc 
PR-URL: nodejs/node#43190
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@richardlau richardlau mentioned this pull request Jul 21, 2023
Closed
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

@watilde watilde watilde approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

8 participants

@F3n67u @nodejs-github-bot @Trott @aduh95 @RaisinTen @jasnell @watilde @RafaelGSS