This change removes `common.noop` from the Node.js internal testing
common module.
Over the last few weeks, I've grown to dislike the `common.noop`
abstraction.
First, new (and experienced) contributors are unaware of it and so it
results in a large number of low-value nits on PRs. It also increases
the number of things newcomers and infrequent contributors have to be
aware of to be effective on the project.
Second, it is confusing. Is it a singleton/property or a getter? Which
should be expected? This can lead to subtle and hard-to-find bugs. (To
my knowledge, none have landed on master. But I also think it's only a
matter of time.)
Third, the abstraction is low-value in my opinion. What does it really
get us? A case could me made that it is without value at all.
Lastly, and this is minor, but the abstraction is wordier than not using
the abstraction. `common.noop` doesn't save anything over `() => {}`.
So, I propose removing it.
PR-URL: https://github.com/nodejs/node/pull/12822
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
common.fail() was added to paste over issues with assert.fail() function
signature. assert.fail() has been updated to accept a single argument so
common.fail() is no longer necessary.
PR-URL: https://github.com/nodejs/node/pull/12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Use the remaining listener directly if the array of listeners has only
one element after running `EventEmitter.prototype.removeListener()`.
Advantages:
- Better memory usage and better performance if no new listeners are
added for the same event.
Disadvantages:
- A new array must be created if new listeners are added for the same
event.
PR-URL: https://github.com/nodejs/node/pull/12043
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Export a new common.noop no-operation function for general use.
Allow using common.mustCall() without a fn argument to simplify
test cases.
Replace various non-op functions throughout tests with common.noop
PR-URL: https://github.com/nodejs/node/pull/12027
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
This commit adds a mustNotCall() helper for testing. This provides
an alternative to using common.fail() as a callback, or creating
a callback function for the sole purpose of calling common.fail().
PR-URL: https://github.com/nodejs/node/pull/11152
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
PR-URL: https://github.com/nodejs/node/pull/11140
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
with no events at all
* test calling removeListener() passing a event type that does
not exist
* test calling eventNames() in a event emitter instance
with no events at all
Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
PR-URL: https://github.com/nodejs/node/pull/10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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: https://github.com/nodejs/node/pull/8168
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit splits several large event emitter tests into smaller
block scoped tests. This keeps the tests more isolated. This
commit also replaces var with const, assert.equal() with
assert.stictEqual(), and other small enhancements.
PR-URL: https://github.com/nodejs/node/pull/6446
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
When removing a `once` listener, the listener being passed to
the `removeListener` callback is the wrapper. This unwraps the
listener so that `removeListener` is passed the actual listener.
PR-URL: https://github.com/nodejs/node/pull/6394
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
In preparation for a lint rule that will enforce
assert.deepStrictEqual() over assert.deepEqual(), change tests and
benchmarks accordingly. For tests and benchmarks that are testing or
benchmarking assert.deepEqual() itself, apply a comment to ignore the
upcoming rule.
PR-URL: https://github.com/nodejs/node/pull/6213
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This commit updates events doc to describe removeListener behaviour
when it is called within a listener. An example is added to make
it more evident.
A test is also incuded to make this behaviour consistent in future
releases.
Fixes: nodejs/node#4759
PR-URL: https://github.com/nodejs/node/pull/5201
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Remove all remaining unused variables from tests in test/parallel.
PR-URL: https://github.com/nodejs/node/pull/4511
Reviewed-By: James M Snell<jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Currently there are many instances where assert.fail is directly passed
to a callback for error handling. Unfortunately this will swallow the
error as it is the third argument of assert.fail that sets the message
not the first.
This commit adds a new function to test/common.js that simply wraps
assert.fail and calls it with the provided message.
Tip of the hat to @trott for pointing me in the direction of this.
PR-URL: https://github.com/nodejs/node/pull/3453
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Enable linting for the test directory. A number of changes was made so
all tests conform the current rules used by lib and src directories. The
only exception for tests is that unreachable (dead) code is allowed.
test-fs-non-number-arguments-throw had to be excluded from the changes
because of a weird issue on Windows CI.
PR-URL: https://github.com/nodejs/io.js/pull/1721
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The order of the `newListener` and `removeListener` events with respect
to the actual adding and removing from the underlying listeners array
should be deterministic. There is no compelling reason for leaving it
indeterminate. Changing the ordering is likely to result in breaking
code that was unwittingly relying on the current behaviour, and the
indeterminancy makes it impossible to use these events to determine when
the first or last listener is added for an event.
PR-URL: https://github.com/iojs/io.js/pull/687
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The copyright and license notice is already in the LICENSE file. There
is no justifiable reason to also require that it be included in every
file, since the individual files are not individually distributed except
as part of the entire package.
This reverts commit 928ea564d1.
Keeping the original Array instance in-place essentially causes a memory leak
on EventEmitters that use an infinite number of event names (an incrementing
counter, for example), which isn't an unreasonable thing to want to do.
Fixes#3702.
Rich Trott
Luigi Pinca
James M Snell
cjihrig
matsuda-koushi
Vinícius do Carmo
DavidCai
Vaibhav
Myles Borins
Roman Reiss
Sam Roberts
isaacs
Fedor Indutny
Ben Noordhuis
Nathan Rajlich
Ryan Dahl
Oleg Efimov
Jonas Pfenniger