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

packages/eslint-plugin-jest/docs/rules/no-disabled-tests.md

@@ -1,33 +1,64 @@
 # 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:
 
 ```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');
+```

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,

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,
+        },
+      ],
     },
   ],
 });

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;
+      }
+    },
+  };
+};

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,23 +36,23 @@ export type MemberExpression = {
   expression: CallExpression,
   property: Identifier,
   object: Identifier,
-  parent: Node,
+  parent: ParentNode,
   loc: NodeLocation,
 };
 
 export type Literal = {
   type: 'Literal',
   value?: string,
   rawValue?: string,
-  parent: Node,
+  parent: ParentNode,
   loc: NodeLocation,
 };
 
 export type CallExpression = {
   type: 'CallExpression',
   arguments: Array<Literal>,
   callee: Identifier | MemberExpression,
-  parent: Node,
+  parent: ParentNode,
   loc: NodeLocation,
 };