Include BaseException in B017 rule (#5466)

Closes #5462.
This commit is contained in:
Charlie Marsh 2023-07-02 20:18:33 -04:00 committed by GitHub
parent f0ec9ecd67
commit af7051b976
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 111 additions and 67 deletions

View File

@ -23,6 +23,10 @@ class Foobar(unittest.TestCase):
with self.assertRaises(Exception):
raise Exception("Evil I say!")
def also_evil_raises(self) -> None:
with self.assertRaises(BaseException):
raise Exception("Evil I say!")
def context_manager_raises(self) -> None:
with self.assertRaises(Exception) as ex:
raise Exception("Context manager is good")
@ -41,6 +45,9 @@ def test_pytest_raises():
with pytest.raises(Exception):
raise ValueError("Hello")
with pytest.raises(Exception), pytest.raises(ValueError):
raise ValueError("Hello")
with pytest.raises(Exception, "hello"):
raise ValueError("This is fine")

View File

@ -1409,7 +1409,7 @@ where
Stmt::With(ast::StmtWith { items, body, .. })
| Stmt::AsyncWith(ast::StmtAsyncWith { items, body, .. }) => {
if self.enabled(Rule::AssertRaisesException) {
flake8_bugbear::rules::assert_raises_exception(self, stmt, items);
flake8_bugbear::rules::assert_raises_exception(self, items);
}
if self.enabled(Rule::PytestRaisesWithMultipleStatements) {
flake8_pytest_style::rules::complex_raises(self, stmt, items, body);

View File

@ -1,22 +1,20 @@
use rustpython_parser::ast::{self, Expr, Ranged, Stmt, WithItem};
use std::fmt;
use rustpython_parser::ast::{self, Expr, Ranged, WithItem};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use crate::checkers::ast::Checker;
#[derive(Debug, PartialEq, Eq)]
pub(crate) enum AssertionKind {
AssertRaises,
PytestRaises,
}
/// ## What it does
/// Checks for `self.assertRaises(Exception)` or `pytest.raises(Exception)`.
/// Checks for `assertRaises` and `pytest.raises` context managers that catch
/// `Exception` or `BaseException`.
///
/// ## Why is this bad?
/// These forms catch every `Exception`, which can lead to tests passing even
/// if, e.g., the code being tested is never executed due to a typo.
/// if, e.g., the code under consideration raises a `SyntaxError` or
/// `IndentationError`.
///
/// Either assert for a more specific exception (builtin or custom), or use
/// `assertRaisesRegex` or `pytest.raises(..., match=<REGEX>)` respectively.
@ -32,30 +30,55 @@ pub(crate) enum AssertionKind {
/// ```
#[violation]
pub struct AssertRaisesException {
kind: AssertionKind,
assertion: AssertionKind,
exception: ExceptionKind,
}
impl Violation for AssertRaisesException {
#[derive_message_formats]
fn message(&self) -> String {
match self.kind {
AssertionKind::AssertRaises => {
format!("`assertRaises(Exception)` should be considered evil")
let AssertRaisesException {
assertion,
exception,
} = self;
format!("`{assertion}({exception})` should be considered evil")
}
AssertionKind::PytestRaises => {
format!("`pytest.raises(Exception)` should be considered evil")
}
#[derive(Debug, PartialEq, Eq)]
enum AssertionKind {
AssertRaises,
PytestRaises,
}
impl fmt::Display for AssertionKind {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
match self {
AssertionKind::AssertRaises => fmt.write_str("assertRaises"),
AssertionKind::PytestRaises => fmt.write_str("pytest.raises"),
}
}
}
#[derive(Debug, PartialEq, Eq)]
enum ExceptionKind {
BaseException,
Exception,
}
impl fmt::Display for ExceptionKind {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
match self {
ExceptionKind::BaseException => fmt.write_str("BaseException"),
ExceptionKind::Exception => fmt.write_str("Exception"),
}
}
}
/// B017
pub(crate) fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[WithItem]) {
let Some(item) = items.first() else {
return;
};
let item_context = &item.context_expr;
let Expr::Call(ast::ExprCall { func, args, keywords, range: _ }) = &item_context else {
pub(crate) fn assert_raises_exception(checker: &mut Checker, items: &[WithItem]) {
for item in items {
let Expr::Call(ast::ExprCall { func, args, keywords, range: _ }) = &item.context_expr else {
return;
};
if args.len() != 1 {
@ -65,18 +88,18 @@ pub(crate) fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items:
return;
}
if !checker
let Some(exception) = checker
.semantic()
.resolve_call_path(args.first().unwrap())
.map_or(false, |call_path| {
matches!(call_path.as_slice(), ["", "Exception"])
})
{
return;
.and_then(|call_path| {
match call_path.as_slice() {
["", "Exception"] => Some(ExceptionKind::Exception),
["", "BaseException"] => Some(ExceptionKind::BaseException),
_ => None,
}
}) else { return; };
let kind = {
if matches!(func.as_ref(), Expr::Attribute(ast::ExprAttribute { attr, .. }) if attr == "assertRaises")
let assertion = if matches!(func.as_ref(), Expr::Attribute(ast::ExprAttribute { attr, .. }) if attr == "assertRaises")
{
AssertionKind::AssertRaises
} else if checker
@ -92,11 +115,14 @@ pub(crate) fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items:
AssertionKind::PytestRaises
} else {
return;
}
};
checker.diagnostics.push(Diagnostic::new(
AssertRaisesException { kind },
stmt.range(),
AssertRaisesException {
assertion,
exception,
},
item.range(),
));
}
}

View File

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

View File

@ -9,7 +9,7 @@ use ruff_python_semantic::ScopeKind;
use crate::checkers::ast::Checker;
#[derive(Debug, PartialEq, Eq)]
pub(crate) enum DeferralKeyword {
enum DeferralKeyword {
Yield,
YieldFrom,
Await,