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(linter/vitest): implement prefer-to-be-object #5321

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/oxc_linter/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ mod vitest {
pub mod no_import_node_test;
pub mod prefer_each;
pub mod prefer_to_be_falsy;
pub mod prefer_to_be_object;
pub mod prefer_to_be_truthy;
pub mod require_local_test_context_for_concurrent_snapshots;
}
Expand Down Expand Up @@ -879,6 +880,7 @@ oxc_macros::declare_all_lint_rules! {
vitest::no_import_node_test,
vitest::prefer_each,
vitest::prefer_to_be_falsy,
vitest::prefer_to_be_object,
vitest::prefer_to_be_truthy,
vitest::no_conditional_tests,
vitest::require_local_test_context_for_concurrent_snapshots,
Expand Down
233 changes: 233 additions & 0 deletions crates/oxc_linter/src/rules/vitest/prefer_to_be_object.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,233 @@
use oxc_ast::{
ast::{Argument, BinaryOperator, Expression},
AstKind,
};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::{GetSpan, Span};

use crate::{
context::LintContext,
rule::Rule,
utils::{
collect_possible_jest_call_node, parse_jest_fn_call, KnownMemberExpressionProperty,
ParsedExpectFnCall, ParsedJestFnCallNew, PossibleJestNode,
},
};

fn prefer_to_be_object(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Prefer `toBeObject()` for object assertions")
.with_help("Consider using `toBeObject()` to test if a value is an object.")
.with_label(span)
}

fn is_parsed_instance_of_matcher_call(
parsed_expect_call: &ParsedExpectFnCall,
matcher: &KnownMemberExpressionProperty,
) -> bool {
parsed_expect_call.args.len() == 1
&& matches!(
parsed_expect_call.args.first(),
Some(Argument::Identifier(id)) if matcher.name().as_deref() == Some("toBeInstanceOf") && id.name == "Object"
)
}

#[derive(Debug, Default, Clone)]
pub struct PreferToBeObject;

declare_oxc_lint!(
/// ### What it does
///
/// This rule enforces using `toBeObject()` to check if a value is of type `Object`.
///
/// ### Why is this bad?
///
/// Using other methods such as `toBeInstanceOf(Object)` or `instanceof Object` can be less clear and potentially misleading. Enforcing the use of `toBeObject()` provides more explicit and readable code, making your intentions clear and improving the overall maintainability and readability of your tests.
///
/// ### Examples
///
/// Examples of **incorrect** code for this rule:
/// ```js
/// expectTypeOf({}).toBeInstanceOf(Object);
/// expectTypeOf({} instanceof Object).toBeTruthy();
/// ```
///
/// Examples of **correct** code for this rule:
/// ```js
/// expectTypeOf({}).toBeObject();
/// expectTypeOf({}).toBeObject();
/// ```
PreferToBeObject,
style,
fix
);

impl Rule for PreferToBeObject {
fn run_once(&self, ctx: &LintContext) {
for possible_vitest_node in &collect_possible_jest_call_node(ctx) {
Self::run(possible_vitest_node, ctx);
}
}
}

impl PreferToBeObject {
fn run<'a>(possible_vitest_node: &PossibleJestNode<'a, '_>, ctx: &LintContext<'a>) {
let node = possible_vitest_node.node;
let AstKind::CallExpression(call_expr) = node.kind() else {
return;
};

let Some(ParsedJestFnCallNew::ExpectTypeOf(parsed_expect_call)) =
parse_jest_fn_call(call_expr, possible_vitest_node, ctx)
else {
return;
};

let Some(matcher) = parsed_expect_call.matcher() else {
return;
};

if is_parsed_instance_of_matcher_call(&parsed_expect_call, matcher) {
ctx.diagnostic_with_fix(prefer_to_be_object(matcher.span), |fixer| {
fixer.replace(Span::new(matcher.span.start, call_expr.span.end), "toBeObject()")
});
return;
}

if matches!(matcher.name().as_deref(), Some("toBeTruthy" | "toBeFalsy")) {
let Some(Expression::CallExpression(parent_call_expr)) = parsed_expect_call.head.parent
else {
return;
};

let Some(arg) = parent_call_expr.arguments.first() else {
return;
};

let expr = match &arg {
Argument::ParenthesizedExpression(paren_expr) => &paren_expr.expression,
_ => arg.to_expression(),
};

let Expression::BinaryExpression(binary_expr) = expr else {
return;
};

if binary_expr.operator != BinaryOperator::Instanceof {
return;
}

let Expression::Identifier(id) = &binary_expr.right else {
return;
};

if id.name == "Object" {
ctx.diagnostic_with_fix(prefer_to_be_object(matcher.span), |fixer| {
let mut formatter = fixer.codegen();
formatter.print_str(fixer.source_range(Span::new(
call_expr.span.start,
binary_expr.left.span().end,
)));
formatter.print_str(
fixer.source_range(Span::new(
binary_expr.span.end,
parent_call_expr.span.end,
)),
);

let not_modifier = parsed_expect_call
.modifiers()
.iter()
.any(|node| node.name().as_deref() == Some("not"));

if (matcher.name().as_deref() == Some("toBeFalsy")) != not_modifier {
formatter.print_str(".not");
}

formatter.print_str(".toBeObject()");

fixer.replace(call_expr.span, formatter)
});
}
}
}
}

#[test]
fn test() {
use crate::tester::Tester;

let pass = vec![
"expectTypeOf.hasAssertions",
"expectTypeOf.hasAssertions()",
"expectTypeOf",
"expectTypeOf().not",
"expectTypeOf().toBe",
"expectTypeOf().toBe(true)",
"expectTypeOf({}).toBe(true)",
"expectTypeOf({}).toBeObject()",
"expectTypeOf({}).not.toBeObject()",
"expectTypeOf([] instanceof Array).not.toBeObject()",
"expectTypeOf({}).not.toBeInstanceOf(Array)",
];

let fail = vec![
// "expectTypeOf(({} instanceof Object)).toBeTruthy();",
"expectTypeOf({} instanceof Object).toBeTruthy();",
"expectTypeOf({} instanceof Object).not.toBeTruthy();",
"expectTypeOf({} instanceof Object).toBeFalsy();",
"expectTypeOf({} instanceof Object).not.toBeFalsy();",
"expectTypeOf({}).toBeInstanceOf(Object);",
"expectTypeOf({}).not.toBeInstanceOf(Object);",
"expectTypeOf(requestValues()).resolves.toBeInstanceOf(Object);",
"expectTypeOf(queryApi()).resolves.not.toBeInstanceOf(Object);",
];

let fix = vec![
(
"expectTypeOf(({} instanceof Object)).toBeTruthy();",
"expectTypeOf(({})).toBeObject();",
None,
),
(
"expectTypeOf({} instanceof Object).toBeTruthy();",
"expectTypeOf({}).toBeObject();",
None,
),
(
"expectTypeOf({} instanceof Object).not.toBeTruthy();",
"expectTypeOf({}).not.toBeObject();",
None,
),
(
"expectTypeOf({} instanceof Object).toBeFalsy();",
"expectTypeOf({}).not.toBeObject();",
None,
),
(
"expectTypeOf({} instanceof Object).not.toBeFalsy();",
"expectTypeOf({}).toBeObject();",
None,
),
("expectTypeOf({}).toBeInstanceOf(Object);", "expectTypeOf({}).toBeObject();", None),
(
"expectTypeOf({}).not.toBeInstanceOf(Object);",
"expectTypeOf({}).not.toBeObject();",
None,
),
(
"expectTypeOf(requestValues()).resolves.toBeInstanceOf(Object);",
"expectTypeOf(requestValues()).resolves.toBeObject();",
None,
),
(
"expectTypeOf(queryApi()).resolves.not.toBeInstanceOf(Object);",
"expectTypeOf(queryApi()).resolves.not.toBeObject();",
None,
),
];
Tester::new(PreferToBeObject::NAME, pass, fail)
.with_vitest_plugin(true)
.expect_fix(fix)
.test_and_snapshot();
}
58 changes: 58 additions & 0 deletions crates/oxc_linter/src/snapshots/prefer_to_be_object.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
---
source: crates/oxc_linter/src/tester.rs
---
⚠ eslint-plugin-vitest(prefer-to-be-object): Prefer `toBeObject()` for object assertions
╭─[prefer_to_be_object.tsx:1:36]
1 │ expectTypeOf({} instanceof Object).toBeTruthy();
· ──────────
╰────
help: Consider using `toBeObject()` to test if a value is an object.

⚠ eslint-plugin-vitest(prefer-to-be-object): Prefer `toBeObject()` for object assertions
╭─[prefer_to_be_object.tsx:1:40]
1 │ expectTypeOf({} instanceof Object).not.toBeTruthy();
· ──────────
╰────
help: Consider using `toBeObject()` to test if a value is an object.

⚠ eslint-plugin-vitest(prefer-to-be-object): Prefer `toBeObject()` for object assertions
╭─[prefer_to_be_object.tsx:1:36]
1 │ expectTypeOf({} instanceof Object).toBeFalsy();
· ─────────
╰────
help: Consider using `toBeObject()` to test if a value is an object.

⚠ eslint-plugin-vitest(prefer-to-be-object): Prefer `toBeObject()` for object assertions
╭─[prefer_to_be_object.tsx:1:40]
1 │ expectTypeOf({} instanceof Object).not.toBeFalsy();
· ─────────
╰────
help: Consider using `toBeObject()` to test if a value is an object.

⚠ eslint-plugin-vitest(prefer-to-be-object): Prefer `toBeObject()` for object assertions
╭─[prefer_to_be_object.tsx:1:18]
1 │ expectTypeOf({}).toBeInstanceOf(Object);
· ──────────────
╰────
help: Consider using `toBeObject()` to test if a value is an object.

⚠ eslint-plugin-vitest(prefer-to-be-object): Prefer `toBeObject()` for object assertions
╭─[prefer_to_be_object.tsx:1:22]
1 │ expectTypeOf({}).not.toBeInstanceOf(Object);
· ──────────────
╰────
help: Consider using `toBeObject()` to test if a value is an object.

⚠ eslint-plugin-vitest(prefer-to-be-object): Prefer `toBeObject()` for object assertions
╭─[prefer_to_be_object.tsx:1:40]
1 │ expectTypeOf(requestValues()).resolves.toBeInstanceOf(Object);
· ──────────────
╰────
help: Consider using `toBeObject()` to test if a value is an object.

⚠ eslint-plugin-vitest(prefer-to-be-object): Prefer `toBeObject()` for object assertions
╭─[prefer_to_be_object.tsx:1:39]
1 │ expectTypeOf(queryApi()).resolves.not.toBeInstanceOf(Object);
· ──────────────
╰────
help: Consider using `toBeObject()` to test if a value is an object.
5 changes: 4 additions & 1 deletion crates/oxc_linter/src/utils/jest/parse_jest_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,10 @@ fn parse_jest_expect_fn_call<'a>(
}
}

let kind = if is_type_of { JestFnKind::ExpectTypeOf } else { JestFnKind::Expect };

let parsed_expect_fn = ParsedExpectFnCall {
kind: JestFnKind::Expect,
kind,
head,
members,
name: Cow::Borrowed(name),
Expand Down Expand Up @@ -330,6 +332,7 @@ fn resolve_first_ident<'a>(expr: &'a Expression<'a>) -> Option<&'a IdentifierRef
}
}

#[derive(Debug)]
pub enum ParsedJestFnCall<'a> {
GeneralJest(ParsedGeneralJestFnCall<'a>),
Expect(ParsedExpectFnCall<'a>),
Expand Down