-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Adding assertion for the test in test-event-emitter-subclass #19082
Conversation
@mscdex @richardlau Please review the code |
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
Who can merge it? |
@stenalpjolly This PR will be hold at least 48-72 hours for waiting input from other Collaborators (refers to COLLABORATOR_GUIDE). If no one requests change for it, this PR will be landed as soon as possible. |
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.
Ignore.
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.
This property isn't declared on the instance but rather the EventEmitter itself. So the test needs to be assert(MyEE.hasOwnProperty('usingDomain');
.
See below for a correction...
But that would fail because MyEE inherits The var assert = require('assert')
var EventsModule = require('events')
var descriptor = Object.getOwnPropertyDescriptor(EventsModule, 'usingDomains')
assert(descriptor) |
The line setting this was removed in a previous commit. This potentially breaks code in the wild using this property. Refs: #17403 (comment) PR-URL: #18944 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sorry, thought this code was using Or if we want to stick with having it in this file, then use |
@stenalpjolly would you be so kind and try to implement the suggestion by @apapirovski? If you need any help, please let me (or any other collaborator) know. |
Ping @stenalpjolly |
Closing due to no response. @stenalpjolly thanks a lot for your contribution and if you would like to work on this again: please either leave a comment so we can reopen the PR or you can just open a new PR (I recommend to do that instead). |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)