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-pytest-style] Implement pytest-unittest-raises-assertion (PT027) #6554

Merged
merged 7 commits into from
Aug 14, 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
48 changes: 48 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_0.py
Original file line number Diff line number Diff line change
@@ -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
12 changes: 12 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pytest_style/PT027_1.py
Original file line number Diff line number Diff line change
@@ -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
7 changes: 7 additions & 0 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
12 changes: 12 additions & 0 deletions crates/ruff/src/rules/flake8_pytest_style/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
185 changes: 184 additions & 1 deletion crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<String> {
let PytestUnittestRaisesAssertion { assertion } = self;
Some(format!("Replace `{assertion}` with `pytest.raises`"))
}
}

/// PT027
pub(crate) fn unittest_raises_assertion(
checker: &Checker,
call: &ast::ExprCall,
) -> Option<Diagnostic> {
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<Cow<'a, str>> {
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() {
Expand Down
Loading
Loading