tools: auto fix custom eslint rule for crypto-check.js #16647

shobhitchittora · 2017-10-31T19:40:44Z

This implements an eslint fixer function to auto insert common.hasCryto check if missed out.

Refs: #16636

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines
test cases added for changes
python ./tools/test.py parallel/test-eslint-crypto-check

Affected core subsystem(s)

Tools

apapirovski

Thanks for starting the work! Left some minor feedback and please see the other PR re: the need for tests.

apapirovski · 2017-10-31T20:21:46Z

tools/eslint-rules/crypto-check.js

+        fix: (fixer) => {
+          return fixer.insertTextAfter(
+            node,
+            '\n\nif (!common.hasCrypto)\n\tcommon.skip("missing crypto");'


What does the node refer to in this context? This should be getting inserted right after common is required rather than somewhere random.

Also, the project doesn't use tabs but rather spaces so \t should be a double space.

Thanks for the comments. Working on commit to fix.

I've pushed the changes. Also please note that I've added a regExp in rules-utils.js, please verify the validity of the same, or suggest if there are any better ways of doing that.

The reason for doing this was because the common module was addressed with different relative paths, such as ../../common or ../common.

apapirovski · 2017-10-31T23:28:26Z

Thanks for the updates! I'll be happy to review again once we have the tests for all of these fixers, just so we can be sure that they're working properly (it's a bit hard to tell without actually running the code).

shobhitchittora · 2017-11-01T09:16:06Z

tools/eslint-rules/crypto-check.js

+        message: msg,
+        fix: (fixer) => {
+          return fixer.insertTextAfter(
+            commonModuleNode,


Need help here @apapirovski . commonModuleNode comes as null here when running make -j4 test. I'm setting commonModuleNode in testCryptoUsage method.

PS: This works fine when running eslint with --fix to fix a generated error.

Ping @apapirovski

tniessen · 2017-11-01T11:23:22Z

tools/eslint-rules/rules-utils.js

@@ -13,6 +13,16 @@ module.exports.isRequired = function(node, modules) {
 };

 /**
+ * Return true if common module is present as a require call
+ * in AST
+ */


This description is not entirely accurate. The given node must be a call to require, it does not work for nodes which contain such a call.

@tniessen Please check now. Suggest if any corrections.

apapirovski · 2017-12-09T21:01:31Z

Much like with the other PR:

I won't outright block this but I'm -1 on landing this. I really don't like the idea of auto fixing these skip checks because of the potential for things going really wrong. The fact that it has to lookup an entirely unrelated part of the code to insert the check is concerning. There are so many different possibilities for what a require that passes that RegExp could actually be requiring.

Sorry @shobhitchittora.

shobhitchittora · 2017-12-10T06:12:14Z

@BridgeAR What are your views on landing this?
@apapirovski I can relate to your comment here. +1

BridgeAR

I personally do not see as much concern as @apapirovski here as these rules are only active for our tests and we should realize if something goes wrong. The latest in the code review.

BridgeAR · 2017-12-10T06:16:38Z

tools/eslint-rules/rules-utils.js

+* in AST Node under inspection
+*/
+module.exports.isCommonModule = function(node) {
+  var regExp = new RegExp(/^(\.\.\/)*common(\.js)?$/);


Why do you not write the RegExp directly? There is no dynamic part as far as I see it.
So /^(\.\.\/)*common(\.js)?$/ does the job and can be moved outside of the function. That way it is only created once.

shobhitchittora · 2018-01-20T07:47:43Z

@BridgeAR would you be so kind and run CI for this too?

targos · 2018-01-20T09:00:40Z

test/parallel/test-eslint-crypto-check.js

-      }
-      require('crypto');
+    if (!common.hasCrypto) {
+      common.skip();


Does this (and other occurences below) pass the test? I think the 'missing crypto' message is missing.

Added the string 'missing crypto' in tests. But there's still something wrong. Any help?

BridgeAR · 2018-01-20T09:04:51Z

@shobhitchittora this has similar issues as the other PRs. Please run the tests by calling make -j4 test. You can also run the individual test by running python ./tools/test.py parallel/test-eslint-crypto-check.

targos · 2018-01-20T09:06:40Z

You can also run the linter only with make lint-js

BridgeAR · 2018-01-31T02:04:47Z

Ping @shobhitchittora

shobhitchittora · 2018-01-31T02:44:36Z

@BridgeAR I'm on it.

shobhitchittora · 2018-02-04T05:06:57Z

@BridgeAR I cannot spot the diff when running python ./tools/test.py parallel/test-eslint-crypto-check fails. Just getting this -

=== release test-eslint-crypto-check ===
Path: parallel/test-eslint-crypto-check
assert.js:43
  throw new errors.AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Output is incorrect.
[00:00|% 100|+   0|-   1]: Done

Any suggestions? Can you spot the error?

XadillaX

lgtm.

BridgeAR · 2018-02-10T17:35:15Z

test/parallel/test-eslint-crypto-check.js

+    `if (!common.hasCrypto) {
+      common.skip();
+    }
+    require("crypto");
    `


Nit: unnecessary change.

BridgeAR · 2018-02-10T17:35:57Z

test/parallel/test-eslint-crypto-check.js

+      `if (!common.hasCrypto) {
+        common.skip("missing crypto");
+      }
+      require("crypto");


This can not be the proper output because there is no common imported in the code.

BridgeAR · 2018-02-10T17:36:18Z

test/parallel/test-eslint-crypto-check.js

+      `if (!common.hasCrypto) {
+        common.skip("missing crypto");
+      }
+      require("crypto");


This can not be the expected output because common is not imported. But not only that, the first statement if (common.foo) {} was just removed? That is definitely not right.

BridgeAR · 2018-02-10T17:39:21Z

@shobhitchittora please rebase and run the code again. In that case the output is going to show what check failed. Please carefully check what change you expect and what not with what input.

Please add another test with a case where common is actually imported in the checked code part.

1. Fixer for crypto-check.js 2. extends tests Refs : nodejs#16636

Removes unwanted indentation Refs : nodejs#16636

Adds check for fixing on when require('common') is present. Refs : nodejs#16636

Adds "missing crypto" message in tests. Refs : nodejs#16636

1. Refactors test 2. Removes commonModule AST node as array. Refs : nodejs#16636

Review comments + updates test cases Refs : nodejs#16636

shobhitchittora

Working tests now. Please check.

shobhitchittora · 2018-02-15T10:10:14Z

test/parallel/test-eslint-crypto-check.js

-      }
-      require('crypto');
+    if (!common.hasCrypto) {
+      common.skip();


BridgeAR

LGTM with the comment addressed and a green CI.

BridgeAR · 2018-02-17T01:24:14Z

tools/eslint-rules/rules-utils.js

+var commonModuleRegExp = new RegExp(/^(\.\.\/)*common(\.js)?$/);
+module.exports.isCommonModule = function(node) {
+  return node.callee.name === 'require' &&
+    commonModuleRegExp.test(node.arguments[0].value);


This will produce access errors due to accessing nodes without arguments. Please guard against them the same as I suggested in the other PR.

BridgeAR · 2018-02-17T01:27:36Z

Light-CI https://ci.nodejs.org/job/node-test-commit-light/284/

BridgeAR · 2018-02-17T04:30:56Z

Seems like there are linting errors.

Removes extra spaces ( lint-error ) + rules-utils argument check suggestoin Refs : nodejs#16636

BridgeAR · 2018-02-17T05:11:48Z

New CI https://ci.nodejs.org/job/node-test-commit-light/285/

1. Fixer for crypto-check.js 2. Extends tests PR-URL: nodejs#16647 Refs: nodejs#16636 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

BridgeAR · 2018-02-17T16:12:29Z

Landed in f242d4b 🎉

@shobhitchittora thanks for being patient :-)

1. Fixer for crypto-check.js 2. Extends tests PR-URL: #16647 Refs: #16636 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

1. Fixer for crypto-check.js 2. Extends tests PR-URL: nodejs#16647 Refs: nodejs#16636 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

1. Fixer for crypto-check.js 2. Extends tests PR-URL: #16647 Refs: #16636 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 31, 2017

apapirovski reviewed Oct 31, 2017

View reviewed changes

shobhitchittora changed the title ~~tools: auto fix custom eslint rule for cryto-check.js~~ tools: auto fix custom eslint rule for crypto-check.js Oct 31, 2017

shobhitchittora commented Nov 1, 2017

View reviewed changes

tniessen reviewed Nov 1, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

BridgeAR reviewed Dec 10, 2017

View reviewed changes

shobhitchittora force-pushed the cryto-check-fix branch 2 times, most recently from d607020 to d8d5713 Compare December 10, 2017 12:44

targos reviewed Jan 20, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

jasnell approved these changes Jan 31, 2018

View reviewed changes

XadillaX approved these changes Feb 4, 2018

View reviewed changes

BridgeAR requested changes Feb 10, 2018

View reviewed changes

shobhitchittora added 5 commits February 12, 2018 10:47

tools: auto fix custom eslint rule for crypto-check.js

b35d423

1. Fixer for crypto-check.js 2. extends tests Refs : nodejs#16636

tools: auto fix custom eslint rule for crypto-check.js

803357b

Removes unwanted indentation Refs : nodejs#16636

tools: auto fix custom eslint rule for crypto-check.js

6d6cdc6

Adds check for fixing on when require('common') is present. Refs : nodejs#16636

tools: auto fix custom eslint rule for crypto-check.js

1f52446

Adds "missing crypto" message in tests. Refs : nodejs#16636

tools: auto fix custom eslint rule for crypto-check.js

2ecf517

1. Refactors test 2. Removes commonModule AST node as array. Refs : nodejs#16636

shobhitchittora force-pushed the cryto-check-fix branch from 94c7b1e to 2ecf517 Compare February 12, 2018 05:32

tools: auto fix custom eslint rule for crypto-check.js

1018b5b

Review comments + updates test cases Refs : nodejs#16636

shobhitchittora force-pushed the cryto-check-fix branch from d6d6c26 to 1018b5b Compare February 15, 2018 10:10

shobhitchittora commented Feb 15, 2018

View reviewed changes

BridgeAR approved these changes Feb 17, 2018

View reviewed changes

tools: auto fix custom eslint rule for crypto-check.js

46206b7

Removes extra spaces ( lint-error ) + rules-utils argument check suggestoin Refs : nodejs#16636

shobhitchittora force-pushed the cryto-check-fix branch from f4c1193 to 46206b7 Compare February 17, 2018 05:01

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018

BridgeAR closed this Feb 17, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added the land-on-v8.x label Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

tools: auto fix custom eslint rule for crypto-check.js #16647

tools: auto fix custom eslint rule for crypto-check.js #16647

Conversation

shobhitchittora commented Oct 31, 2017 • edited Loading

Checklist

Affected core subsystem(s)

apapirovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shobhitchittora Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

shobhitchittora Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski commented Oct 31, 2017

shobhitchittora Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Dec 9, 2017

shobhitchittora commented Dec 10, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shobhitchittora commented Jan 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 20, 2018

targos commented Jan 20, 2018

BridgeAR commented Jan 31, 2018

shobhitchittora commented Jan 31, 2018

shobhitchittora commented Feb 4, 2018 • edited Loading

XadillaX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Feb 10, 2018

shobhitchittora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

shobhitchittora commented Oct 31, 2017 •

edited

Loading

shobhitchittora Oct 31, 2017 •

edited

Loading

shobhitchittora Oct 31, 2017 •

edited

Loading

shobhitchittora Nov 1, 2017 •

edited

Loading

shobhitchittora commented Feb 4, 2018 •

edited

Loading