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

Adding assertion for the test in test-event-emitter-subclass #19082

Closed
wants to merge 1 commit into from

Conversation

stenalpjolly
Copy link

@stenalpjolly stenalpjolly commented Mar 2, 2018

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
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 2, 2018
@stenalpjolly
Copy link
Author

@mscdex @richardlau Please review the code

Copy link
Contributor

@starkwang starkwang left a comment

Choose a reason for hiding this comment

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

LGTM

@stenalpjolly
Copy link
Author

Who can merge it?

@starkwang
Copy link
Contributor

starkwang commented Mar 2, 2018

@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.

@starkwang
Copy link
Contributor

apapirovski
apapirovski previously approved these changes Mar 2, 2018
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Ignore.

Copy link
Member

@apapirovski apapirovski left a 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...

@richardlau
Copy link
Member

This property isn't declared on the instance but rather the EventEmitter itself. So the test needs to be assert(MyEE.hasOwnProperty('usingDomain');.

But that would fail because MyEE inherits usingDomain from EventEmitter.

The myee.hasOwnProperty('usingDomains'); line was added by #18944, which addresses #17403 (comment) where the minimal testcase was:

var assert = require('assert')
var EventsModule = require('events')
var descriptor = Object.getOwnPropertyDescriptor(EventsModule, 'usingDomains')
assert(descriptor)

richardlau referenced this pull request Mar 2, 2018
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@apapirovski
Copy link
Member

apapirovski commented Mar 2, 2018

But that would fail because MyEE inherits usingDomain from EventEmitter.

Sorry, thought this code was using _extend not inherits. Either way, myee.hasOwnProperty is incorrect. This should most likely just be assert(EventEmitter.hasOwnProperty('usingDomains')); and be in an entirely different test file than this one.

Or if we want to stick with having it in this file, then use Reflect.has(MyEE, 'usingDomains');.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

@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.

@BridgeAR
Copy link
Member

Ping @stenalpjolly

@BridgeAR
Copy link
Member

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).

@BridgeAR BridgeAR closed this Apr 28, 2018
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.

8 participants