Skip to content

Commit a316e1e

Browse files
mskeltonmskelton
authored
Merge pull request #222 from playwright-community/no-commented-out-tests
Add `no-commented-out-tests` rule
2 parents 3b3cafd + 718d900 commit a316e1e

File tree

6 files changed

+265
-2
lines changed

6 files changed

+265
-2
lines changed

‎README.md

+1
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ command line option.\
122122
| ✔ | | | [expect-expect](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/expect-expect.md) | Enforce assertion to be made in a test body |
123123
| ✔ | | | [max-nested-describe](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/max-nested-describe.md) | Enforces a maximum depth to nested describe calls |
124124
| ✔ | 🔧 | | [missing-playwright-await](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/missing-playwright-await.md) | Enforce Playwright APIs to be awaited |
125+
| | | | [no-commented-out-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-commented-out-test.md) | Disallow commented out tests |
125126
| ✔ | | | [no-conditional-in-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-conditional-in-test.md) | Disallow conditional logic in tests |
126127
| ✔ | | 💡 | [no-element-handle](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-element-handle.md) | Disallow usage of element handles |
127128
| ✔ | | | [no-eval](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-eval.md) | Disallow usage of `page.$eval()` and `page.$$eval()` |

‎docs/rules/no-commented-out-tests.md

+54
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
# Disallow commented out tests (`no-commented-out-tests`)
2+
3+
This rule raises a warning about commented out tests. It's similar to
4+
`no-skipped-test` rule.
5+
6+
## Rule details
7+
8+
The rule uses fuzzy matching to do its best to determine what constitutes a
9+
commented out test, checking for a presence of `test(`, `test.describe(`,
10+
`test.skip(`, etc. in code comments.
11+
12+
The following patterns are considered warnings:
13+
14+
```js
15+
// describe('foo', () => {});
16+
// test.describe('foo', () => {});
17+
// test('foo', () => {});
18+
19+
// test.describe.skip('foo', () => {});
20+
// test.skip('foo', () => {});
21+
22+
// test.describe['skip']('bar', () => {});
23+
// test['skip']('bar', () => {});
24+
25+
/*
26+
test.describe('foo', () => {});
27+
*/
28+
```
29+
30+
These patterns would not be considered warnings:
31+
32+
```js
33+
describe('foo', () => {});
34+
test.describe('foo', () => {});
35+
test('foo', () => {});
36+
37+
test.describe.only('bar', () => {});
38+
test.only('bar', () => {});
39+
40+
// foo('bar', () => {});
41+
```
42+
43+
### Limitations
44+
45+
The plugin looks at the literal function names within test code, so will not
46+
catch more complex examples of commented out tests, such as:
47+
48+
```js
49+
// const testSkip = test.skip;
50+
// testSkip('skipped test', () => {});
51+
52+
// const myTest = test;
53+
// myTest('does not have function body');
54+
```

‎src/index.ts

+2
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import globals from 'globals';
22
import expectExpect from './rules/expect-expect';
33
import maxNestedDescribe from './rules/max-nested-describe';
44
import missingPlaywrightAwait from './rules/missing-playwright-await';
5+
import noCommentedOutTests from './rules/no-commented-out-tests';
56
import noConditionalInTest from './rules/no-conditional-in-test';
67
import noElementHandle from './rules/no-element-handle';
78
import noEval from './rules/no-eval';
@@ -38,6 +39,7 @@ const index = {
3839
'expect-expect': expectExpect,
3940
'max-nested-describe': maxNestedDescribe,
4041
'missing-playwright-await': missingPlaywrightAwait,
42+
'no-commented-out-tests': noCommentedOutTests,
4143
'no-conditional-in-test': noConditionalInTest,
4244
'no-element-handle': noElementHandle,
4345
'no-eval': noEval,

‎src/rules/no-commented-out-tests.ts

+44
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import { Rule } from 'eslint';
2+
import * as ESTree from 'estree';
3+
import { getTestNames } from '../utils/ast';
4+
5+
function hasTests(context: Rule.RuleContext, node: ESTree.Comment) {
6+
const testNames = getTestNames(context);
7+
const names = testNames.join('|');
8+
const regex = new RegExp(
9+
`^\\s*(${names}|describe)(\\.\\w+|\\[['"]\\w+['"]\\])?\\s*\\(`,
10+
'mu',
11+
);
12+
return regex.test(node.value);
13+
}
14+
15+
export default {
16+
create(context) {
17+
function checkNode(node: ESTree.Comment) {
18+
if (!hasTests(context, node)) return;
19+
20+
context.report({
21+
messageId: 'commentedTests',
22+
node: node as unknown as ESTree.Node,
23+
});
24+
}
25+
26+
return {
27+
Program() {
28+
context.sourceCode.getAllComments().forEach(checkNode);
29+
},
30+
};
31+
},
32+
meta: {
33+
docs: {
34+
category: 'Best Practices',
35+
description: 'Disallow commented out tests',
36+
recommended: true,
37+
url: 'https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-commented-out-tests.md',
38+
},
39+
messages: {
40+
commentedTests: 'Some tests seem to be commented',
41+
},
42+
type: 'problem',
43+
},
44+
} as Rule.RuleModule;

‎src/utils/ast.ts

+6-2
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,13 @@ export function isPropertyAccessor(
6363
return getStringValue(node.property) === name;
6464
}
6565

66-
export function isTestIdentifier(context: Rule.RuleContext, node: ESTree.Node) {
66+
export function getTestNames(context: Rule.RuleContext) {
6767
const aliases = context.settings.playwright?.globalAliases?.test ?? [];
68-
const testNames = ['test', ...aliases];
68+
return ['test', ...aliases];
69+
}
70+
71+
export function isTestIdentifier(context: Rule.RuleContext, node: ESTree.Node) {
72+
const testNames = getTestNames(context);
6973
const regex = new RegExp(`^(${testNames.join('|')})$`);
7074

7175
return (

‎test/spec/no-commented-out-tests.spec.ts

+158
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
import dedent from 'dedent';
2+
import rule from '../../src/rules/no-commented-out-tests';
3+
import { runRuleTester } from '../utils/rule-tester';
4+
5+
const messageId = 'commentedTests';
6+
7+
runRuleTester('no-commented-out-tests', rule, {
8+
invalid: [
9+
{
10+
code: '// test.describe("foo", function () {})',
11+
errors: [{ column: 1, line: 1, messageId }],
12+
},
13+
{
14+
code: '// describe["skip"]("foo", function () {})',
15+
errors: [{ column: 1, line: 1, messageId }],
16+
},
17+
{
18+
code: '// describe[\'skip\']("foo", function () {})',
19+
errors: [{ column: 1, line: 1, messageId }],
20+
},
21+
{
22+
code: '// test("foo", function () {})',
23+
errors: [{ column: 1, line: 1, messageId }],
24+
},
25+
{
26+
code: '// test.only("foo", function () {})',
27+
errors: [{ column: 1, line: 1, messageId }],
28+
},
29+
{
30+
code: '// test["skip"]("foo", function () {})',
31+
errors: [{ column: 1, line: 1, messageId }],
32+
},
33+
{
34+
code: '// test.skip("foo", function () {})',
35+
errors: [{ column: 1, line: 1, messageId }],
36+
},
37+
{
38+
code: '// test["skip"]("foo", function () {})',
39+
errors: [{ column: 1, line: 1, messageId }],
40+
},
41+
{
42+
code: dedent`
43+
// test(
44+
// "foo", function () {}
45+
// )
46+
`,
47+
errors: [{ column: 1, line: 1, messageId }],
48+
},
49+
{
50+
code: dedent`
51+
/* test
52+
(
53+
"foo", function () {}
54+
)
55+
*/
56+
`,
57+
errors: [{ column: 1, line: 1, messageId }],
58+
},
59+
{
60+
code: '// test("has title but no callback")',
61+
errors: [{ column: 1, line: 1, messageId }],
62+
},
63+
{
64+
code: '// test()',
65+
errors: [{ column: 1, line: 1, messageId }],
66+
},
67+
{
68+
code: '// test.someNewMethodThatMightBeAddedInTheFuture()',
69+
errors: [{ column: 1, line: 1, messageId }],
70+
},
71+
{
72+
code: '// test["someNewMethodThatMightBeAddedInTheFuture"]()',
73+
errors: [{ column: 1, line: 1, messageId }],
74+
},
75+
{
76+
code: '// test("has title but no callback")',
77+
errors: [{ column: 1, line: 1, messageId }],
78+
},
79+
{
80+
code: dedent`
81+
foo()
82+
/*
83+
test.describe("has title but no callback", () => {})
84+
*/
85+
bar()
86+
`,
87+
errors: [{ column: 1, line: 2, messageId }],
88+
},
89+
// Global aliases
90+
{
91+
code: '// it("foo", () => {});',
92+
errors: [{ column: 1, line: 1, messageId }],
93+
settings: {
94+
playwright: {
95+
globalAliases: { test: ['it'] },
96+
},
97+
},
98+
},
99+
],
100+
valid: [
101+
'// foo("bar", function () {})',
102+
'test.describe("foo", function () {})',
103+
'test("foo", function () {})',
104+
'test.describe.only("foo", function () {})',
105+
'test.only("foo", function () {})',
106+
'test.skip("foo", function () {})',
107+
'test("foo", function () {})',
108+
'test.only("foo", function () {})',
109+
'test.concurrent("foo", function () {})',
110+
'var appliedSkip = describe.skip; appliedSkip.apply(describe)',
111+
'var calledSkip = test.skip; calledSkip.call(it)',
112+
'({ f: function () {} }).f()',
113+
'(a || b).f()',
114+
'testHappensToStartWithTest()',
115+
'testSomething()',
116+
'// latest(dates)',
117+
'// TODO: unify with Git implementation from Shipit (?)',
118+
'#!/usr/bin/env node',
119+
dedent`
120+
import { pending } from "actions"
121+
122+
test("foo", () => {
123+
expect(pending()).toEqual({})
124+
})
125+
`,
126+
dedent`
127+
const { pending } = require("actions")
128+
129+
test("foo", () => {
130+
expect(pending()).toEqual({})
131+
})
132+
`,
133+
dedent`
134+
test("foo", () => {
135+
const pending = getPending()
136+
expect(pending()).toEqual({})
137+
})
138+
`,
139+
dedent`
140+
test("foo", () => {
141+
expect(pending()).toEqual({})
142+
})
143+
144+
function pending() {
145+
return {}
146+
}
147+
`,
148+
// Global aliases
149+
{
150+
code: 'it("foo", () => {});',
151+
settings: {
152+
playwright: {
153+
globalAliases: { test: ['it'] },
154+
},
155+
},
156+
},
157+
],
158+
});

0 commit comments

Comments
 (0)