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.

Sign up for GitHub

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

Jump to bottom

test: increase coverage for lib/events.js #9865

Closed
wants to merge 3 commits into from
Closed

test: increase coverage for lib/events.js #9865

wants to merge 3 commits into from

Conversation

captainsafia
Copy link
Contributor

@captainsafia captainsafia commented Dec 1, 2016 •
edited by Trott
Loading

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test, events

Description of change

This adds a test for the case in which listeners() is invoked on an
EventEmitter with no events.

Note that the EventEmitter does not allow _events to ever be
undefined as it is instantiated in the initializer. The only situation in
which it might be undefined is if it is monkey-patched externally.
Because of this, the added tests monkey-patches this._events to
mimic this expected behavior.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@captainsafia
test: increase coverage for lib/events.js
6d63d0d 
This adds tests for the case in which listeners() is invoked on a
EventEmitter with no events.
@Trott Trott added the events Issues and PRs related to the events subsystem / EventEmitter. label Dec 1, 2016
Trott
Trott approved these changes Dec 1, 2016
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is ✅

@Trott
Copy link
Member

Trott commented Dec 1, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/5035/

@MylesBorins
Copy link
Contributor

LGTM IF ci is green

@Trott
Copy link
Member

Trott commented Dec 1, 2016

Two failed hosts look CI infra related, not related to this test. But because EVERYONE LOVES GREEN, let's try again: https://ci.nodejs.org/job/node-test-pull-request/5037/

captainsafia added a commit to captainsafia/node that referenced this pull request Dec 1, 2016
@captainsafia
events: remove unneeded safety check code
46ea5b6 
In the process of creating nodejs#9865, it was discovered that the code
checking whether or not events was defined was unnecessary because
the only situation in which events would be undefined is if it is
monkeypatched by an external entity. This should be removed in order
to discourage this. If the test added in nodejs#9865 is merged, it will
need to removed on merge of this commit.
@captainsafia captainsafia mentioned this pull request Dec 1, 2016
Closed
2 tasks
@captainsafia
test: add new test case for undefined _events
c5fd061 
This adds a new test case for the situation in which a class
inherits from the EventEmitter but overrides the constructor
in the EventEmitter so that the _events is never set.
@captainsafia
Copy link
Contributor Author

I added tests for the case explored in #9866 in which _events can be undefined without monkeypatching.

@Trott
Copy link
Member

Trott commented Dec 2, 2016

Thanks for adding the additional test! util.inherits() mutates its first argument. So I would prefer that either util.inherits() be moved right under the class statement or else that the class statement be moved to this block. Otherwise, there may be some surprises if we add another block at a later date that uses TestStream.

@captainsafia
Copy link
Contributor Author

@Trott: Great point! I've update the code.

@Trott
Copy link
Member

Trott commented Dec 2, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/5097/

addaleax
addaleax approved these changes Dec 2, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM :)

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 3, 2016
@captainsafia @Trott
test: increase coverage for lib/events.js
a912b79 
Adds tests for the case in which listeners() is invoked on a
EventEmitter with no events.

Adds a new test case for the situation in which a class
inherits from the EventEmitter but overrides the constructor
in the EventEmitter so that the _events is never set.

PR-URL: nodejs#9865
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@Trott
Copy link
Member

Trott commented Dec 3, 2016

Landed in a912b79

@Trott Trott closed this Dec 3, 2016
addaleax pushed a commit that referenced this pull request Dec 5, 2016
@captainsafia @addaleax
test: increase coverage for lib/events.js
a6377a9 
Adds tests for the case in which listeners() is invoked on a
EventEmitter with no events.

Adds a new test case for the situation in which a class
inherits from the EventEmitter but overrides the constructor
in the EventEmitter so that the _events is never set.

PR-URL: #9865
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
Merged
2 tasks
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
@captainsafia
test: increase coverage for lib/events.js
374ca08 
Adds tests for the case in which listeners() is invoked on a
EventEmitter with no events.

Adds a new test case for the situation in which a class
inherits from the EventEmitter but overrides the constructor
in the EventEmitter so that the _events is never set.

PR-URL: nodejs#9865
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
@captainsafia @MylesBorins
test: increase coverage for lib/events.js
afbd8df 
Adds tests for the case in which listeners() is invoked on a
EventEmitter with no events.

Adds a new test case for the situation in which a class
inherits from the EventEmitter but overrides the constructor
in the EventEmitter so that the _events is never set.

PR-URL: #9865
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@captainsafia @MylesBorins
test: increase coverage for lib/events.js
2957334 
Adds tests for the case in which listeners() is invoked on a
EventEmitter with no events.

Adds a new test case for the situation in which a class
inherits from the EventEmitter but overrides the constructor
in the EventEmitter so that the _events is never set.

PR-URL: #9865
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
events Issues and PRs related to the events subsystem / EventEmitter. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@captainsafia @Trott @MylesBorins @addaleax @lpinca @cjihrig @nodejs-github-bot