From 865205d99295de933a2e5a50b26220bf66cfc57f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 10 May 2023 23:26:29 -0400 Subject: [PATCH] Implement pygrep-hook's Mock-mistake diagnostic (#4366) --- README.md | 2 +- .../test/fixtures/pygrep-hooks/PGH005_0.py | 19 ++++ crates/ruff/src/checkers/ast/mod.rs | 6 ++ crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 7 +- crates/ruff/src/rules/pygrep_hooks/mod.rs | 1 + .../pygrep_hooks/rules/invalid_mock_access.rs | 99 +++++++++++++++++++ .../ruff/src/rules/pygrep_hooks/rules/mod.rs | 8 +- ...grep_hooks__tests__PGH005_PGH005_0.py.snap | 92 +++++++++++++++++ docs/faq.md | 2 +- ruff.schema.json | 1 + 11 files changed, 231 insertions(+), 7 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/pygrep-hooks/PGH005_0.py create mode 100644 crates/ruff/src/rules/pygrep_hooks/rules/invalid_mock_access.rs create mode 100644 crates/ruff/src/rules/pygrep_hooks/snapshots/ruff__rules__pygrep_hooks__tests__PGH005_PGH005_0.py.snap diff --git a/README.md b/README.md index e03235b6a17c9e..e76a31a0ae2bf3 100644 --- a/README.md +++ b/README.md @@ -287,7 +287,7 @@ quality tools, including: - [pandas-vet](https://pypi.org/project/pandas-vet/) - [pep8-naming](https://pypi.org/project/pep8-naming/) - [pydocstyle](https://pypi.org/project/pydocstyle/) -- [pygrep-hooks](https://github.com/pre-commit/pygrep-hooks) ([#980](https://github.com/charliermarsh/ruff/issues/980)) +- [pygrep-hooks](https://github.com/pre-commit/pygrep-hooks) - [pyupgrade](https://pypi.org/project/pyupgrade/) - [tryceratops](https://pypi.org/project/tryceratops/) - [yesqa](https://pypi.org/project/yesqa/) diff --git a/crates/ruff/resources/test/fixtures/pygrep-hooks/PGH005_0.py b/crates/ruff/resources/test/fixtures/pygrep-hooks/PGH005_0.py new file mode 100644 index 00000000000000..cb80cc8c28048d --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pygrep-hooks/PGH005_0.py @@ -0,0 +1,19 @@ +# Errors +assert my_mock.not_called() +assert my_mock.called_once_with() +assert my_mock.not_called +assert my_mock.called_once_with +my_mock.assert_not_called +my_mock.assert_called +my_mock.assert_called_once_with +my_mock.assert_called_once_with +MyMock.assert_called_once_with + +# OK +assert my_mock.call_count == 1 +assert my_mock.called +my_mock.assert_not_called() +my_mock.assert_called() +my_mock.assert_called_once_with() +"""like :meth:`Mock.assert_called_once_with`""" +"""like :meth:`MagicMock.assert_called_once_with`""" diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index f573f1a9445938..2909c19b11e79d 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1657,6 +1657,9 @@ where if self.settings.rules.enabled(Rule::AssertOnStringLiteral) { pylint::rules::assert_on_string_literal(self, test); } + if self.settings.rules.enabled(Rule::InvalidMockAccess) { + pygrep_hooks::rules::non_existent_mock_method(self, test); + } } StmtKind::With { items, body, .. } => { if self.settings.rules.enabled(Rule::AssertRaisesException) { @@ -1948,6 +1951,9 @@ where if self.settings.rules.enabled(Rule::UselessExpression) { flake8_bugbear::rules::useless_expression(self, value); } + if self.settings.rules.enabled(Rule::InvalidMockAccess) { + pygrep_hooks::rules::uncalled_mock_method(self, value); + } if self.settings.rules.enabled(Rule::AsyncioDanglingTask) { if let Some(diagnostic) = ruff::rules::asyncio_dangling_task(value, |expr| { self.ctx.resolve_call_path(expr) diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 54e64c1781746d..9fea0d295c1079 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -563,6 +563,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (PygrepHooks, "002") => Rule::DeprecatedLogWarn, (PygrepHooks, "003") => Rule::BlanketTypeIgnore, (PygrepHooks, "004") => Rule::BlanketNOQA, + (PygrepHooks, "005") => Rule::InvalidMockAccess, // pandas-vet (PandasVet, "002") => Rule::PandasUseOfInplaceArgument, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index db7b827a3904a5..5464f1b210eb3e 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -506,10 +506,11 @@ ruff_macros::register_rules!( rules::flake8_datetimez::rules::CallDateToday, rules::flake8_datetimez::rules::CallDateFromtimestamp, // pygrep-hooks - rules::pygrep_hooks::rules::Eval, - rules::pygrep_hooks::rules::DeprecatedLogWarn, - rules::pygrep_hooks::rules::BlanketTypeIgnore, rules::pygrep_hooks::rules::BlanketNOQA, + rules::pygrep_hooks::rules::BlanketTypeIgnore, + rules::pygrep_hooks::rules::DeprecatedLogWarn, + rules::pygrep_hooks::rules::Eval, + rules::pygrep_hooks::rules::InvalidMockAccess, // pandas-vet rules::pandas_vet::rules::PandasUseOfInplaceArgument, rules::pandas_vet::rules::PandasUseOfDotIsNull, diff --git a/crates/ruff/src/rules/pygrep_hooks/mod.rs b/crates/ruff/src/rules/pygrep_hooks/mod.rs index b31c5028717ca9..a82a3e42857b74 100644 --- a/crates/ruff/src/rules/pygrep_hooks/mod.rs +++ b/crates/ruff/src/rules/pygrep_hooks/mod.rs @@ -20,6 +20,7 @@ mod tests { #[test_case(Rule::BlanketTypeIgnore, Path::new("PGH003_0.py"); "PGH003_0")] #[test_case(Rule::BlanketTypeIgnore, Path::new("PGH003_1.py"); "PGH003_1")] #[test_case(Rule::BlanketNOQA, Path::new("PGH004_0.py"); "PGH004_0")] + #[test_case(Rule::InvalidMockAccess, Path::new("PGH005_0.py"); "PGH005_0")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/pygrep_hooks/rules/invalid_mock_access.rs b/crates/ruff/src/rules/pygrep_hooks/rules/invalid_mock_access.rs new file mode 100644 index 00000000000000..9acdfbe3219d73 --- /dev/null +++ b/crates/ruff/src/rules/pygrep_hooks/rules/invalid_mock_access.rs @@ -0,0 +1,99 @@ +use rustpython_parser::ast::{Expr, ExprKind}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::checkers::ast::Checker; + +#[derive(Debug, PartialEq, Eq)] +enum Reason { + UncalledMethod(String), + NonExistentMethod(String), +} + +/// ## What it does +/// Checks for common mistakes when using mock objects. +/// +/// ## Why is this bad? +/// The `mock` module exposes an assertion API that can be used to verify that +/// mock objects undergo expected interactions. This rule checks for common +/// mistakes when using this API. +/// +/// For example, it checks for mock attribute accesses that should be replaced +/// with mock method calls. +/// +/// ## Example +/// ```python +/// my_mock.assert_called +/// ``` +/// +/// Use instead: +/// ```python +/// my_mock.assert_called() +/// ``` +#[violation] +pub struct InvalidMockAccess { + reason: Reason, +} + +impl Violation for InvalidMockAccess { + #[derive_message_formats] + fn message(&self) -> String { + let InvalidMockAccess { reason } = self; + match reason { + Reason::UncalledMethod(name) => format!("Mock method should be called: `{name}`"), + Reason::NonExistentMethod(name) => format!("Non-existent mock method: `{name}`"), + } + } +} + +/// PGH005 +pub fn uncalled_mock_method(checker: &mut Checker, expr: &Expr) { + if let ExprKind::Attribute { attr, .. } = &expr.node { + if matches!( + attr.as_str(), + "assert_any_call" + | "assert_called" + | "assert_called_once" + | "assert_called_once_with" + | "assert_called_with" + | "assert_has_calls" + | "assert_not_called" + ) { + checker.diagnostics.push(Diagnostic::new( + InvalidMockAccess { + reason: Reason::UncalledMethod(attr.to_string()), + }, + expr.range(), + )); + } + } +} + +/// PGH005 +pub fn non_existent_mock_method(checker: &mut Checker, test: &Expr) { + let attr = match &test.node { + ExprKind::Attribute { attr, .. } => attr, + ExprKind::Call { func, .. } => match &func.node { + ExprKind::Attribute { attr, .. } => attr, + _ => return, + }, + _ => return, + }; + if matches!( + attr.as_str(), + "any_call" + | "called_once" + | "called_once_with" + | "called_with" + | "has_calls" + | "not_called" + ) { + checker.diagnostics.push(Diagnostic::new( + InvalidMockAccess { + reason: Reason::NonExistentMethod(attr.to_string()), + }, + test.range(), + )); + } +} diff --git a/crates/ruff/src/rules/pygrep_hooks/rules/mod.rs b/crates/ruff/src/rules/pygrep_hooks/rules/mod.rs index 3cdb1112e03bb6..b8fbfb773c062f 100644 --- a/crates/ruff/src/rules/pygrep_hooks/rules/mod.rs +++ b/crates/ruff/src/rules/pygrep_hooks/rules/mod.rs @@ -1,9 +1,13 @@ pub(crate) use blanket_noqa::{blanket_noqa, BlanketNOQA}; pub(crate) use blanket_type_ignore::{blanket_type_ignore, BlanketTypeIgnore}; -pub use deprecated_log_warn::{deprecated_log_warn, DeprecatedLogWarn}; -pub use no_eval::{no_eval, Eval}; +pub(crate) use deprecated_log_warn::{deprecated_log_warn, DeprecatedLogWarn}; +pub(crate) use invalid_mock_access::{ + non_existent_mock_method, uncalled_mock_method, InvalidMockAccess, +}; +pub(crate) use no_eval::{no_eval, Eval}; mod blanket_noqa; mod blanket_type_ignore; mod deprecated_log_warn; +mod invalid_mock_access; mod no_eval; diff --git a/crates/ruff/src/rules/pygrep_hooks/snapshots/ruff__rules__pygrep_hooks__tests__PGH005_PGH005_0.py.snap b/crates/ruff/src/rules/pygrep_hooks/snapshots/ruff__rules__pygrep_hooks__tests__PGH005_PGH005_0.py.snap new file mode 100644 index 00000000000000..fbf587a5b827f0 --- /dev/null +++ b/crates/ruff/src/rules/pygrep_hooks/snapshots/ruff__rules__pygrep_hooks__tests__PGH005_PGH005_0.py.snap @@ -0,0 +1,92 @@ +--- +source: crates/ruff/src/rules/pygrep_hooks/mod.rs +--- +PGH005_0.py:2:8: PGH005 Non-existent mock method: `not_called` + | +2 | # Errors +3 | assert my_mock.not_called() + | ^^^^^^^^^^^^^^^^^^^^ PGH005 +4 | assert my_mock.called_once_with() +5 | assert my_mock.not_called + | + +PGH005_0.py:3:8: PGH005 Non-existent mock method: `called_once_with` + | +3 | # Errors +4 | assert my_mock.not_called() +5 | assert my_mock.called_once_with() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005 +6 | assert my_mock.not_called +7 | assert my_mock.called_once_with + | + +PGH005_0.py:4:8: PGH005 Non-existent mock method: `not_called` + | +4 | assert my_mock.not_called() +5 | assert my_mock.called_once_with() +6 | assert my_mock.not_called + | ^^^^^^^^^^^^^^^^^^ PGH005 +7 | assert my_mock.called_once_with +8 | my_mock.assert_not_called + | + +PGH005_0.py:5:8: PGH005 Non-existent mock method: `called_once_with` + | +5 | assert my_mock.called_once_with() +6 | assert my_mock.not_called +7 | assert my_mock.called_once_with + | ^^^^^^^^^^^^^^^^^^^^^^^^ PGH005 +8 | my_mock.assert_not_called +9 | my_mock.assert_called + | + +PGH005_0.py:6:1: PGH005 Mock method should be called: `assert_not_called` + | + 6 | assert my_mock.not_called + 7 | assert my_mock.called_once_with + 8 | my_mock.assert_not_called + | ^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005 + 9 | my_mock.assert_called +10 | my_mock.assert_called_once_with + | + +PGH005_0.py:7:1: PGH005 Mock method should be called: `assert_called` + | + 7 | assert my_mock.called_once_with + 8 | my_mock.assert_not_called + 9 | my_mock.assert_called + | ^^^^^^^^^^^^^^^^^^^^^ PGH005 +10 | my_mock.assert_called_once_with +11 | my_mock.assert_called_once_with + | + +PGH005_0.py:8:1: PGH005 Mock method should be called: `assert_called_once_with` + | + 8 | my_mock.assert_not_called + 9 | my_mock.assert_called +10 | my_mock.assert_called_once_with + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005 +11 | my_mock.assert_called_once_with +12 | MyMock.assert_called_once_with + | + +PGH005_0.py:9:1: PGH005 Mock method should be called: `assert_called_once_with` + | + 9 | my_mock.assert_called +10 | my_mock.assert_called_once_with +11 | my_mock.assert_called_once_with + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005 +12 | MyMock.assert_called_once_with + | + +PGH005_0.py:10:1: PGH005 Mock method should be called: `assert_called_once_with` + | +10 | my_mock.assert_called_once_with +11 | my_mock.assert_called_once_with +12 | MyMock.assert_called_once_with + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005 +13 | +14 | # OK + | + + diff --git a/docs/faq.md b/docs/faq.md index 4cfcd313eaf777..475cffc3c5fcb7 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -71,7 +71,7 @@ natively, including: - [pandas-vet](https://pypi.org/project/pandas-vet/) - [pep8-naming](https://pypi.org/project/pep8-naming/) - [pydocstyle](https://pypi.org/project/pydocstyle/) -- [pygrep-hooks](https://github.com/pre-commit/pygrep-hooks) ([#980](https://github.com/charliermarsh/ruff/issues/980)) +- [pygrep-hooks](https://github.com/pre-commit/pygrep-hooks) - [pyupgrade](https://pypi.org/project/pyupgrade/) - [tryceratops](https://pypi.org/project/tryceratops/) - [yesqa](https://github.com/asottile/yesqa) diff --git a/ruff.schema.json b/ruff.schema.json index e47ab3d06dee3a..fa891b1c6ae57c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1921,6 +1921,7 @@ "PGH002", "PGH003", "PGH004", + "PGH005", "PIE", "PIE7", "PIE79",