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

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

Merged
merged 5 commits into from Jun 30, 2022
Merged

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

merged 5 commits into from Jun 30, 2022

Conversation

F3n67u
Copy link
Member

@F3n67u F3n67u commented May 23, 2022 •
edited

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
Member 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
Member 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

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

@F3n67u
Copy link
Member Author

F3n67u commented Jun 30, 2022 •
edited

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

@RaisinTen
Copy link
Contributor

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
Member Author

F3n67u commented Jun 30, 2022

Landed in 2dccda2

@aduh95 many thanks.

@F3n67u
Copy link
Member 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