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, test: integrate wpt abort_controller tests and fix onabort event #35869

Closed
wants to merge 2 commits into from

Conversation

watilde
Copy link
Member

@watilde watilde commented Oct 29, 2020

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

@watilde 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
Copy link
Member

@benjamingr benjamingr left a 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.

The AbortController abort event should have EventTarget as a target
property of the argument event.
@watilde watilde added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@benjamingr benjamingr left a 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 :]

@watilde watilde added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020
@nodejs-github-bot
Copy link
Collaborator

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

github-actions bot commented Nov 1, 2020

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

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 1, 2020
@watilde watilde removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 1, 2020
watilde added a commit that referenced this pull request Nov 1, 2020
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
Copy link
Member Author

watilde commented Nov 1, 2020

Landed in 5735525...2351e2c

@watilde watilde closed this Nov 1, 2020
@watilde watilde deleted the wpt-abort branch November 1, 2020 11:44
targos pushed a commit that referenced this pull request Nov 3, 2020
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 targos mentioned this pull request Nov 3, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
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 24, 2021
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
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 26, 2021
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
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
targos pushed a commit that referenced this pull request Apr 30, 2021
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
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants