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: add no-confusing-set-time rule #1425

Merged
merged 14 commits into from
Sep 15, 2023
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ set to warn in.\
| [no-commented-out-tests](docs/rules/no-commented-out-tests.md) | Disallow commented out tests | | ✅ | | | |
| [no-conditional-expect](docs/rules/no-conditional-expect.md) | Disallow calling `expect` conditionally | ✅ | | | | |
| [no-conditional-in-test](docs/rules/no-conditional-in-test.md) | Disallow conditional logic in tests | | | | | |
| [no-confusing-set-timeout](docs/rules/no-confusing-set-timeout.md) | Disallow using confusing setTimeout in test | | | | | |
| [no-deprecated-functions](docs/rules/no-deprecated-functions.md) | Disallow use of deprecated functions | ✅ | | 🔧 | | |
| [no-disabled-tests](docs/rules/no-disabled-tests.md) | Disallow disabled tests | | ✅ | | | |
| [no-done-callback](docs/rules/no-done-callback.md) | Disallow using a callback in asynchronous tests and hooks | ✅ | | | 💡 | |
Expand Down
63 changes: 63 additions & 0 deletions docs/rules/no-confusing-set-timeout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# Disallow using confusing setTimeout in test (`no-confusing-set-timeout`)

<!-- end auto-generated rule header -->

`jest.setTimeout` can be called multiple times anywhere within a single test
file. However, only the last call will have an effect, and it will actually be
invoked before any other jest functions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`jest.setTimeout` can be called multiple times anywhere within a single test
file. However, only the last call will have an effect, and it will actually be
invoked before any other jest functions.
While `jest.setTimeout` can be called multiple times anywhere within a single test
file only the last call before any test functions run will have an effect.


## Rule details

This rule describes some tricky ways about `jest.setTimeout` that should not
SimenB marked this conversation as resolved.
Show resolved Hide resolved
recommend in Jest:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about this:

Suggested change
This rule describes some tricky ways about `jest.setTimeout` that should not
recommend in Jest:
This rule checks for several confusing usages of `jest.setTimeout` that looks like it applies to specific tests within the same file, such as:
- being called anywhere other than in global scope
- being called multiple times
- being called after other Jest functions like hooks, `describe`, `test`, or `it`


- should set `jest.setTimeout` in any testsuite methods before(such as
SimenB marked this conversation as resolved.
Show resolved Hide resolved
`describe`, `test` or `it`);
- should set `jest.setTimeout` in global scope.
- should only call `jest.setTimeout` once in a single test file;

Examples of **incorrect** code for this rule:

```js
describe('test foo', () => {
jest.setTimeout(1000);
it('test-description', () => {
// test logic;
});
});

describe('test bar', () => {
it('test-description', () => {
jest.setTimeout(1000);
// test logic;
});
});

test('foo-bar', () => {
jest.setTimeout(1000);
});

describe('unit test', () => {
beforeEach(() => {
jest.setTimeout(1000);
});
});
```

Examples of **correct** code for this rule:

```js
jest.setTimeout(500);
test('test test', () => {
// do some stuff
});
```

```js
jest.setTimeout(1000);
describe('test bar bar', () => {
it('test-description', () => {
// test logic;
});
});
```
1 change: 1 addition & 0 deletions src/__tests__/__snapshots__/rules.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ exports[`rules should export configs that refer to actual rules 1`] = `
"jest/no-commented-out-tests": "error",
"jest/no-conditional-expect": "error",
"jest/no-conditional-in-test": "error",
"jest/no-confusing-set-timeout": "error",
"jest/no-deprecated-functions": "error",
"jest/no-disabled-tests": "error",
"jest/no-done-callback": "error",
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/rules.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { existsSync } from 'fs';
import { resolve } from 'path';
import plugin from '../';

const numberOfRules = 52;
const numberOfRules = 53;
const ruleNames = Object.keys(plugin.rules);
const deprecatedRules = Object.entries(plugin.rules)
.filter(([, rule]) => rule.meta.deprecated)
Expand Down
242 changes: 242 additions & 0 deletions src/rules/__tests__/no-confusing-set-timeout.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,242 @@
import { TSESLint } from '@typescript-eslint/utils';
import dedent from 'dedent';
import rule from '../no-confusing-set-timeout';
import { espreeParser } from './test-utils';

const ruleTester = new TSESLint.RuleTester({
parser: espreeParser,
parserOptions: {
ecmaVersion: 2020,
},
});

ruleTester.run('no-confusing-set-timeout', rule, {
G-Rath marked this conversation as resolved.
Show resolved Hide resolved
valid: [
dedent`
jest.setTimeout(1000);
describe('A', () => {
beforeEach(async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.1', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.2', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
});
`,
dedent`
jest.setTimeout(1000);
window.setTimeout(6000)
describe('A', () => {
beforeEach(async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('test foo', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
});
`,
{
code: dedent`
import { handler } from 'dep/mod';
jest.setTimeout(800);
describe('A', () => {
beforeEach(async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.1', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.2', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
});
`,
parserOptions: { sourceType: 'module' },
},
dedent`
function handler() {}
jest.setTimeout(800);
describe('A', () => {
beforeEach(async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.1', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.2', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
});
`,
dedent`
const { handler } = require('dep/mod');
jest.setTimeout(800);
describe('A', () => {
beforeEach(async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.1', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.2', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
});
`,
dedent`
jest.setTimeout(1000);
window.setTimeout(60000);
`,
'window.setTimeout(60000);',
'setTimeout(1000);',
dedent`
jest.setTimeout(1000);
test('test case', () => {
setTimeout(() => {
Promise.resolv();
}, 5000);
});
`,
dedent`
test('test case', () => {
setTimeout(() => {
Promise.resolv();
}, 5000);
});
`,
],
invalid: [
{
code: dedent`
jest.setTimeout(1000);
setTimeout(1000);
window.setTimeout(1000);
describe('A', () => {
beforeEach(async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.1', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.2', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
});
jest.setTimeout(800);
`,
errors: [
{
messageId: 'orderSetTimeout',
line: 9,
column: 1,
},
{
messageId: 'multipleSetTimeouts',
line: 9,
column: 1,
},
],
},
{
code: dedent`
describe('A', () => {
jest.setTimeout(800);
beforeEach(async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.1', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.2', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
});
`,
errors: [
{
messageId: 'globalSetTimeout',
line: 2,
column: 3,
},
{
messageId: 'orderSetTimeout',
line: 2,
column: 3,
},
],
},
{
code: dedent`
describe('B', () => {
it('B.1', async () => {
await new Promise((resolve) => {
jest.setTimeout(1000);
setTimeout(resolve, 10000).unref();
});
});
it('B.2', async () => {
await new Promise((resolve) => { setTimeout(resolve, 10000).unref(); });
});
});
`,
errors: [
{
messageId: 'globalSetTimeout',
line: 4,
column: 7,
},
{
messageId: 'orderSetTimeout',
line: 4,
column: 7,
},
],
},
{
code: dedent`
test('test-suite', () => {
jest.setTimeout(1000);
});
`,
errors: [
{
messageId: 'globalSetTimeout',
line: 2,
column: 3,
},
{
messageId: 'orderSetTimeout',
line: 2,
column: 3,
},
],
},
{
code: dedent`
describe('A', () => {
beforeEach(async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.1', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.2', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
});
jest.setTimeout(1000);
`,
errors: [
{
messageId: 'orderSetTimeout',
line: 6,
column: 1,
},
],
},
{
code: dedent`
import { jest } from '@jest/globals';
{
jest.setTimeout(800);
}
describe('A', () => {
beforeEach(async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.1', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
it('A.2', async () => { await new Promise(resolve => { setTimeout(resolve, 10000).unref(); });});
});
`,
parserOptions: { sourceType: 'module' },
errors: [
{
messageId: 'globalSetTimeout',
line: 3,
column: 3,
},
],
},
{
code: dedent`
jest.setTimeout(800);
jest.setTimeout(900);
`,
errors: [
{
messageId: 'multipleSetTimeouts',
line: 2,
column: 1,
},
],
},
{
code: dedent`
expect(1 + 2).toEqual(3);
jest.setTimeout(800);
`,
errors: [
{
messageId: 'orderSetTimeout',
line: 2,
column: 1,
},
],
},
],
});
66 changes: 66 additions & 0 deletions src/rules/no-confusing-set-timeout.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import type { TSESTree } from '@typescript-eslint/utils';
import { createRule, getNodeName, parseJestFnCall } from './utils';

function isJestSetTimeout(node: TSESTree.Node) {
return getNodeName(node) === 'jest.setTimeout';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is not correct because it does not account for aliases i.e. this will not get caught:

import { jest as Jest } from '@jest/globals';
{
  Jest.setTimeout(800);
}

While a bit contrived, this is why parseJestFnCall exists and is what you should be checking instead:

function isJestSetTimeout(jestFnCall: ParsedJestFnCall) {
  return (
    jestFnCall.type === 'jest' &&
    jestFnCall.members.length === 1 &&
    isIdentifier(jestFnCall.members[0], 'setTimeout')
  );
}


export default createRule({
G-Rath marked this conversation as resolved.
Show resolved Hide resolved
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow using confusing setTimeout in test',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: 'Disallow using confusing setTimeout in test',
description: 'Disallow confusing usages of jest.setTimeout',

recommended: false,
},
messages: {
globalSetTimeout: '`jest.setTimeout` should be call in `global` scope.',
multipleSetTimeouts:
'Do not call `jest.setTimeout` multiple times, as only the last call will have an effect.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Linting messages shouldn't use full stops because they might appear in sentences constructed by IDEs and such:

Suggested change
globalSetTimeout: '`jest.setTimeout` should be call in `global` scope.',
multipleSetTimeouts:
'Do not call `jest.setTimeout` multiple times, as only the last call will have an effect.',
globalSetTimeout: '`jest.setTimeout` should be call in `global` scope',
multipleSetTimeouts:
'Do not call `jest.setTimeout` multiple times, as only the last call will have an effect',

orderSetTimeout:
'`jest.setTimeout` should be placed before any other jest methods',
},
type: 'problem',
schema: [],
},
defaultOptions: [],
create(context) {
let seenJestTimeout = false;
let shouldEmitOrderSetTimeout = false;

return {
CallExpression(node) {
G-Rath marked this conversation as resolved.
Show resolved Hide resolved
const scope = context.getScope();
const jestFnCall = parseJestFnCall(node, context);

if (!jestFnCall) {
return;
}

const result = isJestSetTimeout(node);
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's call this something like thisIsJestSetTimeoutCall - its a bit verbose, but there's no downside and I think its important to be clear given all the "is jest settimeout" names we've got going on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, naming makes my head burn out.


if (!result) {
if (jestFnCall.type !== 'unknown') {
shouldEmitOrderSetTimeout = true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

By definition there is no known Jest function call of type unknown - it only exists to gracefully handle if Jest ever did get a new type in older versions of the plugin (since we'd have to release a new version adding support); so you shouldn't worry about it here:

Suggested change
if (jestFnCall.type !== 'unknown') {
shouldEmitOrderSetTimeout = true;
}
shouldEmitOrderSetTimeout = true;


return;
}

if (!['global', 'module'].includes(scope.type)) {
context.report({ messageId: 'globalSetTimeout', node });
}

if (shouldEmitOrderSetTimeout) {
context.report({ messageId: 'orderSetTimeout', node });
}

if (seenJestTimeout) {
context.report({ messageId: 'multipleSetTimeouts', node });
} else {
seenJestTimeout = result;
}
},
};
},
});