Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rules): add no-commented-out rule #262

Merged
merged 13 commits into from May 22, 2019
2 changes: 2 additions & 0 deletions README.md
Expand Up @@ -96,6 +96,7 @@ for more information about extending configuration files.
| [lowercase-name][] | Disallow capitalized test names | | ![fixable-green][] |
| [no-alias-methods][] | Disallow alias methods | ![recommended][] | ![fixable-green][] |
| [no-disabled-tests][] | Disallow disabled tests | ![recommended][] | |
| [no-commented-out-tests][] | Disallow commented out tests | | | s |
| [no-empty-title][] | Disallow empty titles | | |
| [no-focused-tests][] | Disallow focused tests | ![recommended][] | |
| [no-hooks][] | Disallow setup and teardown hooks | | |
Expand Down Expand Up @@ -142,6 +143,7 @@ https://github.com/dangreenisrael/eslint-plugin-jest-formatting
[lowercase-name]: docs/rules/lowercase-name.md
[no-alias-methods]: docs/rules/no-alias-methods.md
[no-disabled-tests]: docs/rules/no-disabled-tests.md
[no-commented-out-tests]: docs/rules/no-commented-out-tests.md
[no-empty-title]: docs/rules/no-empty-title.md
[no-focused-tests]: docs/rules/no-focused-tests.md
[no-hooks]: docs/rules/no-hooks.md
Expand Down
61 changes: 61 additions & 0 deletions docs/rules/no-commented-out-tests.md
@@ -0,0 +1,61 @@
# Disallow commented out tests (no-commented-out-tests)

This rule raises a warning about commented out tests. It's similar to
no-disabled-tests rule.

## Rule Details

The rule uses fuzzy matching to do its best to determine what constitutes a
commented out test, checking for a presence of `it(`, `describe(`, `it.skip(`,
etc. in code comments.

<!-- The following patterns are considered warnings:

```js
// describe('foo', () => {});
// it('foo', () => {});
// test('foo', () => {});

// describe.skip('foo', () => {});
// it.skip('foo', () => {});
// test.skip('foo', () => {});

// describe['skip']('bar', () => {});
// it['skip']('bar', () => {});
// test['skip']('bar', () => {});

// xdescribe('foo', () => {});
// xit('foo', () => {});
// xtest('foo', () => {});

/*
describe('foo', () => {});
*/
```

These patterns would not be considered warnings:

```js
describe('foo', () => {});
it('foo', () => {});
test('foo', () => {});

describe.only('bar', () => {});
it.only('bar', () => {});
test.only('bar', () => {});

// foo('bar', () => {});
```

### Limitations

The plugin looks at the literal function names within test code, so will not
catch more complex examples of commented out tests, such as:

```js
// const testSkip = test.skip;
// testSkip('skipped test', () => {});

// const myTest = test;
// myTest('does not have function body');
``` -->
2 changes: 1 addition & 1 deletion src/__tests__/rules.test.js
Expand Up @@ -5,7 +5,7 @@ const path = require('path');
const { rules } = require('../');

const ruleNames = Object.keys(rules);
const numberOfRules = 31;
const numberOfRules = 32;

describe('rules', () => {
it('should have a corresponding doc for each rule', () => {
Expand Down
1 change: 1 addition & 0 deletions src/index.js
Expand Up @@ -38,6 +38,7 @@ module.exports = {
rules: {
'jest/no-alias-methods': 'warn',
'jest/no-disabled-tests': 'warn',
'jest/no-commented-out-tests': 'warn',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing the recommended config is a breaking change.

Not sure this needs to be in the recommended config, at all, but we can revisit that before the next release 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't realise this will modify recommended config. On the other hand, I think it's ok to warn since warnings usually don't fail with tools like lint-staged, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People can run eslint with a flag failing on warnings

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it

'jest/no-focused-tests': 'error',
'jest/no-identical-title': 'error',
'jest/no-jest-import': 'error',
Expand Down
191 changes: 191 additions & 0 deletions src/rules/__tests__/no-commented-out-tests.test.js
@@ -0,0 +1,191 @@
'use strict';

const { RuleTester } = require('eslint');
const rule = require('../no-commented-out-tests');

const ruleTester = new RuleTester({
parserOptions: {
sourceType: 'module',
},
});

ruleTester.run('no-commented-out-tests', rule, {
valid: [
'// foo("bar", function () {})',
'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)',
'({ f: function () {} }).f()',
'(a || b).f()',
'itHappensToStartWithIt()',
'testSomething()',
[
'import { pending } from "actions"',
'',
'test("foo", () => {',
' expect(pending()).toEqual({})',
'})',
].join('\n'),
[
'const { pending } = require("actions")',
'',
'test("foo", () => {',
' expect(pending()).toEqual({})',
'})',
].join('\n'),
[
'test("foo", () => {',
' const pending = getPending()',
' expect(pending()).toEqual({})',
'})',
].join('\n'),
[
'test("foo", () => {',
' expect(pending()).toEqual({})',
'})',
'',
'function pending() {',
' return {}',
'}',
].join('\n'),
],

invalid: [
{
code: '// describe("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// describe["skip"]("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// describe[\'skip\']("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// it.skip("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// it.only("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// it["skip"]("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// test.skip("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// test["skip"]("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// xdescribe("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// xit("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// xtest("foo", function () {})',
errors: [
{ message: 'Some tests seem to be commented', column: 1, line: 1 },
],
},
{
code: '// it("has title but no callback")',
errors: [
{
message: 'Some tests seem to be commented',
column: 1,
line: 1,
},
],
},
{
code: '// it()',
errors: [
{
message: 'Some tests seem to be commented',
column: 1,
line: 1,
},
],
},
{
code: '// test.someNewMethodThatMightBeAddedInTheFuture()',
errors: [
{
message: 'Some tests seem to be commented',
column: 1,
line: 1,
},
],
},
{
code: '// test["someNewMethodThatMightBeAddedInTheFuture"]()',
errors: [
{
message: 'Some tests seem to be commented',
column: 1,
line: 1,
},
],
},
{
code: '// test("has title but no callback")',
errors: [
{
message: 'Some tests seem to be commented',
column: 1,
line: 1,
},
],
},
{
code: `
foo()
/*
describe("has title but no callback", () => {})
*/
bar()`,
errors: [
{
message: 'Some tests seem to be commented',
column: 7,
line: 3,
},
],
},
],
});
37 changes: 37 additions & 0 deletions src/rules/no-commented-out-tests.js
@@ -0,0 +1,37 @@
'use strict';

const { getDocsUrl } = require('./util');

const message = 'Some tests seem to be commented';

function hasTests(node) {
return /x?(test|it|describe)(\.\w+|\[['"]\w+['"]\])?\(.*?\)/.test(node.value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Other than xit, there's also fit
  • The closing parenthesis might cause trouble with multi-line tests in comments, we might need the //s regex modifier? Should add some multiline test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed both comments and added tests. The thing about newlines is that ESLint seems to create a separate comment for each line with line-level comments. We can't detect stuff like:

// test
// (
// 'x
// )

..which, I think, is an OK limitation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For block comments as well though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, best thing might be to just not look for the closing parenthesis? // test( already looks suspicious enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I did just that in the last commit. Let me add a multiline comment test as well.

}

module.exports = {
meta: {
docs: {
url: getDocsUrl(__filename),
},
},
create(context) {
const sourceCode = context.getSourceCode();

function checkNode(node) {
if (!hasTests(node)) return;

context.report({
message,
node,
});
}

return {
Program() {
const comments = sourceCode.getAllComments();

comments.filter(token => token.type !== 'Shebang').forEach(checkNode);
},
};
},
};