diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index f1c2048f5..5f5576776 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -180,6 +180,7 @@ mod jest { pub mod prefer_to_contain; pub mod prefer_to_have_length; pub mod prefer_todo; + pub mod require_hook; pub mod require_to_throw_message; pub mod valid_describe_callback; pub mod valid_expect; @@ -525,6 +526,7 @@ oxc_macros::declare_all_lint_rules! { jest::prefer_to_contain, jest::prefer_to_have_length, jest::prefer_todo, + jest::require_hook, jest::require_to_throw_message, jest::valid_describe_callback, jest::valid_expect, diff --git a/crates/oxc_linter/src/rules/jest/require_hook.rs b/crates/oxc_linter/src/rules/jest/require_hook.rs new file mode 100644 index 000000000..87d83c440 --- /dev/null +++ b/crates/oxc_linter/src/rules/jest/require_hook.rs @@ -0,0 +1,632 @@ +use oxc_allocator::Vec as OxcVec; +use oxc_ast::{ + ast::{Argument, Declaration, Expression, Statement, VariableDeclarationKind}, + AstKind, +}; +use oxc_diagnostics::{ + miette::{self, Diagnostic}, + thiserror::Error, +}; +use oxc_macros::declare_oxc_lint; +use oxc_semantic::AstNode; +use oxc_span::Span; + +use crate::{ + context::LintContext, + rule::Rule, + utils::{ + get_node_name, is_type_of_jest_fn_call, parse_jest_fn_call, JestFnKind, JestGeneralFnKind, + PossibleJestNode, + }, +}; + +#[derive(Debug, Error, Diagnostic)] +#[error("eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook.")] +#[diagnostic(severity(warning), help("This should be done within a hook"))] +struct UseHook(#[label] pub Span); + +#[derive(Debug, Default, Clone)] +pub struct RequireHookConfig { + allowed_function_calls: Vec, +} + +#[derive(Debug, Default, Clone)] +pub struct RequireHook(Box); + +impl std::ops::Deref for RequireHook { + type Target = RequireHookConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +declare_oxc_lint!( + /// ### What it does + /// This rule flags any expression that is either at the toplevel of a test file or + /// directly within the body of a `describe`, _except_ for the following: + /// + /// - `import` statements + /// - `const` variables + /// - `let` _declarations_, and initializations to `null` or `undefined` + /// - Classes + /// - Types + /// - Calls to the standard Jest globals + /// + /// ### Example + /// ```javascript + /// // invalid + /// import { database, isCity } from '../database'; + /// import { Logger } from '../../../src/Logger'; + /// import { loadCities } from '../api'; + /// + /// jest.mock('../api'); + /// + /// const initializeCityDatabase = () => { + /// database.addCity('Vienna'); + /// database.addCity('San Juan'); + /// database.addCity('Wellington'); + /// }; + /// + /// const clearCityDatabase = () => { + /// database.clear(); + /// }; + /// + /// initializeCityDatabase(); + /// + /// test('that persists cities', () => { + /// expect(database.cities.length).toHaveLength(3); + /// }); + /// test('city database has Vienna', () => { + /// expect(isCity('Vienna')).toBeTruthy(); + /// }); + /// + /// test('city database has San Juan', () => { + /// expect(isCity('San Juan')).toBeTruthy(); + /// }); + /// + /// describe('when loading cities from the api', () => { + /// let consoleWarnSpy = jest.spyOn(console, 'warn'); + /// loadCities.mockResolvedValue(['Wellington', 'London']); + /// + /// it('does not duplicate cities', async () => { + /// await database.loadCities(); + /// expect(database.cities).toHaveLength(4); + /// }); + /// }); + /// clearCityDatabase(); + /// + /// // valid + /// import { database, isCity } from '../database'; + /// import { Logger } from '../../../src/Logger'; + /// import { loadCities } from '../api'; + /// + /// jest.mock('../api'); + /// const initializeCityDatabase = () => { + /// database.addCity('Vienna'); + /// database.addCity('San Juan'); + /// database.addCity('Wellington'); + /// }; + /// + /// const clearCityDatabase = () => { + /// database.clear(); + /// }; + /// + /// beforeEach(() => { + /// initializeCityDatabase(); + /// }); + /// + /// test('that persists cities', () => { + /// expect(database.cities.length).toHaveLength(3); + /// }); + /// + /// test('city database has Vienna', () => { + /// expect(isCity('Vienna')).toBeTruthy(); + /// }); + /// + /// test('city database has San Juan', () => { + /// expect(isCity('San Juan')).toBeTruthy(); + /// }); + /// + /// describe('when loading cities from the api', () => { + /// let consoleWarnSpy; + /// beforeEach(() => { + /// consoleWarnSpy = jest.spyOn(console, 'warn'); + /// loadCities.mockResolvedValue(['Wellington', 'London']); + /// }); + /// + /// it('does not duplicate cities', async () => { + /// await database.loadCities(); + /// expect(database.cities).toHaveLength(4); + /// }); + /// }); + /// afterEach(() => { + /// clearCityDatabase(); + /// }); + /// ``` + /// + RequireHook, + style +); + +impl Rule for RequireHook { + fn from_configuration(value: serde_json::Value) -> Self { + let allowed_function_calls = value + .get(0) + .and_then(|config| config.get("allowedFunctionCalls")) + .and_then(serde_json::Value::as_array) + .map(|v| { + v.iter().filter_map(serde_json::Value::as_str).map(ToString::to_string).collect() + }) + .unwrap_or_default(); + + Self(Box::new(RequireHookConfig { allowed_function_calls })) + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let kind = node.kind(); + + if let AstKind::Program(program) = kind { + self.check_block_body(node, &program.body, ctx); + } else if let AstKind::CallExpression(call_expr) = kind { + if !is_type_of_jest_fn_call( + call_expr, + &PossibleJestNode { node, original: None }, + ctx, + &[JestFnKind::General(JestGeneralFnKind::Describe)], + ) || call_expr.arguments.len() < 2 + { + return; + } + + let Some(Argument::Expression(second_arg_expr)) = call_expr.arguments.get(1) else { + return; + }; + + match second_arg_expr { + Expression::FunctionExpression(func_expr) => { + if let Some(func_body) = &func_expr.body { + self.check_block_body(node, &func_body.statements, ctx); + }; + } + Expression::ArrowFunctionExpression(arrow_func_expr) => { + if !arrow_func_expr.expression { + self.check_block_body(node, &arrow_func_expr.body.statements, ctx); + } + } + _ => (), + } + } + } +} + +impl RequireHook { + fn check_block_body<'a>( + &self, + node: &AstNode<'a>, + statements: &'a OxcVec<'a, Statement<'_>>, + ctx: &LintContext<'a>, + ) { + for stmt in statements { + self.check(node, stmt, ctx); + } + } + + fn check<'a>(&self, node: &AstNode<'a>, stmt: &'a Statement<'_>, ctx: &LintContext<'a>) { + if let Statement::ExpressionStatement(expr_stmt) = stmt { + self.check_should_report_in_hook(node, &expr_stmt.expression, ctx); + } else if let Statement::Declaration(Declaration::VariableDeclaration(var_decl)) = stmt { + if var_decl.kind != VariableDeclarationKind::Const + && var_decl.declarations.iter().any(|decl| { + let Some(init_call) = &decl.init else { + return false; + }; + !init_call.is_null_or_undefined() + }) + { + ctx.diagnostic(UseHook(var_decl.span)); + } + } + } + + fn check_should_report_in_hook<'a>( + &self, + node: &AstNode<'a>, + expr: &'a Expression<'a>, + ctx: &LintContext<'a>, + ) { + if let Expression::CallExpression(call_expr) = expr { + let name: String = get_node_name(&call_expr.callee); + + if !(parse_jest_fn_call(call_expr, &PossibleJestNode { node, original: None }, ctx) + .is_some() + || name.starts_with("jest.") + || self.allowed_function_calls.contains(&name)) + { + ctx.diagnostic(UseHook(call_expr.span)); + } + } + } +} + +#[test] +fn tests() { + use crate::tester::Tester; + + let pass = vec![ + ("describe()", None), + ("describe(\"just a title\")", None), + ( + " + describe('a test', () => + test('something', () => { + expect(true).toBe(true); + }) + ); + ", + None, + ), + ( + " + test('it', () => { + // + }); + ", + None, + ), + ( + " + const { myFn } = require('../functions'); + + test('myFn', () => { + expect(myFn()).toBe(1); + }); + ", + None, + ), + ( + " + import { myFn } from '../functions'; + test('myFn', () => { + expect(myFn()).toBe(1); + }); + ", + None, + ), + ( + " + class MockLogger { + log() {} + } + + test('myFn', () => { + expect(myFn()).toBe(1); + }); + ", + None, + ), + ( + " + const { myFn } = require('../functions'); + + describe('myFn', () => { + it('returns one', () => { + expect(myFn()).toBe(1); + }); + }); + ", + None, + ), + ( + " + const { myFn } = require('../functions'); + + describe('myFn', function () { + it('returns one', () => { + expect(myFn()).toBe(1); + }); + }); + ", + None, + ), + ( + " + describe('some tests', () => { + it('is true', () => { + expect(true).toBe(true); + }); + }); + ", + None, + ), + ( + " + describe('some tests', () => { + it('is true', () => { + expect(true).toBe(true); + }); + + describe('more tests', () => { + it('is false', () => { + expect(true).toBe(false); + }); + }); + }); + ", + None, + ), + ( + " + describe('some tests', () => { + let consoleLogSpy; + + beforeEach(() => { + consoleLogSpy = jest.spyOn(console, 'log'); + }); + + it('prints a message', () => { + printMessage('hello world'); + expect(consoleLogSpy).toHaveBeenCalledWith('hello world'); + }); + }); + ", + None, + ), + ( + " + let consoleErrorSpy = null; + + beforeEach(() => { + consoleErrorSpy = jest.spyOn(console, 'error'); + }); + ", + None, + ), + ( + " + let consoleErrorSpy = undefined; + + beforeEach(() => { + consoleErrorSpy = jest.spyOn(console, 'error'); + }); + ", + None, + ), + ( + " + describe('some tests', () => { + beforeEach(() => { + setup(); + }); + }); + ", + None, + ), + ( + " + beforeEach(() => { + initializeCityDatabase(); + }); + + afterEach(() => { + clearCityDatabase(); + }); + + test('city database has Vienna', () => { + expect(isCity('Vienna')).toBeTruthy(); + }); + + test('city database has San Juan', () => { + expect(isCity('San Juan')).toBeTruthy(); + }); + ", + None, + ), + ( + " + describe('cities', () => { + beforeEach(() => { + initializeCityDatabase(); + }); + + test('city database has Vienna', () => { + expect(isCity('Vienna')).toBeTruthy(); + }); + + test('city database has San Juan', () => { + expect(isCity('San Juan')).toBeTruthy(); + }); + + afterEach(() => { + clearCityDatabase(); + }); + }); + ", + None, + ), + ( + " + enableAutoDestroy(afterEach); + + describe('some tests', () => { + it('is false', () => { + expect(true).toBe(true); + }); + }); + ", + Some(serde_json::json!([{ "allowedFunctionCalls": ["enableAutoDestroy"] }])), + ), + ( + " + import { myFn } from '../functions'; + + // todo: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/56545 + declare module 'eslint' { + namespace ESLint { + interface LintResult { + fatalErrorCount: number; + } + } + } + + test('myFn', () => { + expect(myFn()).toBe(1); + }); + ", + None, + ), + ]; + + let fail = vec![ + ("setup();", None), + ( + " + describe('some tests', () => { + setup(); + }); + ", + None, + ), + ( + " + let { setup } = require('./test-utils'); + + describe('some tests', () => { + setup(); + }); + ", + None, + ), + ( + " + describe('some tests', () => { + setup(); + + it('is true', () => { + expect(true).toBe(true); + }); + + describe('more tests', () => { + setup(); + + it('is false', () => { + expect(true).toBe(false); + }); + }); + }); + ", + None, + ), + ( + " + let consoleErrorSpy = jest.spyOn(console, 'error'); + + describe('when loading cities from the api', () => { + let consoleWarnSpy = jest.spyOn(console, 'warn'); + }); + ", + None, + ), + ( + " + let consoleErrorSpy = null; + + describe('when loading cities from the api', () => { + let consoleWarnSpy = jest.spyOn(console, 'warn'); + }); + ", + None, + ), + ("let value = 1", None), + ("let consoleErrorSpy, consoleWarnSpy = jest.spyOn(console, 'error');", None), + ("let consoleErrorSpy = jest.spyOn(console, 'error'), consoleWarnSpy;", None), + ( + " + import { database, isCity } from '../database'; + import { loadCities } from '../api'; + + jest.mock('../api'); + + const initializeCityDatabase = () => { + database.addCity('Vienna'); + database.addCity('San Juan'); + database.addCity('Wellington'); + }; + + const clearCityDatabase = () => { + database.clear(); + }; + + initializeCityDatabase(); + + test('that persists cities', () => { + expect(database.cities.length).toHaveLength(3); + }); + + test('city database has Vienna', () => { + expect(isCity('Vienna')).toBeTruthy(); + }); + + test('city database has San Juan', () => { + expect(isCity('San Juan')).toBeTruthy(); + }); + + describe('when loading cities from the api', () => { + let consoleWarnSpy = jest.spyOn(console, 'warn'); + + loadCities.mockResolvedValue(['Wellington', 'London']); + + it('does not duplicate cities', async () => { + await database.loadCities(); + + expect(database.cities).toHaveLength(4); + }); + + it('logs any duplicates', async () => { + await database.loadCities(); + + expect(consoleWarnSpy).toHaveBeenCalledWith( + 'Ignored duplicate cities: Wellington', + ); + }); + }); + + clearCityDatabase(); + ", + None, + ), + ( + " + enableAutoDestroy(afterEach); + + describe('some tests', () => { + it('is false', () => { + expect(true).toBe(true); + }); + }); + ", + Some(serde_json::json!([{ "allowedFunctionCalls": ["someOtherName"] }])), + ), + ( + " + import { setup } from '../test-utils'; + + // todo: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/56545 + declare module 'eslint' { + namespace ESLint { + interface LintResult { + fatalErrorCount: number; + } + } + } + + describe('some tests', () => { + setup(); + }); + ", + None, + ), + ]; + + Tester::new(RequireHook::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/require_hook.snap b/crates/oxc_linter/src/snapshots/require_hook.snap new file mode 100644 index 000000000..3507904ae --- /dev/null +++ b/crates/oxc_linter/src/snapshots/require_hook.snap @@ -0,0 +1,158 @@ +--- +source: crates/oxc_linter/src/tester.rs +assertion_line: 157 +expression: require_hook +--- + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:1:1] + 1 │ setup(); + · ─────── + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:3:21] + 2 │ describe('some tests', () => { + 3 │ setup(); + · ─────── + 4 │ }); + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:2:17] + 1 │ + 2 │ let { setup } = require('./test-utils'); + · ──────────────────────────────────────── + 3 │ + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:5:21] + 4 │ describe('some tests', () => { + 5 │ setup(); + · ─────── + 6 │ }); + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:3:21] + 2 │ describe('some tests', () => { + 3 │ setup(); + · ─────── + 4 │ + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:10:25] + 9 │ describe('more tests', () => { + 10 │ setup(); + · ─────── + 11 │ + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:2:17] + 1 │ + 2 │ let consoleErrorSpy = jest.spyOn(console, 'error'); + · ─────────────────────────────────────────────────── + 3 │ + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:5:21] + 4 │ describe('when loading cities from the api', () => { + 5 │ let consoleWarnSpy = jest.spyOn(console, 'warn'); + · ───────────────────────────────────────────────── + 6 │ }); + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:5:19] + 4 │ describe('when loading cities from the api', () => { + 5 │ let consoleWarnSpy = jest.spyOn(console, 'warn'); + · ───────────────────────────────────────────────── + 6 │ }); + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:1:1] + 1 │ let value = 1 + · ───────────── + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:1:1] + 1 │ let consoleErrorSpy, consoleWarnSpy = jest.spyOn(console, 'error'); + · ─────────────────────────────────────────────────────────────────── + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:1:1] + 1 │ let consoleErrorSpy = jest.spyOn(console, 'error'), consoleWarnSpy; + · ─────────────────────────────────────────────────────────────────── + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:17:17] + 16 │ + 17 │ initializeCityDatabase(); + · ──────────────────────── + 18 │ + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:51:17] + 50 │ + 51 │ clearCityDatabase(); + · ─────────────────── + 52 │ + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:32:21] + 31 │ describe('when loading cities from the api', () => { + 32 │ let consoleWarnSpy = jest.spyOn(console, 'warn'); + · ───────────────────────────────────────────────── + 33 │ + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:34:21] + 33 │ + 34 │ loadCities.mockResolvedValue(['Wellington', 'London']); + · ────────────────────────────────────────────────────── + 35 │ + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:2:17] + 1 │ + 2 │ enableAutoDestroy(afterEach); + · ──────────────────────────── + 3 │ + ╰──── + help: This should be done within a hook + + ⚠ eslint-plugin-jest(require-hook): Require setup and teardown code to be within a hook. + ╭─[require_hook.tsx:14:21] + 13 │ describe('some tests', () => { + 14 │ setup(); + · ─────── + 15 │ }); + ╰──── + help: This should be done within a hook