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

test: refactor event-emitter-remove-all-listeners #13165

Closed
wants to merge 1 commit into from

Conversation

@Trott
Copy link
Member

commented May 23, 2017

Use common.mustNotCall() to confirm that listener handles never run (as
no events are emitted).

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

test events

test: refactor event-emitter-remove-all-listeners
Use common.mustNotCall() to confirm that listener handles never run (as
no events are emitted).

@Trott Trott added events test labels May 23, 2017

ee.on('bar', common.noop);
ee.on('baz', common.noop);
ee.on('baz', common.noop);
const noop = common.mustNotCall();

This comment has been minimized.

Copy link
@mscdex

mscdex May 23, 2017

Contributor

Why not bring this up to the parent scope and use it in the other places below that are now manually calling common.mustNotCall()?

This comment has been minimized.

Copy link
@refack

refack May 23, 2017

Member

+1
But give it a better name... maybe mustNotCall or if your thight with char dontCall or even doNot

This comment has been minimized.

Copy link
@Trott

Trott May 23, 2017

Author Member

Why not bring this up to the parent scope and use it in the other places below that are now manually calling common.mustNotCall()?

@mscdex Happy to do that if there's consensus that it would be an improvement. I didn't do it because:

  • It's not needed as an identifier in the other scopes. (It's needed here because of the assert.deepStrictEqual() comparisons.)

  • Fewer global identifiers shared across different block scopes helps make those things easily cut-and-paste-able into separate test files for refactoring, etc. It also helps limit the possibility of side effects. (For example, if another test attaches a property to the noop function for some reason, that property will be set for all the other tests.)

But this isn't far off from a tabs-vs-spaces discussion, so I'm happy to just do it if everyone else thinks differently than me. :-D

@lpinca
lpinca approved these changes May 23, 2017
@Trott

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

Landed in 4384f2e

@Trott Trott closed this May 25, 2017

Trott added a commit to Trott/io.js that referenced this pull request May 25, 2017
test: refactor event-emitter-remove-all-listeners
Use common.mustNotCall() to confirm that listener handles never run (as
no events are emitted).

PR-URL: nodejs#13165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell added a commit that referenced this pull request May 28, 2017
test: refactor event-emitter-remove-all-listeners
Use common.mustNotCall() to confirm that listener handles never run (as
no events are emitted).

PR-URL: #13165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.