-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Add EventTarget WPT test AddEventListenerOptions-once #34169
Conversation
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.
LGTM + please add the reference - thanks for this!
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? |
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? |
It's failing because, unlike |
Awesome worked like a charm! Thank you |
Hey! I have another test file ready to go. Should I push it to this PR/branch or include it in another branch? |
@Ethan-Arrowood ... just push it here. We can do another CI run on this and give it another day to land. |
@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. |
829dbba
to
ba06c98
Compare
Alright squashed and fixed up your comments. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 |
e6e632c
to
9449f27
Compare
@aduh95 let me know if theres anything else I should do here |
@Ethan-Arrowood can you rebase to resolve the git conflict please? |
add reference comments for WPT tests convert to common mustCall Update test/parallel/test-eventtarget-whatwg-once.js Co-authored-by: James M Snell <[email protected]> Update test/parallel/test-eventtarget-whatwg-passive.js Co-authored-by: James M Snell <[email protected]> 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
9449f27
to
7f6ada3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Add WPT AddEventListenerOptions-once test. PR-URL: #34169 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
Landed in 7cba786 |
Add WPT AddEventListenerOptions-once test. PR-URL: #34169 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesFirst of many PRs adding WPT tests for EventTarget (and eventually AbortController)