FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add EventTarget WPT test AddEventListenerOptions-once by Ethan-Arrowood · Pull Request #34169 · nodejs/node · GitHub

/ node Public

Add EventTarget WPT test AddEventListenerOptions-once - #34169

Closed
Ethan-Arrowood wants to merge 2 commits into
nodejs:masterfrom
Ethan-Arrowood:add-event-target-wpt
Closed

Add EventTarget WPT test AddEventListenerOptions-once#34169
Ethan-Arrowood wants to merge 2 commits into
nodejs:masterfrom
Ethan-Arrowood:add-event-target-wpt

Conversation

Ethan-Arrowood commented Jul 2, 2020
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

First of many PRs adding WPT tests for EventTarget (and eventually AbortController)

nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 2, 2020

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM + please add the reference - thanks for this!

Ethan-Arrowood changed the title Add EventTarget WPT tests Add EventTarget WPT test AddEventListenerOptions-once Jul 2, 2020
Ethan-Arrowood marked this pull request as ready for review July 2, 2020 15:51
Ethan-Arrowood requested a review from benjamingr July 2, 2020 15:52

Copy link
Copy Markdown
Contributor Author

This test is what is failing the CI: https://github.com/nodejs/node/pull/34169/checks?check_run_id=831452407#step:11:10071

Any ideas?

Ethan-Arrowood commented Jul 7, 2020
edited
Loading

Copy link
Copy Markdown
Contributor Author

New commit moves away from the count checking and reorganizes the tests using mustCall. This now deviates a bit from the WPT tests but I believe achieves the same thing. If this is not the intended change I can revert the last commit!

One note; I tried implementing this:

{
  const document = new EventTarget();

  const handler = common.mustCall(2)
  // Both should only fire on first event
  document.addEventListener('test', handler, { once: true });
  document.addEventListener('test', handler, { once: true });
  // Fire events
  document.dispatchEvent(new Event('test'));
  document.dispatchEvent(new Event('test'));
}

But it fails - probably my own misunderstanding of common.mustCall. Maybe you two have some in sight here?

jasnell commented Jul 7, 2020

Copy link
Copy Markdown
Member

It's failing because, unlike EventEmitter, a given function can only be added once. So the second document.addEventListener('test', handler, { once: true }); has no effect. Try changing it to document.addEventListener('test', handler.bind(), { once: true }); and it should hopefully work.

Copy link
Copy Markdown
Contributor Author

Awesome worked like a charm! Thank you

Copy link
Copy Markdown
Contributor Author

Hey! I have another test file ready to go. Should I push it to this PR/branch or include it in another branch?

jasnell commented Aug 3, 2020

Copy link
Copy Markdown
Member

@Ethan-Arrowood ... just push it here. We can do another CI run on this and give it another day to land.

Copy link
Copy Markdown
Member

@Ethan-Arrowood could you please squash/fixup the appropriate commits and remove the merge commit, our CI doesn't handle merge commits very well? This looks ready to land to me after a successful CI.

Copy link
Copy Markdown
Contributor Author

Alright squashed and fixed up your comments.

lundibundi added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 24, 2020

This comment has been minimized.

This comment has been minimized.

Copy link
Copy Markdown
Contributor Author

This pr #33621 introduces some conflicts with test-eventtarget-whatwg-once.js.

I'm pushing another rebase that fixes some of that. I guess this PR needs to be reviewed again

aduh95 added dont-land-on-v10.x request-ci Add this label to start a Jenkins CI on a PR. labels Nov 6, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Contributor Author

@aduh95 let me know if theres anything else I should do here

aduh95 commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

@Ethan-Arrowood can you rebase to resolve the git conflict please?

aduh95 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 9, 2020
add reference comments for WPT tests

convert to common mustCall

Update test/parallel/test-eventtarget-whatwg-once.js

Co-authored-by: James M Snell <jasnell@gmail.com>

Update test/parallel/test-eventtarget-whatwg-passive.js

Co-authored-by: James M Snell <jasnell@gmail.com>

convert other tests to utilize common mustcall

improve test with bind

add customevent wpt

add no-unused-vars comment

reorder header

utilize common.mustcall

remove internal and use global EventTarget
aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 9, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020

This comment has been minimized.

This comment has been minimized.

Copy link
Copy Markdown
Collaborator

aduh95 pushed a commit that referenced this pull request Nov 10, 2020
Add WPT AddEventListenerOptions-once test.

PR-URL: #34169
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>

aduh95 commented Nov 10, 2020

Copy link
Copy Markdown
Contributor

Landed in 7cba786

aduh95 closed this Nov 10, 2020
codebytere pushed a commit that referenced this pull request Nov 22, 2020
Add WPT AddEventListenerOptions-once test.

PR-URL: #34169
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
codebytere mentioned this pull request Nov 22, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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 outstanding review comments, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL