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

lib: fix AbortSignal.timeout parameter validation #42856

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

dnalborczyk
Copy link
Contributor

@dnalborczyk dnalborczyk commented Apr 25, 2022

No description provided.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 25, 2022
@targos
Copy link
Member

targos commented Apr 25, 2022

Sorry, just realized I read the change backwards.

I'm not sure about this (may be a bug in the test). Firefox also allows 0:
image

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can we add a test?

@targos
Copy link
Member

targos commented Apr 25, 2022

@aduh95 the test is added in #42855

@aduh95
Copy link
Contributor

aduh95 commented Apr 25, 2022

@aduh95 the test is added in #42855

I think it makes more sense to land the fix and the test for it in one commit. @dnalborczyk is it ok for you to add this change to the other PR and close this one?

@aduh95 aduh95 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 Oct 28, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/42856
✔  Done loading data for nodejs/node/pull/42856
----------------------------------- PR info ------------------------------------
Title      lib: fix AbortSignal.timeout parameter validation (#42856)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     dnalborczyk:abort-signal-timeout -> nodejs:main
Labels     author ready, needs-ci
Commits    1
 - lib: fix `AbortSignal.timeout` parameter validation
Committers 1
 - Antoine du Hamel 
PR-URL: https://github.com/nodejs/node/pull/42856
Fixes: https://github.com/nodejs/node/pull/42855
Reviewed-By: Michaël Zasso 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Colin Ihrig 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42856
Fixes: https://github.com/nodejs/node/pull/42855
Reviewed-By: Michaël Zasso 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Colin Ihrig 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 25 Apr 2022 00:16:07 GMT
   ✔  Approvals: 4
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/42856#pullrequestreview-951412614
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/42856#pullrequestreview-951564890
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/42856#pullrequestreview-952570040
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42856#pullrequestreview-1161296570
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-10-29T20:20:44Z: https://ci.nodejs.org/job/node-test-pull-request/47560/
- Querying data for job/node-test-pull-request/47560/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 42856
From https://github.com/nodejs/node
 * branch                  refs/pull/42856/merge -> FETCH_HEAD
✔  Fetched commits as e43ecd5fec30..a8f1d426ab17
--------------------------------------------------------------------------------
[main c7587535eb] lib: fix `AbortSignal.timeout` parameter validation
 Author: Daniel Nalborczyk 
 Date: Sun Apr 24 20:15:18 2022 -0400
 8 files changed, 207 insertions(+), 3 deletions(-)
 create mode 100644 test/fixtures/wpt/dom/abort/abort-signal-timeout.html
 create mode 100644 test/fixtures/wpt/dom/abort/crashtests/timeout-close.html
 create mode 100644 test/fixtures/wpt/dom/abort/reason-constructor.html
   ✔  Patches applied
--------------------------------------------------------------------------------
--------------------------------- New Message ----------------------------------
lib: fix `AbortSignal.timeout` parameter validation

PR-URL: #42856
Fixes: #42855
Reviewed-By: Michaël Zasso [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Colin Ihrig [email protected]
Reviewed-By: Antoine du Hamel [email protected]

[main bab2062fd3] lib: fix AbortSignal.timeout parameter validation
Author: Daniel Nalborczyk [email protected]
Date: Sun Apr 24 20:15:18 2022 -0400
8 files changed, 207 insertions(+), 3 deletions(-)
create mode 100644 test/fixtures/wpt/dom/abort/abort-signal-timeout.html
create mode 100644 test/fixtures/wpt/dom/abort/crashtests/timeout-close.html
create mode 100644 test/fixtures/wpt/dom/abort/reason-constructor.html
✖ bab2062fd35087d3c135a6d7c89a921528e08aec
✔ 0:0 no Co-authored-by metadata co-authored-by-is-trailer
✖ 2:7 Pull request URL must reference a comment or discussion. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/3358468679

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 31, 2022
@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 31, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2022
@nodejs-github-bot nodejs-github-bot merged commit ffa2e96 into nodejs:main Oct 31, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in ffa2e96

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
PR-URL: #42856
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 9, 2022
PR-URL: nodejs#42856
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
PR-URL: #42856
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #42856
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #42856
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #42856
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants