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

tools: custom eslint autofix for inspector-check.js #16646

Closed

Conversation

shobhitchittora
Copy link
Contributor

@shobhitchittora shobhitchittora commented Oct 31, 2017

This adds eslint fixer method for inspector-check.js. When running eslint with --fix flag, the fixer will add common.skipIfInspectorDisabled(); to the file automatically.

Refs : #16636

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • Added/Updated the test cases.
  • Followed commit guidelines.
  • python ./tools/test.py parallel/test-eslint-inspector-check
Affected core subsystem(s)

Tools

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 31, 2017
@shobhitchittora shobhitchittora changed the title eslint autofix for inspector-check tools: eslint autofix for inspector-check Oct 31, 2017
@shobhitchittora shobhitchittora changed the title tools: eslint autofix for inspector-check tools: eslint autofix for inspector-check.js Oct 31, 2017
@shobhitchittora shobhitchittora changed the title tools: eslint autofix for inspector-check.js tools: custom eslint autofix for inspector-check.js Oct 31, 2017
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in the other PR re: creating tests for these to verify they're working as expected. Thanks!

fix: (fixer) => {
return fixer.insertTextAfter(
node,
'\n\n common.skipIfInspectorDisabled();'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the node refer to in this context? I feel like this might be getting inserted in the wrong place.

Also, why the errant space after \n\n?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. This inserts the correction at the end of the file. Working on inserting it below require('../common'). Also will fix space after\n\n.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a new commit fixing the same. Please verify. Thanks.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BridgeAR
Copy link
Member

This needs a rebase.

@shobhitchittora
Copy link
Contributor Author

@BridgeAR Rebased with mater.

@addaleax
Copy link
Member

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 30, 2017
@addaleax
Copy link
Member

addaleax commented Dec 1, 2017

@shobhitchittora Could you run make test on this PR? It looks like the test for this is failing here…

@apapirovski
Copy link
Member

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.

@shobhitchittora
Copy link
Contributor Author

@addaleax I'm getting 68 errors while running make -j4 test on the latest master. Getting errors while Running Markdown linter on docs....

@BridgeAR
Copy link
Member

BridgeAR commented Jan 5, 2018

@nodejs/collaborators PTAL

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments.

var hasInspectorCheck = false;

function testInspectorUsage(context, node) {
if (utils.isRequired(node, ['inspector'])) {
missingCheckNodes.push(node);
}

if (utils.isCommonModule(node)) {
commonModuleNodes.push(node);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is only one required (and only one should be present) using an array here is actually not necessary and a simple assign would be enough as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup correct. Fixing this.

@@ -11,16 +11,21 @@ const utils = require('./rules-utils.js');
// Rule Definition
//------------------------------------------------------------------------------
const msg = 'Please add a skipIfInspectorDisabled() call to allow this ' +
'test to be skippped when Node is built \'--without-inspector\'.';
'test to be skippped when Node is built \'--without-inspector\'.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Unrelated change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

expression.callee &&
(expression.callee.name === 'skip' ||
expression.callee.property &&
expression.callee.property.name === 'skip');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: unrelated change. The same applies to the part above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

'common.skipIfInspectorDisabled(); require("inspector");',
`require("common");
common.skipIfInspectorDisabled();
require("inspector");`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We normally do not use multiline template strings. It will work in this case but it was very hard for me to even see that it is a template string and not three individual entries. So it would be great to change that to either a single line or +.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

errors: [{ message }],
output: 'require("common");\n' +
'common.skipIfInspectorDisabled();\n' +
'require("inspector");'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shobhitchittora is this definitely going to work and will it also insert require("common"); here? Because I do not see this happening in the fixer.

@BridgeAR
Copy link
Member

Mini-CI (enough to test this): https://ci.nodejs.org/job/node-test-commit-light/146/

@BridgeAR
Copy link
Member

not ok 460 parallel/test-eslint-inspector-check
  ---
  duration_ms: 0.330
  severity: fail
  stack: |-
    /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:148
            throw err;
            ^
    
    TypeError: Cannot read property 'range' of undefined

@BridgeAR
Copy link
Member

Ping @shobhitchittora

@shobhitchittora
Copy link
Contributor Author

@BridgeAR I'm on it.

Copy link
Contributor Author

@shobhitchittora shobhitchittora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the said changes.

@shobhitchittora
Copy link
Contributor Author

Same error here -

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

AssertionError [ERR_ASSERTION]: Output is incorrect.

@starkwang How did you check the diffs for the test runner?

@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

@shobhitchittora I am not sure what your question is really about. Can you elaborate so someone else might also be able to answer that question?

@shobhitchittora
Copy link
Contributor Author

@BridgeAR I'm stuck as the test for the rule is failing, throwing -

AssertionError [ERR_ASSERTION]: Output is incorrect.

While running - python ./tools/test.py parallel/test-eslint-inspector-check I'd like the test-runner to show me the expected and actual output received, i.e, the diff ( similar to what @starkwang did for a previous PR of ours).

PS: I tried adding/removing 'require("common");\n' + from the output of the test, but it's not working.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the comments and I recommend to rebase. That way you get a better error output. Please also add a new test case that checks code that imported common.

errors: [{ message }],
output: 'require("common");\n' +
'common.skipIfInspectorDisabled();\n' +
'require("inspector");'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very certain not the correct output as common was not imported in the checked code.

'common.skipIfInspectorDisabled(); require("inspector");',
'require("common");\n' +
'common.skipIfInspectorDisabled();\n' +
'require("inspector");'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either use a single code line (I would do that as in the example above the added one) or indent the code so the concatenated part is deeper indented than the start of the string.

1. Adds fixer method
2. Extends test
3. Fixes merge issue

Refs: nodejs#16636
1. Removes extra indentation
2. Removes array to track commomModule AST node.
3. Refactored test

Refs: nodejs#16636
Review comments + updates tests to work with fixer

Refs: nodejs#16636
@shobhitchittora
Copy link
Contributor Author

@BridgeAR Hope this does it. Please run the CI on this.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the access to the first argument has to be guarded. In case a node has no arguments, this will trigger an error.

So please add a node.arguments.length !== 0 && in the middle.

'common.skipIfInspectorDisabled(); require("inspector");'
'require("common")\n' +
'common.skipIfInspectorDisabled();\n' +
'require("inspector")'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please indent the last two lines by two spaces? :-)

rules-utils arguments check + spacing in test file

Refs: nodejs#16636
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green

@BridgeAR
Copy link
Member

@BridgeAR 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 pushed a commit to BridgeAR/node that referenced this pull request Feb 17, 2018
1. Adds fixer method
2. Extends test

PR-URL: nodejs#16646
Refs: nodejs#16636
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member

Landed in 5156342 🎉

Due to the other rule landing earlier there was a conflict that I resolved while landing.

@BridgeAR BridgeAR closed this Feb 17, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
1. Adds fixer method
2. Extends test

PR-URL: #16646
Refs: #16636
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
1. Adds fixer method
2. Extends test

PR-URL: #16646
Refs: #16636
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
1. Adds fixer method
2. Extends test

PR-URL: #16646
Refs: #16636
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
1. Adds fixer method
2. Extends test

PR-URL: nodejs#16646
Refs: nodejs#16636
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
1. Adds fixer method
2. Extends test

PR-URL: #16646
Refs: #16636
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
1. Adds fixer method
2. Extends test

PR-URL: #16646
Refs: #16636
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants