Upgrade to ESLint 4.1 and add no-focused-tests rule

[bvaughn]

It's too easy for focused tests to slip into the repo..

This PR addresses this by doing the following:

• Runs a lint rule on tests only that errors if it sees fdescribe or fit calls.
• Changes file: to link: for our custom, internal rules (just to simplify updating these in the future).
• Updates eslint from 3.10 -> 4.1 and babel-eslint from 7.1 -> 8.0 so that we can run this new rule only against tests.

[gaearon]

due to our use of file:

Let's change it to link:?

[gaearon]

OK but maybe scope it to -test.js and -test.internal.js?

[bvaughn]

OK but maybe scope it to -test.js and -test.internal.js?

I'm unclear of how to do this?

Based on the docs and this discussion thread, I would expect something like this to work:

module.exports = {
  rules: {
    // ...other rules...
    'react-internal/no-focused-tests': OFF,
  },

  overrides: [
    {
      files: '**/__tests__/*.js',
      rules: {
        'react-internal/no-focused-tests': ERROR,
      }
    }
  ]
};

But I've tried a few variants and none of them work. Seems like overrides is just being ignored. I must be missing something.

I could accomplish this in a different way (below) but it's probably too hacky:

module.exports = function(context) {
  if (!context.getFilename().includes('__tests__')) {
    return {};
  }

  return {
    CallExpression: node => {
      const name = node.callee.name;

      if (name === 'fdescribe' || name === 'fit') {
        context.report(node, 'Focused tests are not allowed.');
      }
    },
  };
};

[bvaughn]

Let's change it to link:?

It seems the contents of eslint-plugin-react-internals get copied to node_modules when you yarn install, but then changes are not synced (regardless of whether we use file: or link:) unless you re-run yarn install.

I'm not very familiar with this type of intra-project linking, so maybe I'm overlooking or misunderstanding something fundamental. 😄

I'm not convinced this needs to be fixed right now though, as running yarn install "fixes" the issue and seems like the obvious thing to try if a lint rule can't be found (even if you don't know about the way these internal rules are managed). Plus this folder doesn't change often.

(I verified my above statement by doing a fresh install on master- rm -rf node_modules && yarn install, then checking out my new branch and running yarn linc and verified that it failed, then running yarn install and re-running yarn linc and verified that it passed.)

[gaearon]

but then changes are not synced (regardless of whether we use file: or link:) unless you re-run yarn install.

link: just creates a symlink so I don't see how changes could be not reflected unless yarn bugged out. Note that you'd need to change lockfile too for that to work. The easiest way is to first yarn remove <name> and then yarn add --dev -W link:<path>.

[thymikee]

@bvaughn Psst, there's eslint-plugin-jest out there in the wild, may be handy here (no-focused-tests rule is already there).

[bvaughn]

Love the suggestion of using a shared rule, @thymikee! When I tried that rule on our codebase, it errored with a "Cannot read property 'type' of undefined" error.

I tried to patch that specific problem and encountered a few more, so I set it aside for the time being. I will try to find time to look into it a little more but I'm not convinced it's worth the effort to switch to this rule at the moment, given the simplicity of it?

[thymikee]

Appreciate if you could trace it and file an appropriate issue :) cc @SimenB

[bvaughn]

Appreciate if you could trace it and file an appropriate issue

Sure, sure. Like I said, there seemed to be several issues. If you beat me to it though, you can reproduce by checking out this repo, applying the below patch, and running, yarn lint:

diff --git a/.eslintrc.js b/.eslintrc.js
index 57f7888f9..7066c3f9c 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -10,6 +10,7 @@ module.exports = {
   'root': true,
 
   plugins: [
+    'jest',
     'react',
     'react-internal',
   ],
@@ -40,6 +41,9 @@ module.exports = {
     'space-before-blocks': ERROR,
     'space-before-function-paren': OFF,
 
+    // https://github.com/jest-community/eslint-plugin-jest
+    'jest/no-focused-tests': ERROR,
+
     // React & JSX
     // Our transforms set this automatically
     'react/jsx-boolean-value': [ERROR, 'always'],
@@ -57,7 +61,6 @@ module.exports = {
 
     // CUSTOM RULES
     // the second argument of warning/invariant should be a literal string
-    'react-internal/no-focused-tests': ERROR,
     'react-internal/no-primitive-constructors': ERROR,
     'react-internal/warning-and-invariant-args': ERROR,
   },
diff --git a/package.json b/package.json
index dfcb33edf..b9eda8901 100644
--- a/package.json
+++ b/package.json
@@ -50,10 +50,11 @@
     "del": "^2.0.2",
     "derequire": "^2.0.3",
     "escape-string-regexp": "^1.0.5",
-    "eslint": "^3.10.2",
+    "eslint": "^4.15.0",
     "eslint-config-fbjs": "^1.1.1",
     "eslint-plugin-babel": "^3.3.0",
     "eslint-plugin-flowtype": "^2.25.0",
+    "eslint-plugin-jest": "^21.5.0",
     "eslint-plugin-react": "^6.7.1",
     "eslint-plugin-react-internal": "link:./scripts/eslint-rules/",
     "expect": "^21.3.0-beta.4",
diff --git a/yarn.lock b/yarn.lock
index a5ca34b57..16f6c5086 100644

(I omitted the yarn.lock portion of the diff since it can be regen'ed easily)

[thymikee]

Got a quick look and the errors in this diff come from ESLint 4 (not sure why though). All seem to work well under v3.

[SimenB]

The rule throwing is no-unused-vars, a core eslint rule. Just upgrading to eslint 4 throws the error, whether or not the jest plugin is in there.

I have zero clue how that happened, my shot in the dark is a bug in babel-eslint.

[SimenB]

It has something to do with config resolution in eslint 4. Doing rm packages/*/package.json makes lint pass with eslint 4. My guess is eslint treats package.json as root, but I don't know why... We don't have this issue in the Jest repo.

[thymikee]

@SimenB upgrading both eslint and babel-eslint works, totally forgot about it.

[SimenB]

The fix is to upgrade to babel-eslint@8, you're hit by https://twitter.com/geteslint/status/944829414159810560.

It will give annoying peer dep warning from fbjs config (maybe you can poke someone for facebook/fbjs#275).

The fix will not be backported to babel-eslint@7 (babel/babel-eslint#542 (comment)).

A potential solution if you want eslint@4 without upgrading babel-eslint is to lock down version 4.13.1.

[bvaughn]

OK but maybe scope it to -test.js and -test.internal.js?

Unfortunately, this was not possible with ESLint 3.x (what we used). The overrides option was added in ESLint 4.1. 😄

I've gone ahead and updated it (see b00249c) and in the process switched us over to use the shared rule mentioned above (see fff725b).

Based on my own local testing, the new rule only runs on tests. yarn lint passes (on our entire codebase) so it seems like our existing rules are also passing.

[bvaughn]

Thanks @thymikee and @SimenB (and Dan of course)

[SimenB]

If you can think of any rules for jest you'd like to have, please say so!