[flake8-bugbear] Add pytest.raises(Exception) support to B017 (#4052)

This commit is contained in:
Alan Du 2023-04-20 23:43:01 -04:00 committed by GitHub
parent ba98149022
commit 82abbc7234
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 82 additions and 22 deletions

View file

@ -1,9 +1,10 @@
""" """
Should emit: Should emit:
B017 - on lines 20 B017 - on lines 23 and 41
""" """
import asyncio import asyncio
import unittest import unittest
import pytest
CONSTANT = True CONSTANT = True
@ -34,3 +35,14 @@ class Foobar(unittest.TestCase):
def raises_with_absolute_reference(self): def raises_with_absolute_reference(self):
with self.assertRaises(asyncio.CancelledError): with self.assertRaises(asyncio.CancelledError):
Foo() Foo()
def test_pytest_raises():
with pytest.raises(Exception):
raise ValueError("Hello")
with pytest.raises(Exception, "hello"):
raise ValueError("This is fine")
with pytest.raises(Exception, match="hello"):
raise ValueError("This is also fine")

View file

@ -6,15 +6,21 @@ use ruff_python_ast::types::Range;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
#[derive(Debug, PartialEq, Eq)]
pub enum AssertionKind {
AssertRaises,
PytestRaises,
}
/// ## What it does /// ## What it does
/// Checks for `self.assertRaises(Exception)`. /// Checks for `self.assertRaises(Exception)` or `pytest.raises(Exception)`.
/// ///
/// ## Why is this bad? /// ## Why is this bad?
/// `assertRaises(Exception)` can lead to your test passing even if the /// These forms catch every `Exception`, which can lead to tests passing even
/// code being tested is never executed due to a typo. /// if, e.g., the code being tested is never executed due to a typo.
/// ///
/// Either assert for a more specific exception (builtin or custom), use /// Either assert for a more specific exception (builtin or custom), or use
/// `assertRaisesRegex` or the context manager form of `assertRaises`. /// `assertRaisesRegex` or `pytest.raises(..., match=<REGEX>)` respectively.
/// ///
/// ## Example /// ## Example
/// ```python /// ```python
@ -26,13 +32,22 @@ use crate::checkers::ast::Checker;
/// self.assertRaises(SomeSpecificException, foo) /// self.assertRaises(SomeSpecificException, foo)
/// ``` /// ```
#[violation] #[violation]
pub struct AssertRaisesException; pub struct AssertRaisesException {
kind: AssertionKind,
}
impl Violation for AssertRaisesException { impl Violation for AssertRaisesException {
#[derive_message_formats] #[derive_message_formats]
fn message(&self) -> String { fn message(&self) -> String {
match self.kind {
AssertionKind::AssertRaises => {
format!("`assertRaises(Exception)` should be considered evil") format!("`assertRaises(Exception)` should be considered evil")
} }
AssertionKind::PytestRaises => {
format!("`pytest.raises(Exception)` should be considered evil")
}
}
}
} }
/// B017 /// B017
@ -41,7 +56,7 @@ pub fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[With
return; return;
}; };
let item_context = &item.context_expr; let item_context = &item.context_expr;
let ExprKind::Call { func, args, .. } = &item_context.node else { let ExprKind::Call { func, args, keywords } = &item_context.node else {
return; return;
}; };
if args.len() != 1 { if args.len() != 1 {
@ -50,9 +65,7 @@ pub fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[With
if item.optional_vars.is_some() { if item.optional_vars.is_some() {
return; return;
} }
if !matches!(&func.node, ExprKind::Attribute { attr, .. } if attr == "assertRaises") {
return;
}
if !checker if !checker
.ctx .ctx
.resolve_call_path(args.first().unwrap()) .resolve_call_path(args.first().unwrap())
@ -61,7 +74,31 @@ pub fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[With
return; return;
} }
checker let kind = {
.diagnostics if matches!(&func.node, ExprKind::Attribute { attr, .. } if attr == "assertRaises") {
.push(Diagnostic::new(AssertRaisesException, Range::from(stmt))); AssertionKind::AssertRaises
} else if checker
.ctx
.resolve_call_path(func)
.map_or(false, |call_path| {
call_path.as_slice() == ["pytest", "raises"]
})
&& !keywords.iter().any(|keyword| {
keyword
.node
.arg
.as_ref()
.map_or(false, |arg| arg == "match")
})
{
AssertionKind::PytestRaises
} else {
return;
}
};
checker.diagnostics.push(Diagnostic::new(
AssertRaisesException { kind },
Range::from(stmt),
));
} }

View file

@ -1,16 +1,27 @@
--- ---
source: crates/ruff/src/rules/flake8_bugbear/mod.rs source: crates/ruff/src/rules/flake8_bugbear/mod.rs
--- ---
B017.py:22:9: B017 `assertRaises(Exception)` should be considered evil B017.py:23:9: B017 `assertRaises(Exception)` should be considered evil
| |
22 | class Foobar(unittest.TestCase): 23 | class Foobar(unittest.TestCase):
23 | def evil_raises(self) -> None: 24 | def evil_raises(self) -> None:
24 | with self.assertRaises(Exception): 25 | with self.assertRaises(Exception):
| _________^ | _________^
25 | | raise Exception("Evil I say!") 26 | | raise Exception("Evil I say!")
| |__________________________________________^ B017 | |__________________________________________^ B017
26 | 27 |
27 | def context_manager_raises(self) -> None: 28 | def context_manager_raises(self) -> None:
|
B017.py:41:5: B017 `pytest.raises(Exception)` should be considered evil
|
41 | def test_pytest_raises():
42 | with pytest.raises(Exception):
| _____^
43 | | raise ValueError("Hello")
| |_________________________________^ B017
44 |
45 | with pytest.raises(Exception, "hello"):
| |