lib: convert signals to array before validation by jazelly · Pull Request #54714 · nodejs/node

jazelly · 2024-09-02T06:43:28Z

Fixes: #54674

nodejs-github-bot · 2024-09-02T06:43:33Z

Review requested:

@nodejs/crypto
@nodejs/web-standards

codecov · 2024-09-02T08:10:08Z

Codecov Report

Attention: Patch coverage is 90.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (981c701) to head (80b16d1).
Report is 252 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/webidl.js	88.31%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54714      +/-   ##
==========================================
- Coverage   87.61%   87.60%   -0.01%     
==========================================
  Files         650      650              
  Lines      182835   182881      +46     
  Branches    35382    35397      +15     
==========================================
+ Hits       160185   160218      +33     
- Misses      15925    15930       +5     
- Partials     6725     6733       +8

Files with missing lines	Coverage Δ
lib/internal/abort_controller.js	`97.90% <100.00%> (+0.03%)`	⬆️
lib/internal/crypto/webidl.js	`98.29% <100.00%> (+0.65%)`	⬆️
lib/internal/webidl.js	`94.57% <88.31%> (-1.30%)`	⬇️

... and 33 files with indirect coverage changes

lib/internal/abort_controller.js

test/parallel/test-abortsignal-any.mjs

mcollina

does this fix any wpts?

KhafraDev · 2024-09-02T15:35:54Z

the WPTs don't usually test webidl validations because implementations are assumed to be using them for the dozens of specs they implement

mcollina

lgtm

mertcanaltin

lgtm

lib/internal/webidl.js

Co-authored-by: Jake Yuesong Li <jake.yuesong@gmail.com>

jakecastelli · 2024-09-03T23:12:34Z

osx failure on CI due to java.io.IOException: No space left on device (6h ago) however I also observed nodejs/jenkins-alerts#2785 (10h ago) mentioned The machine has now enough disk space 🙌

Other failures are most likely flaky tests, rerunning

update: rerun works on osx, seems the no space issue has been resolved.

nodejs-github-bot · 2024-09-04T07:36:13Z

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

nodejs-github-bot · 2024-09-04T10:36:34Z

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

nodejs-github-bot · 2024-09-06T17:40:22Z

Landed in 973144b

Co-authored-by: Jake Yuesong Li <jake.yuesong@gmail.com> PR-URL: #54714 Fixes: #54674 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Co-authored-by: Jake Yuesong Li <jake.yuesong@gmail.com> PR-URL: nodejs#54714 Fixes: nodejs#54674 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Sep 2, 2024

jazelly force-pushed the fix-abort-any branch from 5ad876a to 78b914d Compare September 2, 2024 06:45

jazelly changed the title ~~abort_controller: convert signals to array before validation~~ lib: convert signals to array before validation Sep 2, 2024

aduh95 reviewed Sep 2, 2024

View reviewed changes

lib/internal/abort_controller.js Outdated Show resolved Hide resolved

jakecastelli reviewed Sep 2, 2024

View reviewed changes

test/parallel/test-abortsignal-any.mjs Outdated Show resolved Hide resolved

mcollina reviewed Sep 2, 2024

View reviewed changes

mcollina approved these changes Sep 2, 2024

View reviewed changes

benjamingr approved these changes Sep 2, 2024

View reviewed changes

KhafraDev approved these changes Sep 2, 2024

View reviewed changes

mertcanaltin approved these changes Sep 2, 2024

View reviewed changes

jazelly force-pushed the fix-abort-any branch from 78b914d to 7516052 Compare September 2, 2024 22:58

KhafraDev reviewed Sep 3, 2024

View reviewed changes

lib/internal/webidl.js Outdated Show resolved Hide resolved

lib: convert signals to array before validation

80b16d1

Co-authored-by: Jake Yuesong Li <jake.yuesong@gmail.com>

jazelly force-pushed the fix-abort-any branch from 7516052 to 80b16d1 Compare September 3, 2024 05:04

aymen94 approved these changes Sep 3, 2024

View reviewed changes

jakecastelli 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. web-standards Issues and PRs related to Web APIs labels Sep 3, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2024

This comment was marked as outdated.

Sign in to view

Ethan-Arrowood approved these changes Sep 3, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024

aduh95 approved these changes Sep 6, 2024

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2024

nodejs-github-bot merged commit 973144b into nodejs:main Sep 6, 2024

RafaelGSS mentioned this pull request Sep 16, 2024

v22.9.0 proposal #54966

Merged

targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024

jazelly deleted the fix-abort-any branch November 25, 2024 22:07

This was referenced Apr 16, 2025

lib: convert transfer sequence to array in js #55317

Merged

lib: implement webidl dictionary converter and use it in structuredClone #55489

Merged

Uh oh!

Conversation

jazelly commented Sep 2, 2024

Uh oh!

nodejs-github-bot commented Sep 2, 2024 • edited by jakecastelli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

KhafraDev commented Sep 2, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mertcanaltin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

jakecastelli commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Sep 4, 2024

Uh oh!

nodejs-github-bot commented Sep 4, 2024 • edited by jakecastelli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

nodejs-github-bot commented Sep 2, 2024 •

edited by jakecastelli

Loading

codecov bot commented Sep 2, 2024 •

edited

Loading

jakecastelli commented Sep 3, 2024 •

edited

Loading

nodejs-github-bot commented Sep 4, 2024 •

edited by jakecastelli

Loading