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

test: fix test-eventtarget #33670

Closed

Conversation

lundibundi
Copy link
Member

Looks like this slipped through while lending #33611 and #33622 due to the 'old' (3 days) CI.

The NodeEventTarget target test at the end of a file fails with

Error [ERR_EVENT_RECURSION]: The event "foo" is already being dispatched
    at NodeEventTarget.dispatchEvent (internal/event_target.js:250:13)
    at Timeout._onTimeout (/home/lundibundi/dev/node/node/test/parallel/test-eventtarget.js:460:46)
    at listOnTimeout (internal/timers.js:551:17)
    at processTimers (internal/timers.js:494:7) {
  code: 'ERR_EVENT_RECURSION'
}

But since it is being removed anyway (#33665) I decided to just delete it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

/cc @benjamingr @jasnell @nodejs/events

benjamingr and others added 5 commits May 31, 2020 13:22
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: nodejs#33613
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
PR-URL: nodejs#33618
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#33615
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
PR-URL: nodejs#33623
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 31, 2020
@benjamingr
Copy link
Member

@lundibundi I made a mess landing a PR by mistake and force-pushed. @BridgeAR is currently helping me figure out what went wrong.

Anyway, LGTM and 👍 to fast track.

@BridgeAR
Copy link
Member

Seems like this test just landed about an hour ago and there is another issue on master right now. I am going to force push these out, if no one is against that?

@benjamingr
Copy link
Member

I am fine with either landing this or the force push.

As NodeEventTarget is likely to be removed soon anyway.

@BridgeAR
Copy link
Member

Closing as these are not on master anymore.

@BridgeAR BridgeAR closed this May 31, 2020
@BridgeAR
Copy link
Member

Thanks for opening the fix though!

@lundibundi
Copy link
Member Author

Well, it looks like the new Event() and 8ae28ff is still there. I could update the branch to only have the fix for that.

@benjamingr
Copy link
Member

I'm not sure I understand the issue with 8ae28ff does it fail CI or is otherwise problematic?

@BridgeAR
Copy link
Member

@lundibundi thanks, it's now also reverted. I reopened all corresponding PRs and I am looking into PRs that might have been worked on in the meanwhile and check that everything is in order for them right now.

@BridgeAR
Copy link
Member

@benjamingr it failed a test. Even if the CI was originally green, something that landed intermediately since the last CI run could have caused the new code to fail / cause issues with other code.

@lundibundi
Copy link
Member Author

That commit adds a check for no-arg Event constructor call, and such call is was present https://github.com/nodejs/node/blob/master/test/parallel/test-eventtarget.js#L411
(the test for Symbol.toStringTag of Event).

@BridgeAR @benjamingr great, then it should be okay.

@BridgeAR
Copy link
Member

Ok, I just checked and everything seems to be in good shape. The good part is that it's a very new code part that no one else was working on right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants