GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test, url: update WHATWG URL parser to align with latest spec by F3n67u · Pull Request #43190 · nodejs/node

Conversation

@F3n67u

@F3n67u F3n67u commented May 23, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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
    • Reject non-IPv4 hostnames that end in numbers(Reject non-IPv4 hostnames that end in numbers. whatwg/url#619)
    • Split forbidden host/domain code points, and add all C0 controls and add U+007F to the latter(Split forbidden host/domain code points, and add all C0 controls and add U+007F to the latter whatwg/url#685)

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
leizongmin and bricss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels May 23, 2022
@F3n67u
F3n67u force-pushed the update-wpt-url branch from 81c3eca to 644b1c5 Compare May 23, 2022 15:13
@F3n67u
F3n67u force-pushed the update-wpt-url branch from 83b70c6 to 9d8ddc1 Compare June 3, 2022 17:02
@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. Only starts once the PR has an approving review. label Jun 7, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 7, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@Trott

Trott commented Jun 7, 2022

Copy link
Copy Markdown
Member

@nodejs/url @nodejs/testing
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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. Only starts once the PR has an approving review. label Jun 11, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 11, 2022
F3n67u added 4 commits June 29, 2022 08:19
@F3n67u
test: remove WPT tests that are now .any.js in the upstream
0a13097
@F3n67u
test: add isShadowRealm property to global.GLOBAL to avoid wpt te…
c655514 
…st crash
@F3n67u
url: reject non-IPv4 hostnames that end in numbers
5ad2a9b
@F3n67u
url: split forbidden host/domain code points, and add all C0 controls…
d97fd8e 
… and add U+007F to the latter
@F3n67u
F3n67u force-pushed the update-wpt-url branch from 3c19001 to d97fd8e Compare June 29, 2022 00:19
@F3n67u F3n67u added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 29, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 29, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@F3n67u F3n67u added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jun 29, 2022
@F3n67u

F3n67u commented Jun 29, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@F3n67u

F3n67u commented Jun 29, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

aduh95 commented Jun 30, 2022

Copy link
Copy Markdown
Contributor

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

Adding author ready PRs with CI started, the required approvals, and no outstanding review comments. is definitely the right thing to do here, FYI I try to make a tour of all author ready PRs with CI started, the required approvals, and no outstanding review comments. 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.
F3n67u and RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jun 30, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@F3n67u

F3n67u commented Jun 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

RaisinTen commented Jun 30, 2022

Copy link
Copy Markdown
Member

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) ;)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit 2dccda2 into nodejs:main Jun 30, 2022
@aduh95

aduh95 commented Jun 30, 2022

Copy link
Copy Markdown
Contributor

Landed in 2dccda2
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@F3n67u
F3n67u deleted the update-wpt-url branch June 30, 2022 22:58
@F3n67u

F3n67u commented Jun 30, 2022

Copy link
Copy Markdown
Contributor Author

Landed in 2dccda2

@aduh95 many thanks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@F3n67u

F3n67u commented Jun 30, 2022

Copy link
Copy Markdown
Contributor Author

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.
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Jul 4, 2022
CI Reliability 2022-07-04 nodejs/reliability#317
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 <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@targos targos mentioned this pull request Jul 12, 2022
v18.6.0 release proposal #43789
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 <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
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 <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
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 <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@richardlau richardlau mentioned this pull request Jul 21, 2023
yarn 1.x.x invalid url with node 18.17.0 VS 18.16.1 #48855
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

URL is not compliant to specification URL parser does not validate ipv4 with more than 4 parts [URL] Split forbidden host/domain code points, and add all C0 controls and add U+007F to the latter

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@F3n67u @nodejs-github-bot @Trott @aduh95 @RaisinTen @jasnell @watilde @RafaelGSS
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.