test, tools: add common.noop and common.mustNotCall lint rule by jasnell · Pull Request #12027 · nodejs/node · GitHub

Throughout the tests, creation of non-op functions is fairly extensive, and it is not uncommon to find common.mustCall(() => {}) or common.mustCall(function() {}), or noop = () => {}; type declarations throughout.

This PR introduces a common.noop non-op function that is used as an alternative to redeclaring nonops all the time.

The common.mustCall() method is also modified such that the fn argument defaults to common.noop if undefined, making it unnecessary to pass in a function when a nonop is needed.

There were also a couple of places where common.mustCall(fn, 0) was used to identify a function that should not be called. These are replace with common.mustNotCall() and a lint rule is added.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test, tools

Nit: Indicate what you are preferring it over. Maybe Prefer common.mustNotCall(msg) over common.mustCall(fn, 0) or whatever.

(Ah, I see it's also in the message supplied to the user. Might not be terrible to put it in the comment overview too, though.)

Done!

Seems good to me.

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

(LGTM but I'd like more eyes, large changes XD)

Are you using nested ifs instead of lots of && because it looks cleaner?

Could you at least break these out into one or more helper functions (isCommonMethod(), isMustCall())?

cc/ @not-an-aardvark, @silverwind, @targos for the eslint side

Linter rule LGTM

Could you at least break these out into one or more helper functions (isCommonMethod(), isMustCall())?

@cjihrig ... updated! new and improved!

Beautiful. One thing I just noticed - do you think it's worth adjusting the argument checking in the lint rule, since common.mustCall(0) would be possible now too?

Good point lol

common.mustCall(0) would be possible now too?

Out of interest, is there a reason to allow common.mustCall(0)? If the linter tells you to replace that with common.mustCall() that makes sense to me.

The point is that common.mustCall(0) should be caught by the lint rule. It should be replaced with common.mustNotCall(). common.mustCall(0) was not a valid use of mustCall() prior to this PR.

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

Prefer using `common.mustNotCall()` over `common.mustCall(fn, 0)`

@cjihrig ... updated to catch common.mustCall(0)

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

The rule LGTM. I didn't re-review everything else. I'm assuming that part didn't change.

Only CI failure is unrelated.

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

Prefer using `common.mustNotCall()` over `common.mustCall(fn, 0)` PR-URL: #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>

Landed in 4f2e372...20b1823

This will need to be manually backported to v7.x

@jasnell this needs backporting, because its absence blocks #11876 (and will probably block more tests from backporting)

@jasnell would you mind backporting to v6.x-staging?

guide.

ping @jasnell one more time for backport

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

Prefer using `common.mustNotCall()` over `common.mustCall(fn, 0)` PR-URL: #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>

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

Prefer using `common.mustNotCall()` over `common.mustCall(fn, 0)` PR-URL: #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>

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 24, 2017

Trott reviewed Mar 24, 2017

View reviewed changes

Fishrock123 approved these changes Mar 24, 2017

View reviewed changes

jasnell force-pushed the common-noop branch from f375496 to f4c70c4 Compare March 24, 2017 22:07

richardlau approved these changes Mar 25, 2017

View reviewed changes

gibfahn reviewed Mar 25, 2017

View reviewed changes

gibfahn approved these changes Mar 25, 2017

View reviewed changes

not-an-aardvark approved these changes Mar 25, 2017

View reviewed changes

cjihrig approved these changes Mar 25, 2017

View reviewed changes

jasnell force-pushed the common-noop branch from f4c70c4 to e9a3c84 Compare March 25, 2017 01:59

fhinkel approved these changes Mar 26, 2017

View reviewed changes

jasnell added 2 commits March 26, 2017 12:07

test: add common.noop, default for common.mustCall()

78afc49

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

tools: add rule prefering common.mustNotCall()

5215766

Prefer using `common.mustNotCall()` over `common.mustCall(fn, 0)`

jasnell force-pushed the common-noop branch from e9a3c84 to 5215766 Compare March 26, 2017 19:09

jasnell closed this Mar 26, 2017

MylesBorins added the backport-to-v7.x label Mar 28, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

evanlucas mentioned this pull request Apr 25, 2017

stream: Fixes missing 'unpipe' event for pipes made with {end: false} #11876

Closed

3 tasks

gibfahn removed the backport-requested-v7.x label Jun 14, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the backport-requested-v6.x label Jun 17, 2017

Trott mentioned this pull request Jul 17, 2017

timers: do not use user object call/apply #12960

Closed

3 tasks

refack mentioned this pull request Jul 17, 2017

test: check number of message events #13125

Closed

2 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Aug 14, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

test, tools: add common.noop and common.mustNotCall lint rule by jasnell · Pull Request #12027 · nodejs/node · GitHub

Uh oh!

Uh oh!

test, tools: add common.noop and common.mustNotCall lint rule - #12027

test, tools: add common.noop and common.mustNotCall lint rule#12027jasnell wants to merge 2 commits intonodejs:masternodejs/node:masterfrom jasnell:common-noopjasnell/node:common-noopCopy head branch name to clipboard

Conversation

jasnell commented Mar 24, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

gibfahn Mar 25, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 25, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Mar 25, 2017

Uh oh!

not-an-aardvark left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 25, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 25, 2017

Uh oh!

cjihrig commented Mar 25, 2017

Uh oh!

jasnell commented Mar 25, 2017

Uh oh!

gibfahn commented Mar 25, 2017

Uh oh!

cjihrig commented Mar 25, 2017

Uh oh!

jasnell commented Mar 26, 2017

Uh oh!

jasnell commented Mar 26, 2017

Uh oh!

cjihrig commented Mar 26, 2017

Uh oh!

jasnell commented Mar 26, 2017

Uh oh!

jasnell commented Mar 26, 2017

Uh oh!

MylesBorins commented Mar 28, 2017

Uh oh!

sam-github commented May 15, 2017

Uh oh!

gibfahn commented Jun 17, 2017

Uh oh!

MylesBorins commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

test, tools: add common.noop and common.mustNotCall lint rule#12027
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:common-noop