fix(no-jasmine-globals): fix false positives for pending/fail/spyOn (#…

…205) Fixes #156
jest-community · Nov 25, 2018 · edbd1a2 · edbd1a2
1 parent 4db773d
commit edbd1a2
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 65 deletions.
diff --git a/rules/__tests__/no-jasmine-globals.test.js b/rules/__tests__/no-jasmine-globals.test.js
@@ -15,6 +15,9 @@ ruleTester.run('no-jasmine-globals', rule, {
     'test("foo", function () {})',
     'foo()',
     `require('foo')('bar')`,
+    'function callback(fail) { fail() }',
+    'var spyOn = require("actions"); spyOn("foo")',
+    'function callback(pending) { pending() }',
   ],
   invalid: [
     {

diff --git a/rules/no-disabled-tests.js b/rules/no-disabled-tests.js
@@ -1,33 +1,6 @@
 'use strict';
 
-const { getDocsUrl, getNodeName } = require('./util');
-
-function collectReferences(scope) {
-  const locals = new Set();
-  const unresolved = new Set();
-
-  let currentScope = scope;
-
-  while (currentScope !== null) {
-    for (const ref of currentScope.variables) {
-      const isReferenceDefined = ref.defs.some(def => {
-        return def.type !== 'ImplicitGlobalVariable';
-      });
-
-      if (isReferenceDefined) {
-        locals.add(ref.name);
-      }
-    }
-
-    for (const ref of currentScope.through) {
-      unresolved.add(ref.identifier.name);
-    }
-
-    currentScope = currentScope.upper;
-  }
-
-  return { locals, unresolved };
-}
+const { getDocsUrl, getNodeName, scopeHasLocalReference } = require('./util');
 
 module.exports = {
   meta: {
@@ -67,15 +40,7 @@ module.exports = {
         }
       },
       'CallExpression[callee.name="pending"]'(node) {
-        const references = collectReferences(context.getScope());
-
-        if (
-          // `pending` was found as a local variable or function declaration.
-          references.locals.has('pending') ||
-          // `pending` was not found as an unresolved reference,
-          // meaning it is likely not an implicit global reference.
-          !references.unresolved.has('pending')
-        ) {
+        if (scopeHasLocalReference(context.getScope(), 'pending')) {
           return;
         }
 

diff --git a/rules/no-jasmine-globals.js b/rules/no-jasmine-globals.js
@@ -1,13 +1,24 @@
 'use strict';
 
-const { getDocsUrl, getNodeName } = require('./util');
+const { getDocsUrl, getNodeName, scopeHasLocalReference } = require('./util');
 
 module.exports = {
   meta: {
     docs: {
       url: getDocsUrl(__filename),
     },
     fixable: 'code',
+    messages: {
+      illegalGlobal:
+        'Illegal usage of global `{{ global }}`, prefer `{{ replacement }}`',
+      illegalMethod:
+        'Illegal usage of `{{ method }}`, prefer `{{ replacement }}`',
+      illegalFail:
+        'Illegal usage of `fail`, prefer throwing an error, or the `done.fail` callback',
+      illegalPending:
+        'Illegal usage of `pending`, prefer explicitly skipping a test using `test.skip`',
+      illegalJasmine: 'Illegal usage of jasmine global',
+    },
   },
   create(context) {
     return {
@@ -17,30 +28,39 @@ module.exports = {
         if (!calleeName) {
           return;
         }
+        if (
+          calleeName === 'spyOn' ||
+          calleeName === 'spyOnProperty' ||
+          calleeName === 'fail' ||
+          calleeName === 'pending'
+        ) {
+          if (scopeHasLocalReference(context.getScope(), calleeName)) {
+            // It's a local variable, not a jasmine global.
+            return;
+          }
 
-        if (calleeName === 'spyOn' || calleeName === 'spyOnProperty') {
-          context.report({
-            node,
-            message: `Illegal usage of global \`${calleeName}\`, prefer \`jest.spyOn\``,
-          });
-          return;
-        }
-
-        if (calleeName === 'fail') {
-          context.report({
-            node,
-            message:
-              'Illegal usage of `fail`, prefer throwing an error, or the `done.fail` callback',
-          });
-          return;
-        }
-
-        if (calleeName === 'pending') {
-          context.report({
-            node,
-            message:
-              'Illegal usage of `pending`, prefer explicitly skipping a test using `test.skip`',
-          });
+          switch (calleeName) {
+            case 'spyOn':
+            case 'spyOnProperty':
+              context.report({
+                node,
+                messageId: 'illegalGlobal',
+                data: { global: calleeName, replacement: 'jest.spyOn' },
+              });
+              break;
+            case 'fail':
+              context.report({
+                node,
+                messageId: 'illegalFail',
+              });
+              break;
+            case 'pending':
+              context.report({
+                node,
+                messageId: 'illegalPending',
+              });
+              break;
+          }
           return;
         }
 
@@ -59,30 +79,42 @@ module.exports = {
                 return [fixer.replaceText(node.callee.object, 'expect')];
               },
               node,
-              message: `Illegal usage of \`${calleeName}\`, prefer \`expect.${functionName}\``,
+              messageId: 'illegalMethod',
+              data: {
+                method: calleeName,
+                replacement: `expect.${functionName}`,
+              },
             });
             return;
           }
 
           if (functionName === 'addMatchers') {
             context.report({
               node,
-              message: `Illegal usage of \`${calleeName}\`, prefer \`expect.extend\``,
+              messageId: 'illegalMethod',
+              data: {
+                method: calleeName,
+                replacement: `expect.extend`,
+              },
             });
             return;
           }
 
           if (functionName === 'createSpy') {
             context.report({
               node,
-              message: `Illegal usage of \`${calleeName}\`, prefer \`jest.fn\``,
+              messageId: 'illegalMethod',
+              data: {
+                method: calleeName,
+                replacement: 'jest.fn',
+              },
             });
             return;
           }
 
           context.report({
             node,
-            message: 'Illegal usage of jasmine global',
+            messageId: 'illegalJasmine',
           });
         }
       },

diff --git a/rules/util.js b/rules/util.js
@@ -142,6 +142,44 @@ const getDocsUrl = filename => {
   return `${REPO_URL}/blob/v${version}/docs/rules/${ruleName}.md`;
 };
 
+const collectReferences = scope => {
+  const locals = new Set();
+  const unresolved = new Set();
+
+  let currentScope = scope;
+
+  while (currentScope !== null) {
+    for (const ref of currentScope.variables) {
+      const isReferenceDefined = ref.defs.some(def => {
+        return def.type !== 'ImplicitGlobalVariable';
+      });
+
+      if (isReferenceDefined) {
+        locals.add(ref.name);
+      }
+    }
+
+    for (const ref of currentScope.through) {
+      unresolved.add(ref.identifier.name);
+    }
+
+    currentScope = currentScope.upper;
+  }
+
+  return { locals, unresolved };
+};
+
+const scopeHasLocalReference = (scope, referenceName) => {
+  const references = collectReferences(scope);
+  return (
+    // referenceName was found as a local variable or function declaration.
+    references.locals.has(referenceName) ||
+    // referenceName was not found as an unresolved reference,
+    // meaning it is likely not an implicit global reference.
+    !references.unresolved.has(referenceName)
+  );
+};
+
 module.exports = {
   method,
   method2,
@@ -160,4 +198,5 @@ module.exports = {
   isFunction,
   isTestCase,
   getDocsUrl,
+  scopeHasLocalReference,
 };