test: increase coverage for lib/events.js by captainsafia · Pull Request #9865 · nodejs/node

captainsafia · 2016-12-01T04:11:54Z

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.

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

Trott

LGTM if CI is ✅

Trott · 2016-12-01T04:14:35Z

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

MylesBorins · 2016-12-01T04:23:57Z

LGTM IF ci is green

Trott · 2016-12-01T04:44:59Z

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/

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.

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 · 2016-12-01T21:52:44Z

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

Trott · 2016-12-02T03:57:53Z

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 · 2016-12-02T04:43:18Z

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

Trott · 2016-12-02T04:45:11Z

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

addaleax

Still LGTM :)

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 · 2016-12-03T04:45:56Z

Landed in 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: #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>

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>

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>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016

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.

captainsafia force-pushed the event-emitter-coverage branch from bb6db3d to 6d63d0d Compare December 1, 2016 04:12

Trott added the events Issues and PRs related to the events subsystem / EventEmitter. label Dec 1, 2016

Trott approved these changes Dec 1, 2016

View reviewed changes

cjihrig approved these changes Dec 1, 2016

View reviewed changes

captainsafia mentioned this pull request Dec 1, 2016

events: remove unneeded safety check code #9866

Closed

2 tasks

lpinca approved these changes Dec 1, 2016

View reviewed changes

addaleax approved these changes Dec 1, 2016

View reviewed changes

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.

test: move TestStream inheritance modification

f6d8b1b

addaleax approved these changes Dec 2, 2016

View reviewed changes

Trott closed this Dec 3, 2016

Fishrock123 mentioned this pull request Dec 5, 2016

v7.2.1 proposal #10127

Merged

2 tasks

MylesBorins added dont-land-on-v4.x labels Dec 20, 2016

MylesBorins mentioned this pull request Dec 21, 2016

V6.9.3 proposal #10394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

test: increase coverage for lib/events.js#9865
captainsafia wants to merge 3 commits intonodejs:masterfrom
captainsafia:event-emitter-coverage

captainsafia commented Dec 1, 2016 •

edited by Trott

Loading

Uh oh!

Trott left a comment

Uh oh!

Trott commented Dec 1, 2016

Uh oh!

MylesBorins commented Dec 1, 2016

Uh oh!

Trott commented Dec 1, 2016

Uh oh!

captainsafia commented Dec 1, 2016

Uh oh!

Trott commented Dec 2, 2016

Uh oh!

captainsafia commented Dec 2, 2016

Uh oh!

Trott commented Dec 2, 2016

Uh oh!

addaleax left a comment

Uh oh!

Trott commented Dec 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

captainsafia commented Dec 1, 2016 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 1, 2016

Uh oh!

MylesBorins commented Dec 1, 2016

Uh oh!

Trott commented Dec 1, 2016

Uh oh!

captainsafia commented Dec 1, 2016

Uh oh!

Trott commented Dec 2, 2016

Uh oh!

captainsafia commented Dec 2, 2016

Uh oh!

Trott commented Dec 2, 2016

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

captainsafia commented Dec 1, 2016 •

edited by Trott

Loading