Skip to content

Commit 7344607

Browse files
SimenBSimenB
authored
feat(rules): add no-test-callback rule (#179)
1 parent 9a6ce6c commit 7344607

File tree

5 files changed

+287
-0
lines changed

5 files changed

+287
-0
lines changed

‎README.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ for more information about extending configuration files.
9191
| [no-jasmine-globals][] | Disallow Jasmine globals | | ![fixable-yellow][] |
9292
| [no-jest-import][] | Disallow importing `jest` | ![recommended][] | |
9393
| [no-large-snapshots][] | Disallow large snapshots | | |
94+
| [no-test-callback][] | Using a callback in asynchronous tests | | ![fixable-green][] |
9495
| [no-test-prefixes][] | Disallow using `f` & `x` prefixes to define focused/skipped tests | | ![fixable-green][] |
9596
| [no-test-return-statement][] | Disallow explicitly returning from tests | | |
9697
| [prefer-expect-assertions][] | Suggest using `expect.assertions()` OR `expect.hasAssertions()` | | |
@@ -121,6 +122,7 @@ for more information about extending configuration files.
121122
[no-jasmine-globals]: docs/rules/no-jasmine-globals.md
122123
[no-jest-import]: docs/rules/no-jest-import.md
123124
[no-large-snapshots]: docs/rules/no-large-snapshots.md
125+
[no-test-callback]: docs/rules/no-test-callback.md
124126
[no-test-prefixes]: docs/rules/no-test-prefixes.md
125127
[no-test-return-statement]: docs/rules/no-test-return-statement.md
126128
[prefer-expect-assertions]: docs/rules/prefer-expect-assertions.md

‎docs/rules/no-test-callback.md‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
# Avoid using a callback in asynchronous tests (no-test-callback)
2+
3+
Jest allows you to pass a callback to test definitions, typically called `done`,
4+
that is later invoked to indicate that the asynchronous test is complete.
5+
6+
However, that means that if your test throws (e.g. because of a failing
7+
assertion), `done` will never be called unless you manually use `try-catch`.
8+
9+
```js
10+
test('some test', done => {
11+
expect(false).toBe(true);
12+
done();
13+
});
14+
```
15+
16+
The test above will time out instead of failing the assertions, since `done` is
17+
never called.
18+
19+
Correct way of doing the same thing is to wrap it in `try-catch`.
20+
21+
```js
22+
test('some test', done => {
23+
try {
24+
expect(false).toBe(true);
25+
done();
26+
} catch (e) {
27+
done(e);
28+
}
29+
});
30+
```
31+
32+
However, Jest supports a second way of having asynchronous tests - using
33+
promises.
34+
35+
```js
36+
test('some test', () => {
37+
return new Promise(done => {
38+
expect(false).toBe(true);
39+
done();
40+
});
41+
});
42+
```
43+
44+
Even though `done` is never called here, the Promise will still reject, and Jest
45+
will report the assertion error correctly.
46+
47+
## Rule details
48+
49+
This rule triggers a warning if you have a `done` callback in your test.
50+
51+
The following patterns are considered warnings:
52+
53+
```js
54+
test('myFunction()', done => {
55+
// ...
56+
});
57+
58+
test('myFunction()', function(done) {
59+
// ...
60+
});
61+
```
62+
63+
The following patterns are not considered warnings:
64+
65+
```js
66+
test('myFunction()', () => {
67+
expect(myFunction()).toBeTruthy();
68+
});
69+
70+
test('myFunction()', () => {
71+
return new Promise(done => {
72+
expect(myFunction()).toBeTruthy();
73+
done();
74+
});
75+
});
76+
```

‎index.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ const preferInlineSnapshots = require('./rules/prefer-inline-snapshots');
2424
const preferStrictEqual = require('./rules/prefer-strict-equal');
2525
const requireTothrowMessage = require('./rules/require-tothrow-message');
2626
const noAliasMethods = require('./rules/no-alias-methods');
27+
const noTestCallback = require('./rules/no-test-callback');
2728

2829
const snapshotProcessor = require('./processors/snapshot-processor');
2930

@@ -95,5 +96,6 @@ module.exports = {
9596
'prefer-strict-equal': preferStrictEqual,
9697
'require-tothrow-message': requireTothrowMessage,
9798
'no-alias-methods': noAliasMethods,
99+
'no-test-callback': noTestCallback,
98100
},
99101
};

‎rules/__tests__/no-test-callback.test.js‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
'use strict';
2+
3+
const RuleTester = require('eslint').RuleTester;
4+
const rule = require('../no-test-callback');
5+
6+
const ruleTester = new RuleTester({
7+
parserOptions: {
8+
ecmaVersion: 8,
9+
},
10+
});
11+
12+
ruleTester.run('no-test-callback', rule, {
13+
valid: [
14+
'test("something", () => {})',
15+
'test("something", async () => {})',
16+
'test("something", function() {})',
17+
'test("something", async function () {})',
18+
'test("something", someArg)',
19+
],
20+
invalid: [
21+
{
22+
code: 'test("something", done => {done();})',
23+
errors: [
24+
{
25+
message: 'Illegal usage of test callback',
26+
line: 1,
27+
column: 19,
28+
},
29+
],
30+
output:
31+
'test("something", () => {return new Promise(done => {done();})})',
32+
},
33+
{
34+
code: 'test("something", (done) => {done();})',
35+
errors: [
36+
{
37+
message: 'Illegal usage of test callback',
38+
line: 1,
39+
column: 20,
40+
},
41+
],
42+
output:
43+
'test("something", () => {return new Promise((done) => {done();})})',
44+
},
45+
{
46+
code: 'test("something", done => done())',
47+
errors: [
48+
{
49+
message: 'Illegal usage of test callback',
50+
line: 1,
51+
column: 19,
52+
},
53+
],
54+
output: 'test("something", () => new Promise(done => done()))',
55+
},
56+
{
57+
code: 'test("something", (done) => done())',
58+
errors: [
59+
{
60+
message: 'Illegal usage of test callback',
61+
line: 1,
62+
column: 20,
63+
},
64+
],
65+
output: 'test("something", () => new Promise((done) => done()))',
66+
},
67+
{
68+
code: 'test("something", function(done) {done();})',
69+
errors: [
70+
{
71+
message: 'Illegal usage of test callback',
72+
line: 1,
73+
column: 28,
74+
},
75+
],
76+
output:
77+
'test("something", function() {return new Promise((done) => {done();})})',
78+
},
79+
{
80+
code: 'test("something", function (done) {done();})',
81+
errors: [
82+
{
83+
message: 'Illegal usage of test callback',
84+
line: 1,
85+
column: 29,
86+
},
87+
],
88+
output:
89+
'test("something", function () {return new Promise((done) => {done();})})',
90+
},
91+
{
92+
code: 'test("something", async done => {done();})',
93+
errors: [
94+
{
95+
message: 'Illegal usage of test callback',
96+
line: 1,
97+
column: 25,
98+
},
99+
],
100+
output:
101+
'test("something", async () => {await new Promise(done => {done();})})',
102+
},
103+
{
104+
code: 'test("something", async done => done())',
105+
errors: [
106+
{
107+
message: 'Illegal usage of test callback',
108+
line: 1,
109+
column: 25,
110+
},
111+
],
112+
output: 'test("something", async () => new Promise(done => done()))',
113+
},
114+
{
115+
code: 'test("something", async function (done) {done();})',
116+
errors: [
117+
{
118+
message: 'Illegal usage of test callback',
119+
line: 1,
120+
column: 35,
121+
},
122+
],
123+
output:
124+
'test("something", async function () {await new Promise((done) => {done();})})',
125+
},
126+
],
127+
});

‎rules/no-test-callback.js‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
'use strict';
2+
3+
const getDocsUrl = require('./util').getDocsUrl;
4+
const isTestCase = require('./util').isTestCase;
5+
6+
module.exports = {
7+
meta: {
8+
docs: {
9+
url: getDocsUrl(__filename),
10+
},
11+
fixable: 'code',
12+
},
13+
create(context) {
14+
return {
15+
CallExpression(node) {
16+
if (!isTestCase(node) || node.arguments.length !== 2) {
17+
return;
18+
}
19+
20+
const callback = node.arguments[1];
21+
22+
if (
23+
!/^(Arrow)?FunctionExpression$/.test(callback.type) ||
24+
callback.params.length !== 1
25+
) {
26+
return;
27+
}
28+
29+
const argument = callback.params[0];
30+
context.report({
31+
node: argument,
32+
message: 'Illegal usage of test callback',
33+
fix(fixer) {
34+
const sourceCode = context.getSourceCode();
35+
const body = callback.body;
36+
const firstBodyToken = sourceCode.getFirstToken(body);
37+
const lastBodyToken = sourceCode.getLastToken(body);
38+
const tokenBeforeArgument = sourceCode.getTokenBefore(argument);
39+
const tokenAfterArgument = sourceCode.getTokenAfter(argument);
40+
const argumentInParens =
41+
tokenBeforeArgument.value === '(' &&
42+
tokenAfterArgument.value === ')';
43+
44+
let argumentFix = fixer.replaceText(argument, '()');
45+
46+
if (argumentInParens) {
47+
argumentFix = fixer.remove(argument);
48+
}
49+
50+
let newCallback = argument.name;
51+
52+
if (argumentInParens) {
53+
newCallback = `(${newCallback})`;
54+
}
55+
56+
let beforeReplacement = `new Promise(${newCallback} => `;
57+
let afterReplacement = ')';
58+
let replaceBefore = true;
59+
60+
if (body.type === 'BlockStatement') {
61+
const keyword = callback.async ? 'await' : 'return';
62+
63+
beforeReplacement = `${keyword} ${beforeReplacement}{`;
64+
afterReplacement += '}';
65+
replaceBefore = false;
66+
}
67+
68+
return [
69+
argumentFix,
70+
replaceBefore
71+
? fixer.insertTextBefore(firstBodyToken, beforeReplacement)
72+
: fixer.insertTextAfter(firstBodyToken, beforeReplacement),
73+
fixer.insertTextAfter(lastBodyToken, afterReplacement),
74+
];
75+
},
76+
});
77+
},
78+
};
79+
},
80+
};

0 commit comments

Comments
 (0)