test: remove common.noop #12822

Trott · 2017-05-04T04:25:46Z

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.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test

refack · 2017-05-04T04:37:48Z

I don't remember who said that Function.prototype.call is a built in noop for when you need to refer to it by name (removeListener, etc)

TimothyGu · 2017-05-04T06:41:06Z

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

TimothyGu

LGTM if CI is green.

jasnell · 2017-05-04T12:40:07Z

I'm going to abstain on this. I find it useful (which is why I added it :-) ...). I would likely just do it as a revert of the original commit that added it then do a separate commit that adds back in the changes to common.mustCall() defaulting to a no-op.

lpinca · 2017-05-04T16:23:28Z

Most of the problems caused by common.noop can be avoided by paying a little attention when using it so I'm with @jasnell in the sense that I find it useful but I'm also fine with not using it at all :)

benjamingr · 2017-05-04T17:08:33Z

Note that I reviewed the changes and the changes LGTM, but I'm +0 on actually removing common.noop.

Trott · 2017-05-05T20:23:21Z

Rebased to resolve a few conflicts. New CI: https://ci.nodejs.org/job/node-test-pull-request/7907/

Trott · 2017-05-06T06:09:02Z

Obviously, I"m all for this, but there are more neutral comments than supportive comments. If anyone who hasn't weighed in yet has objections, please comment! Thanks! /cc @nodejs/testing

thefourtheye · 2017-05-08T04:06:14Z

-0. I kinda liked the idea of common.noop. If we are repeating something in many files, it is better to put it in a common module. Moreover, if we want to use normal functions instead of arrow functions for noop, for whatever reason, again there will be code churn.

Trott · 2017-05-10T02:16:26Z

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

Trott · 2017-05-10T02:33:37Z

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

Trott · 2017-05-10T21:02:40Z

Rebased. Another CI: https://ci.nodejs.org/job/node-test-pull-request/7998/

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.

Trott · 2017-07-03T05:08:36Z

It's time...

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

Trott · 2017-07-03T18:41:06Z

Landed in 380929e

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: nodejs#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>

addaleax · 2017-07-11T18:22:49Z

@Trott This doesn’t land cleanly on 8.x; can you backport it? (link to the backport guide)

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: nodejs#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>

Trott · 2017-07-11T20:59:09Z

Backport in #14174

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: #12822 Backport-PR-URL: #14174 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins · 2017-08-16T06:48:51Z

Managed to get this to land on v6.x

this common.noop thing has been a trip, just backported it yesterday 😅

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: #12822 Backport-PR-URL: #14174 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>

Trott added the test Issues and PRs related to the tests. label May 4, 2017

nodejs-github-bot added addons Issues and PRs related to native addons. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels May 4, 2017

not-an-aardvark approved these changes May 4, 2017

View reviewed changes

TimothyGu approved these changes May 4, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request May 4, 2017

test: reduce string concatenations #12735

Closed

24 tasks

benjamingr approved these changes May 4, 2017

View reviewed changes

aqrln mentioned this pull request May 4, 2017

test: make common.noop a getter #12732

Closed

4 tasks

lpinca mentioned this pull request May 5, 2017

tls: accept lookup option for tls.connect() #12839

Closed

4 tasks

Trott force-pushed the nononoop branch from bafc989 to 33eba38 Compare May 5, 2017 20:19

gibfahn approved these changes May 6, 2017

View reviewed changes

addaleax approved these changes May 7, 2017

View reviewed changes

Trott force-pushed the nononoop branch from 33eba38 to 7972beb Compare May 10, 2017 02:14

Trott force-pushed the nononoop branch from 7972beb to 6ad4ea3 Compare May 10, 2017 02:33

Trott force-pushed the nononoop branch from 6ad4ea3 to c358923 Compare May 10, 2017 21:01

Trott force-pushed the nononoop branch 3 times, most recently from 37d072e to 7bf77f7 Compare May 17, 2017 04:49

Trott force-pushed the nononoop branch from 91407d8 to 50541c1 Compare July 3, 2017 05:01

Trott force-pushed the nononoop branch from 50541c1 to d797b19 Compare July 3, 2017 05:02

gibfahn approved these changes Jul 3, 2017

View reviewed changes

Trott closed this Jul 3, 2017

addaleax added the backport-requested-v8.x label Jul 11, 2017

Trott mentioned this pull request Jul 11, 2017

[v8.x backport] test: remove common.noop #14174

Closed

2 tasks

addaleax added backported-to-v8.x and removed backport-requested-v8.x labels Jul 12, 2017

addaleax mentioned this pull request Jul 18, 2017

v8.2.0 proposal #13744

Merged

MylesBorins added the land-on-v6.x label Aug 16, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

Trott deleted the nononoop branch January 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: remove common.noop #12822

test: remove common.noop #12822

Trott commented May 4, 2017

refack commented May 4, 2017

TimothyGu commented May 4, 2017

TimothyGu left a comment

jasnell commented May 4, 2017

lpinca commented May 4, 2017

benjamingr commented May 4, 2017

Trott commented May 5, 2017

Trott commented May 6, 2017

thefourtheye commented May 8, 2017

Trott commented May 10, 2017

Trott commented May 10, 2017

Trott commented May 10, 2017

Trott commented Jul 3, 2017

Trott commented Jul 3, 2017

addaleax commented Jul 11, 2017

Trott commented Jul 11, 2017

MylesBorins commented Aug 16, 2017

test: remove common.noop #12822

test: remove common.noop #12822

Conversation

Trott commented May 4, 2017

Checklist

Affected core subsystem(s)

refack commented May 4, 2017

TimothyGu commented May 4, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

jasnell commented May 4, 2017

lpinca commented May 4, 2017

benjamingr commented May 4, 2017

Trott commented May 5, 2017

Trott commented May 6, 2017

thefourtheye commented May 8, 2017

Trott commented May 10, 2017

Trott commented May 10, 2017

Trott commented May 10, 2017

Trott commented Jul 3, 2017

Trott commented Jul 3, 2017

addaleax commented Jul 11, 2017

Trott commented Jul 11, 2017

MylesBorins commented Aug 16, 2017