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: exercise EE function type checking paths #8168

Merged
merged 1 commit into from Sep 9, 2016

Conversation

Projects
None yet
6 participants
@cjihrig
Contributor

cjihrig commented Aug 18, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

This commit adds tests for on(), once(), removeListener(), and prependOnceListener(), which all throw a TypeError if the listener argument is not a function.

@cjihrig cjihrig added the events label Aug 18, 2016

@Trott

View changes

Show outdated Hide outdated test/parallel/test-event-emitter-add-listeners.js
@Trott

View changes

Show outdated Hide outdated test/parallel/test-event-emitter-add-listeners.js
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 18, 2016

Member

LGTM if CI is green

Member

Trott commented Aug 18, 2016

LGTM if CI is green

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
const ee = new EventEmitter();
ee.on('foo', null);
}, /^TypeError: "listener" argument must be a function$/);

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 19, 2016

Member

Adding a final message param may be helpful (I forget but I don't think throws logs the regex?)

@Fishrock123

Fishrock123 Aug 19, 2016

Member

Adding a final message param may be helpful (I forget but I don't think throws logs the regex?)

This comment has been minimized.

@cjihrig

cjihrig Aug 19, 2016

Contributor

What would you like the message to be? If an incorrect error is thrown, the message isn't used at all. If an error isn't thrown, it says AssertionError: Missing expected exception., which is what I would have used as the message.

@cjihrig

cjihrig Aug 19, 2016

Contributor

What would you like the message to be? If an incorrect error is thrown, the message isn't used at all. If an error isn't thrown, it says AssertionError: Missing expected exception., which is what I would have used as the message.

This comment has been minimized.

@cjihrig

cjihrig Aug 24, 2016

Contributor

Ping @Fishrock123 on ideas here.

@cjihrig

cjihrig Aug 24, 2016

Contributor

Ping @Fishrock123 on ideas here.

const ee = new EventEmitter();
ee.removeListener('foo', null);
}, /^TypeError: "listener" argument must be a function$/);

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 19, 2016

Member

ditto

@Fishrock123
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Aug 19, 2016

Member

lgtm with ci & nits

Member

Fishrock123 commented Aug 19, 2016

lgtm with ci & nits

// Verify that the listener must be a function
assert.throws(() => {
const ee = new EventEmitter();

This comment has been minimized.

@jasnell

jasnell Aug 19, 2016

Member

extremely minor nit: unnecessary blank line?

@jasnell

jasnell Aug 19, 2016

Member

extremely minor nit: unnecessary blank line?

This comment has been minimized.

@cjihrig

cjihrig Aug 19, 2016

Contributor

I dunno. A blank line after a variable declaration is pretty common throughout the codebase.

@cjihrig

cjihrig Aug 19, 2016

Contributor

I dunno. A blank line after a variable declaration is pretty common throughout the codebase.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 19, 2016

Member

LGTM with the nits addressed.

Member

jasnell commented Aug 19, 2016

LGTM with the nits addressed.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
test: exercise EE function type checking paths
This commit adds tests for on(), once(), removeListener(),
and prependOnceListener(), which all throw a TypeError if
the listener argument is not a function.

PR-URL: #8168
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@cjihrig cjihrig merged commit e57ff45 into nodejs:master Sep 9, 2016

@cjihrig cjihrig deleted the cjihrig:ee-test branch Sep 9, 2016

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

test: exercise EE function type checking paths
This commit adds tests for on(), once(), removeListener(),
and prependOnceListener(), which all throw a TypeError if
the listener argument is not a function.

PR-URL: #8168
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport

Member

MylesBorins commented Sep 30, 2016

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment