tools: add eslint check for skipIfEslintMissing #20372

richardlau · 2018-04-27T20:02:26Z

Add a custom eslint rule to check for common.skipIfEslintMissing() to
allow tests to run from source tarballs that do not include eslint.

Fix up tests that were failing the new check.

Refs: #20336 (comment)

Checklist

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

addaleax · 2018-04-27T20:04:32Z

tools/eslint-rules/eslint-check.js

+// Rule Definition
+//------------------------------------------------------------------------------
+const msg = 'Please add a skipIfEslintMissing() call to allow this test to ' +
+            'be skippped when Node.js is built from a source tarball.';


typo: skippped?

I based this on the equivalent inspector-check and didn't notice it was wrong there. I'll correct both.

richardlau · 2018-04-27T20:28:16Z

Fun. So we have a failing eslint rule test, e.g., https://ci.nodejs.org/job/node-test-commit-plinux/nodes=ppcle-ubuntu1404/17266/console

not ok 448 parallel/test-eslint-require-buffer
  ---
  duration_ms: 2.34
  severity: fail
  exitcode: 1
  stack: |-
    /home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:148
            throw err;
            ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 'Use const { Buffer } = require(\'buffer\'); at the beginning of this file'
    + 'Use const Buffer = require(\'buffer\').Buffer; at the beginning of this file' ('Use const { Buffer } = require(\'buffer\'); at the beginning of this file' strictEqual 'Use const Buffer = require(\'buffer\').Buffer; at the beginning of this file')
        at assertMessageMatches (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:442:24)
        at testInvalidTemplate (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:494:29)
        at Function.RuleTester.it (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:581:25)
        at Function.itDefaultHandler (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:143:23)
        at test.invalid.forEach.invalid (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:580:32)
        at Array.forEach (<anonymous>)
        at Function.RuleTester.describe (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:579:30)
        at Function.describeDefaultHandler (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:160:19)
        at Function.RuleTester.describe (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:578:24)
        at Function.describeDefaultHandler (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/tools/node_modules/eslint/lib/testers/rule-tester.js:160:19)
  ...

This PR doesn't touch that test, nor the eslint check that it exercises but it does fix common.skipIfEslintMissing() so this test probably failed before but was being skipped over as eslint wasn't detected 😱.

richardlau · 2018-04-27T20:38:07Z

#19701 changed the message but not the corresponding test.

richardlau · 2018-04-27T20:48:12Z

Fixed up the test-eslint-require-buffer test. Aborted the failing in progress CI and rebuilding: https://ci.nodejs.org/job/node-test-pull-request/14555/

refack · 2018-04-27T20:52:07Z

We have a chicken-egg problem. The CI machines don't install eslint, so these tests are never run. On the other hand this is meta-meta-meta-testing (writing a test to verify a test to the linter is properly skipped 😵) and might be overkill? Why not just move the test-eslint-* files to their own suite and only run that in special circumstancess (maybe only on the linter CI)

richardlau · 2018-04-27T20:59:08Z

We have a chicken-egg problem. The CI machines don't install eslint, so these tests are never run.

The CI machines don't need to install eslint as it's already in the source tree from a git checkout. It's only when building from the source tarball that eslint is missing. The tests weren't being run because the common.skipIfEslintMissing check was looking in the wrong place.

richardlau · 2018-04-27T21:11:13Z

Why not just move the test-eslint-* files to their own suite and only run that in special circumstancess (maybe only on the linter CI)

I'm not opposed to this idea (if someone else does the work* 😛) since these tests are testing our custom eslint rules and not testing Node.js itself.

* It's now 10pm here -- I started working on this three hours ago thinking this would be a quick fix (inserting the missing common.skipIfEslintMissing calls) then thought I'd implement a custom eslint rule to catch regressions. Then spent an hour trying to work out why the new rule test didn't fail (hint you need to clear the eslint cache before any new rules are picked up). And then the test-eslint-require-buffer failure.

refack · 2018-04-27T21:14:53Z

The CI machines don't need to install eslint as it's already in the source tree from a git checkout. It's only when building from the source tarball that eslint is missing. The tests weren't being run because the common.skipIfEslintMissing check was looking in the wrong place.

Ohh right. So it makes me think about wrong assumptions... the skip rule should look for positive indicator (e.g. existence of .git directory) instead of a negative indicator, so that if things change it'll detect it and not silintly skip (i.e. fail)

refack · 2018-04-27T21:17:02Z

It's now 10pm here ...

I'll see what I can come up with...

richardlau · 2018-04-27T21:25:22Z

Ohh right. So it makes me think about wrong assumptions... the skip rule should look for positive indicator (e.g. existence of .git directory) instead of a negative indicator, so that if things change it'll detect it and not silintly skip (i.e. fail)

It checks for the presence of the eslint directory:

node/test/common/index.js

Lines 495 to 501 in a4b4854

    
           exports.skipIfEslintMissing = function() { 
        
             if (!exports.fileExists( 
        
               path.join('..', '..', 'tools', 'node_modules', 'eslint') 
        
             )) { 
        
               exports.skip('missing ESLint'); 
        
             } 
        
           };

(Note that at the moment it's looking in a relative directory and hence incorrect (fixed as part of this PR)).

refack · 2018-04-27T22:42:55Z

The rule is skip if !exist that's a double negative, not a positive

node/test/common/index.js

Line 496 in a4b4854

if (!exports.fileExists(

So it's error prone — if path calculation is wrong, skip. If we tree-factor and moved the dir (again), skip. If we finish rolling-up eslint into a single file, skip 🤷‍♂️ .

richardlau · 2018-04-27T23:39:15Z

cc @nodejs/linting

richardlau · 2018-04-28T05:18:49Z

arm CI rerun passes: https://ci.nodejs.org/job/node-test-commit-arm-fanned/829/

richardlau · 2018-05-04T14:14:13Z

I didn't label this author-ready due to concerns raised by @refack. While I agree that the skipIfEslintMissing check is fragile if the eslint directory changes, this PR is no less fragile than before and would actually fix the rules tests so that they are run on the CI plus improve the situation for Debian (#20336) and anyone else building from the source tarball.

I'm on a tablet at the moment so can't land this right now, but unless there are actual objections I'll land early next week (unless someone else lands first).

Add a custom eslint rule to check for `common.skipIfEslintMissing()` to allow tests to run from source tarballs that do not include eslint. Fix up rule tests that were failing the new check. Refs: nodejs#20336 PR-URL: nodejs#20372 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

richardlau · 2018-05-08T14:47:31Z

Landed in 870ae72

Add a custom eslint rule to check for `common.skipIfEslintMissing()` to allow tests to run from source tarballs that do not include eslint. Fix up rule tests that were failing the new check. Refs: #20336 PR-URL: #20372 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Apr 27, 2018

addaleax approved these changes Apr 27, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

richardlau mentioned this pull request Apr 27, 2018

mips failures for parallel/test-buffer-write(float|double) #20336

Closed

Trott approved these changes Apr 28, 2018

View reviewed changes

cjihrig approved these changes May 3, 2018

View reviewed changes

jasnell approved these changes May 3, 2018

View reviewed changes

richardlau force-pushed the eslintcheck branch from 4be9ad2 to 870ae72 Compare May 8, 2018 14:45

richardlau merged commit 870ae72 into nodejs:master May 8, 2018

richardlau deleted the eslintcheck branch May 8, 2018 14:48

MylesBorins mentioned this pull request May 8, 2018

v10.1.0 proposal #20606

Merged

richardlau mentioned this pull request Jun 10, 2018

[v9.x backport] assert: fix diff color output #21154

Closed

4 tasks

richardlau mentioned this pull request Nov 23, 2018

make test error - Error: Cannot find module '../../tools/eslint' #24579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: add eslint check for skipIfEslintMissing #20372

tools: add eslint check for skipIfEslintMissing #20372

richardlau commented Apr 27, 2018 •

edited

Loading

addaleax Apr 27, 2018

richardlau Apr 27, 2018

This comment has been minimized.

richardlau commented Apr 27, 2018

richardlau commented Apr 27, 2018

richardlau commented Apr 27, 2018

refack commented Apr 27, 2018

richardlau commented Apr 27, 2018

richardlau commented Apr 27, 2018 •

edited

Loading

refack commented Apr 27, 2018

refack commented Apr 27, 2018

richardlau commented Apr 27, 2018

refack commented Apr 27, 2018

richardlau commented Apr 27, 2018

richardlau commented Apr 28, 2018 •

edited

Loading

richardlau commented May 4, 2018

richardlau commented May 8, 2018

tools: add eslint check for skipIfEslintMissing #20372

tools: add eslint check for skipIfEslintMissing #20372

Conversation

richardlau commented Apr 27, 2018 • edited Loading

Checklist

addaleax Apr 27, 2018

Choose a reason for hiding this comment

richardlau Apr 27, 2018

Choose a reason for hiding this comment

This comment has been minimized.

richardlau commented Apr 27, 2018

richardlau commented Apr 27, 2018

richardlau commented Apr 27, 2018

refack commented Apr 27, 2018

richardlau commented Apr 27, 2018

richardlau commented Apr 27, 2018 • edited Loading

refack commented Apr 27, 2018

refack commented Apr 27, 2018

richardlau commented Apr 27, 2018

refack commented Apr 27, 2018

richardlau commented Apr 27, 2018

richardlau commented Apr 28, 2018 • edited Loading

richardlau commented May 4, 2018

richardlau commented May 8, 2018

richardlau commented Apr 27, 2018 •

edited

Loading

richardlau commented Apr 27, 2018 •

edited

Loading

richardlau commented Apr 28, 2018 •

edited

Loading