GeezerVison

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

GeezerVison is showing github.com — lib, test: integrate wpt abort_controller tests and fix onabort event by watilde · Pull Request #35869 · nodejs/node

Conversation

watilde commented Oct 29, 2020 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

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

benjamingr and bricss reacted with thumbs up emoji

All reactions

👍 2 reactions

watilde added events

Issues and PRs related to EventEmitter and the events module.

test

Issues and PRs related to Node.js core tests and test infrastructure.

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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

Comment thread

lib/internal/abort_controller.js Resolved

Uh oh!

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

benjamingr reviewed

Oct 29, 2020

View reviewed changes

Comment thread

lib/internal/event_target.js Resolved

Uh oh!

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

benjamingr requested changes

Oct 29, 2020

View reviewed changes

benjamingr left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

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

watilde reacted with heart emoji

All reactions

❤️ 1 reaction

watilde added 2 commits

October 31, 2020 07:38


          lib: let abort_controller target be EventTarget

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. Only starts once the PR has an approving review. label

Oct 31, 2020

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

Oct 31, 2020

nodejs-github-bot commented Oct 31, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

benjamingr reviewed

Oct 31, 2020

View reviewed changes

Comment thread

test/parallel/test-eventtarget.js Resolved

Uh oh!

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

benjamingr approved these changes

Oct 31, 2020

View reviewed changes

benjamingr left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please consider fixing the test before merging.

Mostly LGTM

Good work :]

Sorry, something went wrong.

Uh oh!

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

watilde reacted with heart emoji

All reactions

❤️ 1 reaction

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. Only starts once the PR has an approving review. label

Oct 31, 2020

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

Oct 31, 2020

nodejs-github-bot commented Oct 31, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

watilde added the commit-queue PRs queued for automated landing through the Commit Queue. label

Nov 1, 2020

github-actions Bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Nov 1, 2020

github-actions Bot commented Nov 1, 2020

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

github-actions Bot added the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label

Nov 1, 2020

watilde removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label

Nov 1, 2020

watilde added a commit that referenced this pull request

Nov 1, 2020


          lib: let abort_controller target be EventTarget

44571d1

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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

watilde added a commit that referenced this pull request

Nov 1, 2020


          test: integrate abort_controller tests from wpt

2351e2c

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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

watilde commented Nov 1, 2020

Copy link

Copy Markdown

Member Author

Landed in 5735525...2351e2c

benjamingr 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.

watilde closed this

Nov 1, 2020

watilde deleted the wpt-abort branch

November 1, 2020 11:44

targos pushed a commit that referenced this pull request

Nov 3, 2020


          lib: let abort_controller target be EventTarget

d2f574b

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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos pushed a commit that referenced this pull request

Nov 3, 2020


          test: integrate abort_controller tests from wpt

da612df

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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

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 pushed a commit to targos/node that referenced this pull request

Apr 30, 2021


          lib: let abort_controller target be EventTarget

f76ab5a

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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos pushed a commit to targos/node that referenced this pull request

Apr 30, 2021


          test: integrate abort_controller tests from wpt

76cdf87

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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos pushed a commit that referenced this pull request

Apr 30, 2021


          lib: let abort_controller target be EventTarget

b2f8e8d

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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos pushed a commit that referenced this pull request

Apr 30, 2021


          test: integrate abort_controller tests from wpt

88f4261

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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

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

Reviewers

Trott

Trott approved these changes

benjamingr

benjamingr approved these changes

+1 more reviewer

targos

targos approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

events

Issues and PRs related to EventEmitter and the events module.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

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.

Uh oh!

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

6 participants

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.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

watilde commented Oct 29, 2020 •

edited

Loading

Uh oh!

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