lib, test: integrate wpt abort_controller tests and fix onabort event by watilde · Pull Request #35869 · nodejs/node

watilde · 2020-10-29T17:18:24Z

Integrate abort_controller's test cases from the WPT and fixed event.target handling based on failure cases of WPT's test result.

Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

lib/internal/abort_controller.js

lib/internal/event_target.js

benjamingr

The fix in abort_controller isn't quite right, I left a comment on a suggested alternative fix.

The comment asking for a test isn't blocking.

Other than that - this is really great and a helpful step towards unflagging.

The AbortController abort event should have EventTarget as a target property of the argument event.

Refs: web-platform-tests/wpt#9361

nodejs-github-bot · 2020-10-31T04:22:23Z

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

test/parallel/test-eventtarget.js

benjamingr

Please consider fixing the test before merging.

Mostly LGTM

Good work :]

nodejs-github-bot · 2020-10-31T09:16:09Z

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

github-actions · 2020-11-01T11:12:46Z

Commit Queue failed

- Loading data for nodejs/node/pull/35869
✔  Done loading data for nodejs/node/pull/35869
----------------------------------- PR info ------------------------------------
Title      lib, test: integrate wpt abort_controller tests and fix onabort event (#35869)
Author     Daijiro Wachi  (@watilde)
Branch     watilde:wpt-abort -> nodejs:master
Labels     events, lib / src, test
Commits    2
 - lib: let abort_controller target be EventTarget
 - test: integrate abort_controller tests from wpt
Committers 1
 - Daijiro Wachi 
PR-URL: https://github.com/nodejs/node/pull/35869
Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js
Reviewed-By: Rich Trott 
Reviewed-By: Michaël Zasso 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35869
Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js
Reviewed-By: Rich Trott 
Reviewed-By: Michaël Zasso 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-10-31T09:16:09Z: https://ci.nodejs.org/job/node-test-pull-request/33988/
- Querying data for job/node-test-pull-request/33988/
✔  Build data downloaded
- Querying failures of job/node-test-commit/41799/
✔  Data downloaded
   ✖  1 failure(s) on the last Jenkins CI run
   ℹ  This PR was created on Thu, 29 Oct 2020 17:18:24 GMT
   ✔  Approvals: 3
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35869#pullrequestreview-519999779
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/35869#pullrequestreview-520063736
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/35869#pullrequestreview-521123505
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/340140135

The AbortController abort event should have EventTarget as a target property of the argument event. PR-URL: #35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: web-platform-tests/wpt#9361 PR-URL: #35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

watilde · 2020-11-01T11:44:43Z

Landed in 5735525...2351e2c

The AbortController abort event should have EventTarget as a target property of the argument event. PR-URL: #35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: web-platform-tests/wpt#9361 PR-URL: #35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

The AbortController abort event should have EventTarget as a target property of the argument event. PR-URL: nodejs#35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: web-platform-tests/wpt#9361 PR-URL: nodejs#35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

The AbortController abort event should have EventTarget as a target property of the argument event. PR-URL: nodejs#35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: web-platform-tests/wpt#9361 PR-URL: nodejs#35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

The AbortController abort event should have EventTarget as a target property of the argument event. PR-URL: nodejs#35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: web-platform-tests/wpt#9361 PR-URL: nodejs#35869 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

The AbortController abort event should have EventTarget as a target property of the argument event. PR-URL: #35869 Backport-PR-URL: #38386 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: web-platform-tests/wpt#9361 PR-URL: #35869 Backport-PR-URL: #38386 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

watilde added events Issues and PRs related to the events subsystem / EventEmitter. test Issues and PRs related to the tests. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 29, 2020

Trott approved these changes Oct 29, 2020

View reviewed changes

targos approved these changes Oct 29, 2020

View reviewed changes

benjamingr approved these changes Oct 29, 2020

View reviewed changes

benjamingr reviewed Oct 29, 2020

View reviewed changes

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

benjamingr reviewed Oct 29, 2020

View reviewed changes

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

benjamingr requested changes Oct 29, 2020

View reviewed changes

watilde added 2 commits October 31, 2020 07:38

lib: let abort_controller target be EventTarget

6763810

The AbortController abort event should have EventTarget as a target property of the argument event.

test: integrate abort_controller tests from wpt

9cf54b8

Refs: web-platform-tests/wpt#9361

watilde force-pushed the wpt-abort branch from 29cf5a1 to 94b7e92 Compare October 31, 2020 04:09

watilde added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020

benjamingr reviewed Oct 31, 2020

View reviewed changes

test/parallel/test-eventtarget.js Outdated Show resolved Hide resolved

benjamingr approved these changes Oct 31, 2020

View reviewed changes

watilde force-pushed the wpt-abort branch from 94b7e92 to 9cf54b8 Compare October 31, 2020 09:03

watilde added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020

watilde added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2020

github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 1, 2020

watilde removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 1, 2020

watilde closed this Nov 1, 2020

watilde deleted the wpt-abort branch November 1, 2020 11:44

targos mentioned this pull request Nov 3, 2020

v15.1.0 release proposal #35948

Merged

BethGriggs added the dont-land-on-v14.x label Dec 7, 2020

targos added backport-open-v14.x and removed dont-land-on-v14.x labels Apr 24, 2021

targos mentioned this pull request Apr 24, 2021

[v14.x] Backport AbortController and friends #38386

Closed

targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 30, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

lib, test: integrate wpt abort_controller tests and fix onabort event#35869

lib, test: integrate wpt abort_controller tests and fix onabort event#35869
watilde wants to merge 2 commits intonodejs:masterfrom
watilde:wpt-abort

watilde commented Oct 29, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

benjamingr left a comment

Uh oh!

nodejs-github-bot commented Oct 31, 2020

Uh oh!

Uh oh!

benjamingr left a comment

Uh oh!

nodejs-github-bot commented Oct 31, 2020

Uh oh!

github-actions bot commented Nov 1, 2020

Uh oh!

watilde commented Nov 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

watilde commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 31, 2020

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 31, 2020

Uh oh!

github-actions bot commented Nov 1, 2020

Uh oh!

watilde commented Nov 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

watilde commented Oct 29, 2020 •

edited

Loading