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, tools: add common.noop and common.mustNotCall lint rule #12027

Closed
wants to merge 2 commits into from

Conversation

@jasnell
Copy link
Member

commented Mar 24, 2017

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

tools/eslint-rules/prefer-common-mustnotcall.js Outdated
@@ -0,0 +1,31 @@
/**
* @fileoverview Prefer common.mustNotCall(msg)

This comment has been minimized.

Copy link
@Trott

Trott Mar 24, 2017

Member

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

This comment has been minimized.

Copy link
@Trott

Trott Mar 24, 2017

Member

(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.)

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 24, 2017

Author Member

Done!

@Fishrock123
Copy link
Member

left a comment

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)

@jasnell jasnell force-pushed the jasnell:common-noop branch Mar 24, 2017

tools/eslint-rules/prefer-common-mustnotcall.js Outdated
if (node.callee.property &&
node.callee.property.name === 'mustCall') {
if (node.arguments.length >= 2) {
if (node.arguments[1].value == 0) {

This comment has been minimized.

Copy link
@gibfahn

gibfahn Mar 25, 2017

Member

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

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 25, 2017

Contributor

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

@gibfahn

This comment has been minimized.

Copy link
Member

commented Mar 25, 2017

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

@not-an-aardvark
Copy link
Member

left a comment

Linter rule LGTM

tools/eslint-rules/prefer-common-mustnotcall.js Outdated
if (node.callee.property &&
node.callee.property.name === 'mustCall') {
if (node.arguments.length >= 2) {
if (node.arguments[1].value == 0) {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 25, 2017

Contributor

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

@jasnell jasnell force-pushed the jasnell:common-noop branch Mar 25, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2017

@cjihrig ... updated! new and improved!

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2017

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?

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2017

Good point lol

@gibfahn

This comment has been minimized.

Copy link
Member

commented Mar 25, 2017

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.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2017

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.

jasnell added 2 commits Mar 24, 2017
test: add common.noop, default for common.mustCall()
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()
Prefer using `common.mustNotCall()` over `common.mustCall(fn, 0)`

@jasnell jasnell force-pushed the jasnell:common-noop branch to 5215766 Mar 26, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

Only CI failure is unrelated.

jasnell added a commit that referenced this pull request Mar 26, 2017
test: add common.noop, default for common.mustCall()
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>
jasnell added a commit that referenced this pull request Mar 26, 2017
tools: add rule prefering common.mustNotCall()
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>
@jasnell

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

Landed in 4f2e372...20b1823

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

This will need to be manually backported to v7.x

@sam-github

This comment has been minimized.

Copy link
Member

commented May 15, 2017

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

@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 17, 2017

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

guide.

@Trott Trott referenced this pull request Jul 17, 2017
3 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

ping @jasnell one more time for backport

@refack refack referenced this pull request Jul 17, 2017
2 of 2 tasks complete
MylesBorins added a commit that referenced this pull request Aug 14, 2017
test: add common.noop, default for common.mustCall()
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>
MylesBorins added a commit that referenced this pull request Aug 14, 2017
tools: add rule prefering common.mustNotCall()
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>
MylesBorins added a commit that referenced this pull request Aug 16, 2017
test: add common.noop, default for common.mustCall()
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>
MylesBorins added a commit that referenced this pull request Aug 16, 2017
tools: add rule prefering common.mustNotCall()
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>
@MylesBorins MylesBorins referenced this pull request Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.