From edbd1a2967bba335e46c4adabca258cbcd40f8d4 Mon Sep 17 00:00:00 2001 From: Jonathan del Strother Date: Sun, 25 Nov 2018 19:27:38 +0000 Subject: [PATCH] fix(no-jasmine-globals): fix false positives for pending/fail/spyOn (#205) Fixes #156 --- rules/__tests__/no-jasmine-globals.test.js | 3 + rules/no-disabled-tests.js | 39 +--------- rules/no-jasmine-globals.js | 88 +++++++++++++++------- rules/util.js | 39 ++++++++++ 4 files changed, 104 insertions(+), 65 deletions(-) diff --git a/rules/__tests__/no-jasmine-globals.test.js b/rules/__tests__/no-jasmine-globals.test.js index 7262292aa..9e4f1df7c 100644 --- 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 index 4da48c95d..a7ebad201 100644 --- 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 index 5a5f31285..31c2410d2 100644 --- a/rules/no-jasmine-globals.js +++ b/rules/no-jasmine-globals.js @@ -1,6 +1,6 @@ 'use strict'; -const { getDocsUrl, getNodeName } = require('./util'); +const { getDocsUrl, getNodeName, scopeHasLocalReference } = require('./util'); module.exports = { meta: { @@ -8,6 +8,17 @@ module.exports = { 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,7 +79,11 @@ 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; } @@ -67,7 +91,11 @@ module.exports = { if (functionName === 'addMatchers') { context.report({ node, - message: `Illegal usage of \`${calleeName}\`, prefer \`expect.extend\``, + messageId: 'illegalMethod', + data: { + method: calleeName, + replacement: `expect.extend`, + }, }); return; } @@ -75,14 +103,18 @@ module.exports = { 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 index 410d343fe..9f8861a78 100644 --- 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, };