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

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

Merged
merged 5 commits into from
Apr 21, 2023
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
14 changes: 13 additions & 1 deletion crates/ruff/resources/test/fixtures/flake8_bugbear/B017.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
"""
Should emit:
B017 - on lines 20
B017 - on lines 23 and 41
"""
import asyncio
import unittest
import pytest

CONSTANT = True

Expand Down Expand Up @@ -34,3 +35,14 @@ def regex_raises(self) -> None:
def raises_with_absolute_reference(self):
with self.assertRaises(asyncio.CancelledError):
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")
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,21 @@ use ruff_python_ast::types::Range;

use crate::checkers::ast::Checker;

#[derive(Debug, PartialEq, Eq)]
pub enum AssertionKind {
AssertRaises,
PytestRaises,
}

/// ## What it does
/// Checks for `self.assertRaises(Exception)`.
/// Checks for `self.assertRaises(Exception)` or `pytest.raises(Exception)`.
///
/// ## Why is this bad?
/// `assertRaises(Exception)` can lead to your test passing even if the
/// code being tested is never executed due to a typo.
/// 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.
///
/// Either assert for a more specific exception (builtin or custom), use
/// `assertRaisesRegex` or the context manager form of `assertRaises`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This felt misleading, since the context manager form of assertRaises actually gives a lint error!

/// Either assert for a more specific exception (builtin or custom), or use
/// `assertRaisesRegex` or `pytest.raises(..., match=<REGEX>)` respectively.
///
/// ## Example
/// ```python
Expand All @@ -26,12 +32,21 @@ use crate::checkers::ast::Checker;
/// self.assertRaises(SomeSpecificException, foo)
/// ```
#[violation]
pub struct AssertRaisesException;
pub struct AssertRaisesException {
kind: AssertionKind,
}

impl Violation for AssertRaisesException {
#[derive_message_formats]
fn message(&self) -> String {
format!("`assertRaises(Exception)` should be considered evil")
match self.kind {
AssertionKind::AssertRaises => {
format!("`assertRaises(Exception)` should be considered evil")
}
AssertionKind::PytestRaises => {
format!("`pytest.raises(Exception)` should be considered evil")
}
}
}
}

Expand All @@ -41,7 +56,7 @@ pub fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[With
return;
};
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;
};
if args.len() != 1 {
Expand All @@ -50,9 +65,7 @@ pub fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[With
if item.optional_vars.is_some() {
return;
}
if !matches!(&func.node, ExprKind::Attribute { attr, .. } if attr == "assertRaises") {
return;
}

if !checker
.ctx
.resolve_call_path(args.first().unwrap())
Expand All @@ -61,7 +74,31 @@ pub fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[With
return;
}

checker
.diagnostics
.push(Diagnostic::new(AssertRaisesException, Range::from(stmt)));
let kind = {
if matches!(&func.node, ExprKind::Attribute { attr, .. } if attr == "assertRaises") {
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),
));
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,27 @@
---
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 | def evil_raises(self) -> None:
24 | with self.assertRaises(Exception):
23 | class Foobar(unittest.TestCase):
24 | def evil_raises(self) -> None:
25 | with self.assertRaises(Exception):
| _________^
25 | | raise Exception("Evil I say!")
26 | | raise Exception("Evil I say!")
| |__________________________________________^ B017
26 |
27 | def context_manager_raises(self) -> None:
27 |
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"):
|