From 70696061cd0822e9212725415aabc77905f85067 Mon Sep 17 00:00:00 2001 From: Harutaka Kawamura Date: Tue, 15 Aug 2023 05:25:23 +0900 Subject: [PATCH] [`flake8-pytest-style`] Implement `pytest-unittest-raises-assertion` (`PT027`) (#6554) --- .../fixtures/flake8_pytest_style/PT027_0.py | 48 ++++ .../fixtures/flake8_pytest_style/PT027_1.py | 12 + .../src/checkers/ast/analyze/expression.rs | 7 + crates/ruff/src/codes.rs | 1 + .../ruff/src/rules/flake8_pytest_style/mod.rs | 12 + .../flake8_pytest_style/rules/assertion.rs | 185 ++++++++++++- ...__flake8_pytest_style__tests__PT027_0.snap | 248 ++++++++++++++++++ ...__flake8_pytest_style__tests__PT027_1.snap | 21 ++ .../tryceratops/rules/raise_vanilla_args.rs | 3 +- ruff.schema.json | 1 + 10 files changed, 536 insertions(+), 2 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_0.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_1.py create mode 100644 crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT027_0.snap create mode 100644 crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT027_1.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_0.py b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_0.py new file mode 100644 index 0000000000..b9614f0647 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_0.py @@ -0,0 +1,48 @@ +import unittest + + +class Test(unittest.TestCase): + def test_errors(self): + with self.assertRaises(ValueError): + raise ValueError + with self.assertRaises(expected_exception=ValueError): + raise ValueError + + with self.failUnlessRaises(ValueError): + raise ValueError + + with self.assertRaisesRegex(ValueError, "test"): + raise ValueError("test") + + with self.assertRaisesRegex(ValueError, expected_regex="test"): + raise ValueError("test") + + with self.assertRaisesRegex( + expected_exception=ValueError, expected_regex="test" + ): + raise ValueError("test") + + with self.assertRaisesRegex( + expected_regex="test", expected_exception=ValueError + ): + raise ValueError("test") + + with self.assertRaisesRegexp(ValueError, "test"): + raise ValueError("test") + + def test_unfixable_errors(self): + with self.assertRaises(ValueError, msg="msg"): + raise ValueError + + with self.assertRaises( + # comment + ValueError + ): + raise ValueError + + with ( + self + # comment + .assertRaises(ValueError) + ): + raise ValueError diff --git a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_1.py b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_1.py new file mode 100644 index 0000000000..708a582ad3 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_1.py @@ -0,0 +1,12 @@ +import unittest +import pytest + + +class Test(unittest.TestCase): + def test_pytest_raises(self): + with pytest.raises(ValueError): + raise ValueError + + def test_errors(self): + with self.assertRaises(ValueError): + raise ValueError diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index 2ea6293cdc..2951899f4c 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -756,6 +756,13 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { checker.diagnostics.push(diagnostic); } } + if checker.enabled(Rule::PytestUnittestRaisesAssertion) { + if let Some(diagnostic) = + flake8_pytest_style::rules::unittest_raises_assertion(checker, call) + { + checker.diagnostics.push(diagnostic); + } + } if checker.enabled(Rule::SubprocessPopenPreexecFn) { pylint::rules::subprocess_popen_preexec_fn(checker, call); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index b8ac97ab17..79add843bc 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -697,6 +697,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8PytestStyle, "024") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestUnnecessaryAsyncioMarkOnFixture), (Flake8PytestStyle, "025") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestErroneousUseFixturesOnFixture), (Flake8PytestStyle, "026") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestUseFixturesWithoutParameters), + (Flake8PytestStyle, "027") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestUnittestRaisesAssertion), // flake8-pie (Flake8Pie, "790") => (RuleGroup::Unspecified, rules::flake8_pie::rules::UnnecessaryPass), diff --git a/crates/ruff/src/rules/flake8_pytest_style/mod.rs b/crates/ruff/src/rules/flake8_pytest_style/mod.rs index c36222950e..023ba3d23e 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/mod.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/mod.rs @@ -250,6 +250,18 @@ mod tests { Settings::default(), "PT026" )] + #[test_case( + Rule::PytestUnittestRaisesAssertion, + Path::new("PT027_0.py"), + Settings::default(), + "PT027_0" + )] + #[test_case( + Rule::PytestUnittestRaisesAssertion, + Path::new("PT027_1.py"), + Settings::default(), + "PT027_1" + )] fn test_pytest_style( rule_code: Rule, path: &Path, diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs index dbb571c527..5514cd8744 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs @@ -7,12 +7,14 @@ use libcst_native::{ ParenthesizedNode, SimpleStatementLine, SimpleWhitespace, SmallStatement, Statement, TrailingWhitespace, UnaryOperation, }; -use ruff_python_ast::{self as ast, BoolOp, ExceptHandler, Expr, Keyword, Ranged, Stmt, UnaryOp}; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::Truthiness; use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::{ + self as ast, Arguments, BoolOp, ExceptHandler, Expr, Keyword, Ranged, Stmt, UnaryOp, +}; use ruff_python_ast::{visitor, whitespace}; use ruff_python_codegen::Stylist; use ruff_source_file::Locator; @@ -21,6 +23,7 @@ use crate::autofix::codemods::CodegenStylist; use crate::checkers::ast::Checker; use crate::cst::matchers::match_indented_block; use crate::cst::matchers::match_module; +use crate::importer::ImportRequest; use crate::registry::AsRule; use super::unittest_assert::UnittestAssert; @@ -291,6 +294,186 @@ pub(crate) fn unittest_assertion( } } +/// ## What it does +/// Checks for uses of exception-related assertion methods from the `unittest` +/// module. +/// +/// ## Why is this bad? +/// To enforce the assertion style recommended by `pytest`, `pytest.raises` is +/// preferred over the exception-related assertion methods in `unittest`, like +/// `assertRaises`. +/// +/// ## Example +/// ```python +/// import unittest +/// +/// +/// class TestFoo(unittest.TestCase): +/// def test_foo(self): +/// with self.assertRaises(ValueError): +/// raise ValueError("foo") +/// ``` +/// +/// Use instead: +/// ```python +/// import unittest +/// import pytest +/// +/// +/// class TestFoo(unittest.TestCase): +/// def test_foo(self): +/// with pytest.raises(ValueError): +/// raise ValueError("foo") +/// ``` +/// +/// ## References +/// - [`pytest` documentation: Assertions about expected exceptions](https://docs.pytest.org/en/latest/how-to/assert.html#assertions-about-expected-exceptions) +#[violation] +pub struct PytestUnittestRaisesAssertion { + assertion: String, +} + +impl Violation for PytestUnittestRaisesAssertion { + const AUTOFIX: AutofixKind = AutofixKind::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + let PytestUnittestRaisesAssertion { assertion } = self; + format!("Use `pytest.raises` instead of unittest-style `{assertion}`") + } + + fn autofix_title(&self) -> Option { + let PytestUnittestRaisesAssertion { assertion } = self; + Some(format!("Replace `{assertion}` with `pytest.raises`")) + } +} + +/// PT027 +pub(crate) fn unittest_raises_assertion( + checker: &Checker, + call: &ast::ExprCall, +) -> Option { + let Expr::Attribute(ast::ExprAttribute { attr, .. }) = call.func.as_ref() else { + return None; + }; + + if !matches!( + attr.as_str(), + "assertRaises" | "failUnlessRaises" | "assertRaisesRegex" | "assertRaisesRegexp" + ) { + return None; + } + + let mut diagnostic = Diagnostic::new( + PytestUnittestRaisesAssertion { + assertion: attr.to_string(), + }, + call.func.range(), + ); + if checker.patch(diagnostic.kind.rule()) + && !checker.indexer().has_comments(call, checker.locator()) + { + if let Some(args) = to_pytest_raises_args(checker, attr.as_str(), &call.arguments) { + diagnostic.try_set_fix(|| { + let (import_edit, binding) = checker.importer().get_or_import_symbol( + &ImportRequest::import("pytest", "raises"), + call.func.start(), + checker.semantic(), + )?; + let edit = Edit::range_replacement(format!("{binding}({args})"), call.range()); + Ok(Fix::suggested_edits(import_edit, [edit])) + }); + } + } + + Some(diagnostic) +} + +fn to_pytest_raises_args<'a>( + checker: &Checker<'a>, + attr: &str, + arguments: &Arguments, +) -> Option> { + let args = match attr { + "assertRaises" | "failUnlessRaises" => { + match (arguments.args.as_slice(), arguments.keywords.as_slice()) { + // Ex) `assertRaises(Exception)` + ([arg], []) => Cow::Borrowed(checker.locator().slice(arg.range())), + // Ex) `assertRaises(expected_exception=Exception)` + ([], [kwarg]) + if kwarg + .arg + .as_ref() + .is_some_and(|id| id.as_str() == "expected_exception") => + { + Cow::Borrowed(checker.locator().slice(kwarg.value.range())) + } + _ => return None, + } + } + "assertRaisesRegex" | "assertRaisesRegexp" => { + match (arguments.args.as_slice(), arguments.keywords.as_slice()) { + // Ex) `assertRaisesRegex(Exception, regex)` + ([arg1, arg2], []) => Cow::Owned(format!( + "{}, match={}", + checker.locator().slice(arg1.range()), + checker.locator().slice(arg2.range()) + )), + // Ex) `assertRaisesRegex(Exception, expected_regex=regex)` + ([arg], [kwarg]) + if kwarg + .arg + .as_ref() + .is_some_and(|arg| arg.as_str() == "expected_regex") => + { + Cow::Owned(format!( + "{}, match={}", + checker.locator().slice(arg.range()), + checker.locator().slice(kwarg.value.range()) + )) + } + // Ex) `assertRaisesRegex(expected_exception=Exception, expected_regex=regex)` + ([], [kwarg1, kwarg2]) + if kwarg1 + .arg + .as_ref() + .is_some_and(|id| id.as_str() == "expected_exception") + && kwarg2 + .arg + .as_ref() + .is_some_and(|id| id.as_str() == "expected_regex") => + { + Cow::Owned(format!( + "{}, match={}", + checker.locator().slice(kwarg1.value.range()), + checker.locator().slice(kwarg2.value.range()) + )) + } + // Ex) `assertRaisesRegex(expected_regex=regex, expected_exception=Exception)` + ([], [kwarg1, kwarg2]) + if kwarg1 + .arg + .as_ref() + .is_some_and(|id| id.as_str() == "expected_regex") + && kwarg2 + .arg + .as_ref() + .is_some_and(|id| id.as_str() == "expected_exception") => + { + Cow::Owned(format!( + "{}, match={}", + checker.locator().slice(kwarg2.value.range()), + checker.locator().slice(kwarg1.value.range()) + )) + } + _ => return None, + } + } + _ => return None, + }; + Some(args) +} + /// PT015 pub(crate) fn assert_falsy(checker: &mut Checker, stmt: &Stmt, test: &Expr) { if Truthiness::from_expr(test, |id| checker.semantic().is_builtin(id)).is_falsey() { diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT027_0.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT027_0.snap new file mode 100644 index 0000000000..8ac1da881c --- /dev/null +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT027_0.snap @@ -0,0 +1,248 @@ +--- +source: crates/ruff/src/rules/flake8_pytest_style/mod.rs +--- +PT027_0.py:6:14: PT027 [*] Use `pytest.raises` instead of unittest-style `assertRaises` + | +4 | class Test(unittest.TestCase): +5 | def test_errors(self): +6 | with self.assertRaises(ValueError): + | ^^^^^^^^^^^^^^^^^ PT027 +7 | raise ValueError +8 | with self.assertRaises(expected_exception=ValueError): + | + = help: Replace `assertRaises` with `pytest.raises` + +ℹ Suggested fix +1 1 | import unittest + 2 |+import pytest +2 3 | +3 4 | +4 5 | class Test(unittest.TestCase): +5 6 | def test_errors(self): +6 |- with self.assertRaises(ValueError): + 7 |+ with pytest.raises(ValueError): +7 8 | raise ValueError +8 9 | with self.assertRaises(expected_exception=ValueError): +9 10 | raise ValueError + +PT027_0.py:8:14: PT027 [*] Use `pytest.raises` instead of unittest-style `assertRaises` + | +6 | with self.assertRaises(ValueError): +7 | raise ValueError +8 | with self.assertRaises(expected_exception=ValueError): + | ^^^^^^^^^^^^^^^^^ PT027 +9 | raise ValueError + | + = help: Replace `assertRaises` with `pytest.raises` + +ℹ Suggested fix +1 1 | import unittest + 2 |+import pytest +2 3 | +3 4 | +4 5 | class Test(unittest.TestCase): +5 6 | def test_errors(self): +6 7 | with self.assertRaises(ValueError): +7 8 | raise ValueError +8 |- with self.assertRaises(expected_exception=ValueError): + 9 |+ with pytest.raises(ValueError): +9 10 | raise ValueError +10 11 | +11 12 | with self.failUnlessRaises(ValueError): + +PT027_0.py:11:14: PT027 [*] Use `pytest.raises` instead of unittest-style `failUnlessRaises` + | + 9 | raise ValueError +10 | +11 | with self.failUnlessRaises(ValueError): + | ^^^^^^^^^^^^^^^^^^^^^ PT027 +12 | raise ValueError + | + = help: Replace `failUnlessRaises` with `pytest.raises` + +ℹ Suggested fix +1 1 | import unittest + 2 |+import pytest +2 3 | +3 4 | +4 5 | class Test(unittest.TestCase): +-------------------------------------------------------------------------------- +8 9 | with self.assertRaises(expected_exception=ValueError): +9 10 | raise ValueError +10 11 | +11 |- with self.failUnlessRaises(ValueError): + 12 |+ with pytest.raises(ValueError): +12 13 | raise ValueError +13 14 | +14 15 | with self.assertRaisesRegex(ValueError, "test"): + +PT027_0.py:14:14: PT027 [*] Use `pytest.raises` instead of unittest-style `assertRaisesRegex` + | +12 | raise ValueError +13 | +14 | with self.assertRaisesRegex(ValueError, "test"): + | ^^^^^^^^^^^^^^^^^^^^^^ PT027 +15 | raise ValueError("test") + | + = help: Replace `assertRaisesRegex` with `pytest.raises` + +ℹ Suggested fix +1 1 | import unittest + 2 |+import pytest +2 3 | +3 4 | +4 5 | class Test(unittest.TestCase): +-------------------------------------------------------------------------------- +11 12 | with self.failUnlessRaises(ValueError): +12 13 | raise ValueError +13 14 | +14 |- with self.assertRaisesRegex(ValueError, "test"): + 15 |+ with pytest.raises(ValueError, match="test"): +15 16 | raise ValueError("test") +16 17 | +17 18 | with self.assertRaisesRegex(ValueError, expected_regex="test"): + +PT027_0.py:17:14: PT027 [*] Use `pytest.raises` instead of unittest-style `assertRaisesRegex` + | +15 | raise ValueError("test") +16 | +17 | with self.assertRaisesRegex(ValueError, expected_regex="test"): + | ^^^^^^^^^^^^^^^^^^^^^^ PT027 +18 | raise ValueError("test") + | + = help: Replace `assertRaisesRegex` with `pytest.raises` + +ℹ Suggested fix +1 1 | import unittest + 2 |+import pytest +2 3 | +3 4 | +4 5 | class Test(unittest.TestCase): +-------------------------------------------------------------------------------- +14 15 | with self.assertRaisesRegex(ValueError, "test"): +15 16 | raise ValueError("test") +16 17 | +17 |- with self.assertRaisesRegex(ValueError, expected_regex="test"): + 18 |+ with pytest.raises(ValueError, match="test"): +18 19 | raise ValueError("test") +19 20 | +20 21 | with self.assertRaisesRegex( + +PT027_0.py:20:14: PT027 [*] Use `pytest.raises` instead of unittest-style `assertRaisesRegex` + | +18 | raise ValueError("test") +19 | +20 | with self.assertRaisesRegex( + | ^^^^^^^^^^^^^^^^^^^^^^ PT027 +21 | expected_exception=ValueError, expected_regex="test" +22 | ): + | + = help: Replace `assertRaisesRegex` with `pytest.raises` + +ℹ Suggested fix +1 1 | import unittest + 2 |+import pytest +2 3 | +3 4 | +4 5 | class Test(unittest.TestCase): +-------------------------------------------------------------------------------- +17 18 | with self.assertRaisesRegex(ValueError, expected_regex="test"): +18 19 | raise ValueError("test") +19 20 | +20 |- with self.assertRaisesRegex( +21 |- expected_exception=ValueError, expected_regex="test" +22 |- ): + 21 |+ with pytest.raises(ValueError, match="test"): +23 22 | raise ValueError("test") +24 23 | +25 24 | with self.assertRaisesRegex( + +PT027_0.py:25:14: PT027 [*] Use `pytest.raises` instead of unittest-style `assertRaisesRegex` + | +23 | raise ValueError("test") +24 | +25 | with self.assertRaisesRegex( + | ^^^^^^^^^^^^^^^^^^^^^^ PT027 +26 | expected_regex="test", expected_exception=ValueError +27 | ): + | + = help: Replace `assertRaisesRegex` with `pytest.raises` + +ℹ Suggested fix +1 1 | import unittest + 2 |+import pytest +2 3 | +3 4 | +4 5 | class Test(unittest.TestCase): +-------------------------------------------------------------------------------- +22 23 | ): +23 24 | raise ValueError("test") +24 25 | +25 |- with self.assertRaisesRegex( +26 |- expected_regex="test", expected_exception=ValueError +27 |- ): + 26 |+ with pytest.raises(ValueError, match="test"): +28 27 | raise ValueError("test") +29 28 | +30 29 | with self.assertRaisesRegexp(ValueError, "test"): + +PT027_0.py:30:14: PT027 [*] Use `pytest.raises` instead of unittest-style `assertRaisesRegexp` + | +28 | raise ValueError("test") +29 | +30 | with self.assertRaisesRegexp(ValueError, "test"): + | ^^^^^^^^^^^^^^^^^^^^^^^ PT027 +31 | raise ValueError("test") + | + = help: Replace `assertRaisesRegexp` with `pytest.raises` + +ℹ Suggested fix +1 1 | import unittest + 2 |+import pytest +2 3 | +3 4 | +4 5 | class Test(unittest.TestCase): +-------------------------------------------------------------------------------- +27 28 | ): +28 29 | raise ValueError("test") +29 30 | +30 |- with self.assertRaisesRegexp(ValueError, "test"): + 31 |+ with pytest.raises(ValueError, match="test"): +31 32 | raise ValueError("test") +32 33 | +33 34 | def test_unfixable_errors(self): + +PT027_0.py:34:14: PT027 Use `pytest.raises` instead of unittest-style `assertRaises` + | +33 | def test_unfixable_errors(self): +34 | with self.assertRaises(ValueError, msg="msg"): + | ^^^^^^^^^^^^^^^^^ PT027 +35 | raise ValueError + | + = help: Replace `assertRaises` with `pytest.raises` + +PT027_0.py:37:14: PT027 Use `pytest.raises` instead of unittest-style `assertRaises` + | +35 | raise ValueError +36 | +37 | with self.assertRaises( + | ^^^^^^^^^^^^^^^^^ PT027 +38 | # comment +39 | ValueError + | + = help: Replace `assertRaises` with `pytest.raises` + +PT027_0.py:44:13: PT027 Use `pytest.raises` instead of unittest-style `assertRaises` + | +43 | with ( +44 | self + | _____________^ +45 | | # comment +46 | | .assertRaises(ValueError) + | |_________________________^ PT027 +47 | ): +48 | raise ValueError + | + = help: Replace `assertRaises` with `pytest.raises` + + diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT027_1.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT027_1.snap new file mode 100644 index 0000000000..7656398481 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT027_1.snap @@ -0,0 +1,21 @@ +--- +source: crates/ruff/src/rules/flake8_pytest_style/mod.rs +--- +PT027_1.py:11:14: PT027 [*] Use `pytest.raises` instead of unittest-style `assertRaises` + | +10 | def test_errors(self): +11 | with self.assertRaises(ValueError): + | ^^^^^^^^^^^^^^^^^ PT027 +12 | raise ValueError + | + = help: Replace `assertRaises` with `pytest.raises` + +ℹ Suggested fix +8 8 | raise ValueError +9 9 | +10 10 | def test_errors(self): +11 |- with self.assertRaises(ValueError): + 11 |+ with pytest.raises(ValueError): +12 12 | raise ValueError + + diff --git a/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_args.rs b/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_args.rs index d9cc91351b..aadfa930c4 100644 --- a/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_args.rs +++ b/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_args.rs @@ -58,7 +58,8 @@ pub(crate) fn raise_vanilla_args(checker: &mut Checker, expr: &Expr) { func, arguments: Arguments { args, .. }, .. - }) = expr else { + }) = expr + else { return; }; diff --git a/ruff.schema.json b/ruff.schema.json index c60a447c5a..ed2098be3e 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2335,6 +2335,7 @@ "PT024", "PT025", "PT026", + "PT027", "PTH", "PTH1", "PTH10",