From f67476e59e6e97a216be42fab0df5122aa5b9aed Mon Sep 17 00:00:00 2001 From: John Adams Date: Mon, 25 Sep 2017 14:44:34 -0500 Subject: [PATCH] Allow eslint-plugin to recognize more disabled tests (#4533) Already recognizes: * tests that append `skip` (e.g. `it.skip(...)`) * tests that prepend `x` (e.g. `xit(...)`) This change adds: * tests that don't contain a function body * tests that contain a call to `pending()` https://jasmine.github.io/2.0/introduction.html#section-Pending_Specs --- .../docs/rules/no-disabled-tests.md | 43 +++++- packages/eslint-plugin-jest/src/index.js | 1 + .../rules/__tests__/no_skipped_tests.test.js | 83 +++++++--- .../src/rules/no_disabled_tests.js | 143 ++++++++++++------ .../eslint-plugin-jest/src/rules/types.js | 14 +- 5 files changed, 202 insertions(+), 82 deletions(-) diff --git a/packages/eslint-plugin-jest/docs/rules/no-disabled-tests.md b/packages/eslint-plugin-jest/docs/rules/no-disabled-tests.md index 0d9258abb904..9df966140af9 100644 --- a/packages/eslint-plugin-jest/docs/rules/no-disabled-tests.md +++ b/packages/eslint-plugin-jest/docs/rules/no-disabled-tests.md @@ -1,24 +1,41 @@ # Disallow Disabled Tests (no-disabled-tests) -Jest has a feature that allows you to skip tests by appending `.skip` or prepending `x` to a test-suite or a test-case. -Sometimes tests are skipped as part of a debugging process and aren't intended to be committed. This rule reminds you to remove .skip or the x prefix from your tests. +Jest has a feature that allows you to temporarily mark tests as disabled. This +feature is often helpful while debugging or to create placeholders for future +tests. Before committing changes we may want to check that all tests are +running. + +This rule raises a warning about disabled tests. ## Rule Details -This rule looks for every `describe.skip`, `it.skip`, `test.skip`, `xdescribe`, `xit` and `xtest` occurrences within the source code. +There are a number of ways to disable tests in Jest: +* by appending `.skip` to the test-suite or test-case +* by prepending the test function name with `x` +* by declaring a test with a name but no function body +* by making a call to `pending()` anywhere within the test The following patterns are considered warnings: ```js describe.skip('foo', () => {}); it.skip('foo', () => {}); +test.skip('foo', () => {}); + describe['skip']('bar', () => {}); it['skip']('bar', () => {}); -test.skip('foo', () => {}); test['skip']('bar', () => {}); + xdescribe('foo', () => {}); xit('foo', () => {}); -xtest('bar', () => {}); +xtest('foo', () => {}); + +it('bar'); +test('bar'); + +it('foo', () => { + pending() +}); ``` These patterns would not be considered warnings: @@ -26,8 +43,22 @@ These patterns would not be considered warnings: ```js describe('foo', () => {}); it('foo', () => {}); +test('foo', () => {}); + describe.only('bar', () => {}); it.only('bar', () => {}); -test('foo', () => {}); test.only('bar', () => {}); ``` + +### Limitations + +The plugin looks at the literal function names within test code, so will not +catch more complex examples of disabled tests, such as: + +```js +const testSkip = test.skip; +testSkip('skipped test', () => {}); + +const myTest = test; +myTest('does not have function body'); +``` diff --git a/packages/eslint-plugin-jest/src/index.js b/packages/eslint-plugin-jest/src/index.js index 32791fe38b02..bc4d1d076a8a 100644 --- a/packages/eslint-plugin-jest/src/index.js +++ b/packages/eslint-plugin-jest/src/index.js @@ -37,6 +37,7 @@ module.exports = { it: false, jasmine: false, jest: false, + pending: false, pit: false, require: false, test: false, diff --git a/packages/eslint-plugin-jest/src/rules/__tests__/no_skipped_tests.test.js b/packages/eslint-plugin-jest/src/rules/__tests__/no_skipped_tests.test.js index d3c7479b853a..d07fdacb64ef 100644 --- a/packages/eslint-plugin-jest/src/rules/__tests__/no_skipped_tests.test.js +++ b/packages/eslint-plugin-jest/src/rules/__tests__/no_skipped_tests.test.js @@ -16,52 +16,89 @@ import {RuleTester} from 'eslint'; const {rules} = require('../../'); const ruleTester = new RuleTester(); -const expectedErrorMessage = 'Unexpected disabled test.'; ruleTester.run('no-disabled-tests', rules['no-disabled-tests'], { valid: [ - 'describe()', - 'it()', - 'describe.only()', - 'it.only()', - 'test()', - 'test.only()', + 'describe("foo", function () {})', + 'it("foo", function () {})', + 'describe.only("foo", function () {})', + 'it.only("foo", function () {})', + 'test("foo", function () {})', + 'test.only("foo", function () {})', 'var appliedSkip = describe.skip; appliedSkip.apply(describe)', 'var calledSkip = it.skip; calledSkip.call(it)', ], invalid: [ { - code: 'describe.skip()', - errors: [{message: expectedErrorMessage, column: 10, line: 1}], + code: 'describe.skip("foo", function () {})', + errors: [{message: 'Skipped test suite', column: 1, line: 1}], }, { - code: 'describe["skip"]()', - errors: [{message: expectedErrorMessage, column: 10, line: 1}], + code: 'describe["skip"]("foo", function () {})', + errors: [{message: 'Skipped test suite', column: 1, line: 1}], }, { - code: 'it.skip()', - errors: [{message: expectedErrorMessage, column: 4, line: 1}], + code: 'it.skip("foo", function () {})', + errors: [{message: 'Skipped test', column: 1, line: 1}], }, { - code: 'it["skip"]()', - errors: [{message: expectedErrorMessage, column: 4, line: 1}], + code: 'it["skip"]("foo", function () {})', + errors: [{message: 'Skipped test', column: 1, line: 1}], }, { - code: 'test.skip()', - errors: [{message: expectedErrorMessage, column: 6, line: 1}], + code: 'test.skip("foo", function () {})', + errors: [{message: 'Skipped test', column: 1, line: 1}], }, { - code: 'test["skip"]()', - errors: [{message: expectedErrorMessage, column: 6, line: 1}], + code: 'test["skip"]("foo", function () {})', + errors: [{message: 'Skipped test', column: 1, line: 1}], }, { - code: 'xdescribe()', - errors: [{message: expectedErrorMessage, column: 1, line: 1}], + code: 'xdescribe("foo", function () {})', + errors: [{message: 'Disabled test suite', column: 1, line: 1}], }, { - code: 'xit()', - errors: [{message: expectedErrorMessage, column: 1, line: 1}], + code: 'xit("foo", function () {})', + errors: [{message: 'Disabled test', column: 1, line: 1}], + }, + { + code: 'xtest("foo", function () {})', + errors: [{message: 'Disabled test', column: 1, line: 1}], + }, + { + code: 'it("has title but no callback")', + errors: [ + { + message: 'Test is missing function argument', + column: 1, + line: 1, + }, + ], + }, + { + code: 'test("has title but no callback")', + errors: [ + { + message: 'Test is missing function argument', + column: 1, + line: 1, + }, + ], + }, + { + code: 'it("contains a call to pending", function () { pending() })', + errors: [{message: 'Call to pending() within test', column: 48, line: 1}], + }, + { + code: 'describe("contains a call to pending", function () { pending() })', + errors: [ + { + message: 'Call to pending() within test suite', + column: 54, + line: 1, + }, + ], }, ], }); diff --git a/packages/eslint-plugin-jest/src/rules/no_disabled_tests.js b/packages/eslint-plugin-jest/src/rules/no_disabled_tests.js index cdbb0aa9ef16..d21ce07ecd86 100644 --- a/packages/eslint-plugin-jest/src/rules/no_disabled_tests.js +++ b/packages/eslint-plugin-jest/src/rules/no_disabled_tests.js @@ -8,50 +8,99 @@ * @flow */ -import type {EslintContext, CallExpression} from './types'; - -/* $FlowFixMe */ -const testFunctions = Object.assign(Object.create(null), { - describe: true, - it: true, - test: true, -}); - -const matchesTestFunction = object => object && testFunctions[object.name]; - -const isCallToSkippedTestFunction = object => - object && object.name[0] === 'x' && testFunctions[object.name.substring(1)]; - -const isPropertyNamedSkip = property => - property && (property.name === 'skip' || property.value === 'skip'); - -const isCallToTestSkipFunction = callee => - matchesTestFunction(callee.object) && isPropertyNamedSkip(callee.property); - -export default (context: EslintContext) => ({ - CallExpression(node: CallExpression) { - const callee = node.callee; - if (!callee) { - return; - } - - if ( - callee.type === 'MemberExpression' && - isCallToTestSkipFunction(callee) - ) { - context.report({ - message: 'Unexpected disabled test.', - node: callee.property, - }); - return; - } - - if (callee.type === 'Identifier' && isCallToSkippedTestFunction(callee)) { - context.report({ - message: 'Unexpected disabled test.', - node: callee, - }); - return; - } - }, -}); +import type {Node, EslintContext, CallExpression} from './types'; + +function getName(node: ?Node): ?string { + function joinNames(a: ?string, b: ?string): ?string { + return a && b ? a + '.' + b : null; + } + + switch (node && node.type) { + case 'Identifier': + // $FlowFixMe: ignore duck-typed node property + return node.name; + case 'Literal': + // $FlowFixMe: ignore duck-typed node property + return node.value; + case 'MemberExpression': + // $FlowFixMe: ignore duck-typed node properties + return joinNames(getName(node.object), getName(node.property)); + } + + return null; +} + +export default (context: EslintContext) => { + let suiteDepth = 0; + let testDepth = 0; + + return { + CallExpression: (node: CallExpression) => { + const functionName = getName(node.callee); + + switch (functionName) { + case 'describe': + suiteDepth++; + break; + + case 'describe.skip': + context.report({message: 'Skipped test suite', node}); + break; + + case 'it': + case 'test': + testDepth++; + if (node.arguments.length < 2) { + context.report({ + message: 'Test is missing function argument', + node, + }); + } + break; + + case 'it.skip': + case 'test.skip': + context.report({message: 'Skipped test', node}); + break; + + case 'pending': + if (testDepth > 0) { + context.report({ + message: 'Call to pending() within test', + node, + }); + } else if (suiteDepth > 0) { + context.report({ + message: 'Call to pending() within test suite', + node, + }); + } + break; + + case 'xdescribe': + context.report({message: 'Disabled test suite', node}); + break; + + case 'xit': + case 'xtest': + context.report({message: 'Disabled test', node}); + break; + } + }, + + 'CallExpression:exit': (node: CallExpression) => { + const functionName = getName(node.callee); + + switch (functionName) { + case 'describe': + suiteDepth--; + break; + + case 'it': + case 'test': + testDepth--; + break; + } + }, + }; +}; diff --git a/packages/eslint-plugin-jest/src/rules/types.js b/packages/eslint-plugin-jest/src/rules/types.js index 7b2ee0f7f67c..2d568dfbea50 100644 --- a/packages/eslint-plugin-jest/src/rules/types.js +++ b/packages/eslint-plugin-jest/src/rules/types.js @@ -8,8 +8,6 @@ * @flow */ -type Node = MemberExpression | CallExpression; - type Location = { column: number, line: number, @@ -20,11 +18,15 @@ type NodeLocation = { start: Location, }; +type ParentNode = CallExpression | MemberExpression; + +export type Node = CallExpression | MemberExpression | Identifier | Literal; + export type Identifier = { type: 'Identifier', name: string, value: string, - parent: Node, + parent: ParentNode, loc: NodeLocation, }; @@ -34,7 +36,7 @@ export type MemberExpression = { expression: CallExpression, property: Identifier, object: Identifier, - parent: Node, + parent: ParentNode, loc: NodeLocation, }; @@ -42,7 +44,7 @@ export type Literal = { type: 'Literal', value?: string, rawValue?: string, - parent: Node, + parent: ParentNode, loc: NodeLocation, }; @@ -50,7 +52,7 @@ export type CallExpression = { type: 'CallExpression', arguments: Array, callee: Identifier | MemberExpression, - parent: Node, + parent: ParentNode, loc: NodeLocation, };