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

events: Start porting WPT tests #33621

Closed
wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

@benjamingr benjamingr commented May 28, 2020

I tried porting the WPT tests through git node wpt - it worked pretty well initially but there were several big issues:

  • All the meaningful tests are HTML files.
  • Even the JS files contained a lot of DOM logic.
  • The WHATWG tests don't test EventTarget directly - rather they test document and window as event targets.

I started down the path of implementing a document, window, appendChild and then decided it would probably be easier to just manually port these tests after changing them to terms Node understands.

Ideally, we could contribute these back to WPT as less-tried-to-document tests for EventTarget.

For example where the test does document.addEventListener I do let document = new EventEmitter() first.

When I tried porting https://github.com/web-platform-tests/wpt/blob/master/dom/events/AddEventListenerOptions-once.html#L36-L57 it threw ERR_EVENT_RECURSION indicating that that behavior is probably not spec compliant. @jasnell any objections to changing it to align with the spec and allow dispatching an event while in an event handler for the same handler?

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

@benjamingr benjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label May 28, 2020
@benjamingr benjamingr requested a review from jasnell May 28, 2020 17:18
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 28, 2020
@jasnell
Copy link
Member

jasnell commented May 28, 2020

We had a similar issue when introducing the WHATWG URL parser which is why we have so many test-whatwg-url-* tests and don't rely so much on the WPT tests for those. I suspect we'll have to do the same.

When I tried porting web-platform-tests/wpt:dom/events/AddEventListenerOptions-once.html@master#L36-L57 it threw ERR_EVENT_RECURSION indicating that that behavior is probably not spec compliant. @jasnell any objections to changing it to align with the spec and allow dispatching an event while in an event handler for the same handler?

Yeah, I suspect we should actually file this as an issue with the EventTarget standard itself. The following test case actually crashes Chrome for me and Firefox throws with an error about too much recursion:

const et = new EventTarget();
et.addEventListener('foo', () => et.dispatchEvent(new Event('foo')));
et.dispatchEvent(new Event('foo'));

It's an obvious footgun that really shouldn't be allowed. Note that we currently have the same issue with EventEmitter in that we allow ee.on('foo', () => ee.emit('foo')) which throws a max call stack exceeded error. It's a deeper problem with EventTarget because the error is just pushed to uncaught error and does not necessarily stop the progression of the code in browsers. The ERR_EVENT_RECURSION may not be fully spec compliant here but it prevents these issues and should be kept.

@benjamingr
Copy link
Member Author

benjamingr commented May 28, 2020

@jasnell

. I suspect we'll have to do the same.

Roger, I'll start porting the whatwg tests that route. I'm also checking with the #whatwg people on IRC to see if they're interesting in upstreaming those tests (for example: tests directly testing EventTarget rather than document)

I think I fixed the root cause for the recursion issue in #33624 - it's fine to dispatch events from events with the same type as long as they're not the same isntance.

If you'd like we can add a "max call depth" or similar check though I would rather not and have the user just reach "Maximum call stack exceeded" like any other non-event-related recusion and like browsers do.

I'll add a "WIP" to the title in the meantime (at least until #33624 and some other fixes land).

@benjamingr benjamingr changed the title events: Start porting WPT tests WIP: events: Start porting WPT tests May 28, 2020
@benjamingr
Copy link
Member Author

@jasnell any feelings regarding just porting all the WPTs in one PR (with a pass/fail status, a bunch failing) rather than make a lot of smaller changes?

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@jasnell jasnell added the eventtarget Issues and PRs related to the EventTarget implementation. label Jun 19, 2020
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2020
@nodejs-github-bot
Copy link
Collaborator

@benjamingr benjamingr changed the title WIP: events: Start porting WPT tests events: Start porting WPT tests Nov 5, 2020
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 5, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 5, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/33621
✔  Done loading data for nodejs/node/pull/33621
----------------------------------- PR info ------------------------------------
Title      events: Start porting WPT tests (#33621)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     benjamingr:event-target-wpt-start -> nodejs:master
Labels     events, eventtarget, test
Commits    1
 - events: port some wpt tests
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/33621
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/33621
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - events: port some wpt tests
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-11-05T18:04:15Z: https://ci.nodejs.org/job/node-test-pull-request/34100/
- Querying data for job/node-test-pull-request/34100/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Thu, 28 May 2020 17:18:14 GMT
   ✔  Approvals: 1
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/33621#pullrequestreview-421144116
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

@benjamingr
Copy link
Member Author

I'll land it manually

benjamingr added a commit that referenced this pull request Nov 6, 2020
PR-URL: #33621
Reviewed-By: James M Snell <[email protected]>
@benjamingr
Copy link
Member Author

Landed in 2a1273c 🎉

@benjamingr benjamingr closed this Nov 6, 2020
@Ethan-Arrowood
Copy link
Contributor

Ethan-Arrowood commented Nov 6, 2020

this creates conflicts with tests added in #34169

@benjamingr benjamingr deleted the event-target-wpt-start branch November 6, 2020 15:10
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #33621
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
PR-URL: #33621
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #33621
Reviewed-By: James M Snell <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #33621
Reviewed-By: James M Snell <[email protected]>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 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. eventtarget Issues and PRs related to the EventTarget implementation. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants