tools: auto fix custom eslint rule for crypto-check.js by shobhitchittora · Pull Request #16647 · nodejs/node · GitHub

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

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

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

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.

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

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

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.

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.

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

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.

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.

done.

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

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?

Done now.

@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.

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

Ping @shobhitchittora

@BridgeAR I'm on it.

@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?

lgtm.

Nit: unnecessary change.

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

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.

@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

Working tests now. Please check.

Done now.

LGTM with the comment addressed and a green CI.

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

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

Seems like there are linting errors.

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

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 <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

1. Fixer for crypto-check.js 2. Extends tests PR-URL: nodejs#16647 Refs: nodejs#16636 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

1. Fixer for crypto-check.js 2. Extends tests PR-URL: #16647 Refs: #16636 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 outstanding review comments, 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

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

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

tools: auto fix custom eslint rule for crypto-check.js by shobhitchittora · Pull Request #16647 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

tools: auto fix custom eslint rule for crypto-check.js#16647shobhitchittora wants to merge 7 commits intonodejs:masternodejs/node:masterfrom shobhitchittora:cryto-check-fixshobhitchittora/node:cryto-check-fixCopy head branch name to clipboard

Conversation

shobhitchittora commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shobhitchittora Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shobhitchittora Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Oct 31, 2017

Uh oh!

shobhitchittora Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Dec 9, 2017

Uh oh!

shobhitchittora commented Dec 10, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shobhitchittora commented Jan 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 20, 2018

Uh oh!

targos commented Jan 20, 2018

Uh oh!

BridgeAR commented Jan 31, 2018

Uh oh!

shobhitchittora commented Jan 31, 2018

Uh oh!

shobhitchittora commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XadillaX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 10, 2018

Uh oh!

shobhitchittora left a comment

Choose a reason for hiding this comment

Uh oh!

tools: auto fix custom eslint rule for crypto-check.js#16647
shobhitchittora wants to merge 7 commits into
nodejs:masterfrom
shobhitchittora:cryto-check-fix

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