From a48dafa2f3b11cc901207bcb8c937cef6cbbb66c Mon Sep 17 00:00:00 2001 From: harupy Date: Thu, 10 Aug 2023 22:37:00 +0900 Subject: [PATCH 1/4] PT008 and PT009 docs --- .../flake8_pytest_style/rules/assertion.rs | 22 +++++++++++++++++ .../rules/flake8_pytest_style/rules/patch.rs | 24 +++++++++++++++++++ 2 files changed, 46 insertions(+) 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 c70982d93cbab..a596c7c333397 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs @@ -145,6 +145,28 @@ impl Violation for PytestAssertAlwaysFalse { } } +/// ## What it does +/// Checks for `unittest`'s assertion methods. +/// +/// ## Why is this bad? +/// To make use of `pytest`'s assertion rewriting, a regular `assert` statement is +/// preferred over `unittest`'s assertion methods. +/// +/// ## Example +/// class TestFoo(unittest.TestCase): +/// def test_foo(self): +/// self.assertEqual(a, b) +/// ``` +/// +/// Use instead: +/// ```python +/// class TestFoo(unittest.TestCase): +/// def test_foo(self): +/// assert a == b +/// ``` +/// +/// - [`pytest` documentation: `Assertion introspection details`](https://docs.pytest.org/en/7.1.x/how-to/assert.html#assertion-introspection-details) + #[violation] pub struct PytestUnittestAssertion { assertion: String, diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs index fd6f10827697c..e91e8657abdc7 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs @@ -5,6 +5,30 @@ use ruff_python_ast::visitor; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::{self as ast, Expr, Parameters, Ranged}; +/// ## What it does +/// Checks for monkey patching calls that use `lambda` as the new value. +/// +/// ## Why is this bad? +/// `return_value` conveys the intent more clearly and allows using methods for +/// verifying the number of calls or the arguments passed to the patched function +/// (e.g., `assert_called_once_with`). +/// +/// ## Example +/// def test_foo(mocker): +/// mocker.patch('module.target', lambda x, y: 7) +/// ``` +/// +/// Use instead: +/// ```python +/// def test_foo(mocker): +/// mocker.patch('module.target', return_value=7) +/// # if lambda parameters are used, it's not a violation +/// mocker.patch('module.other_target', lambda x, y: x) +/// ``` +/// +/// ## References +/// - [`unittest.mock.patch`](https://docs.python.org/3/library/unittest.mock.html#unittest.mock.patch) +/// - [`pytest-mock`](https://pypi.org/project/pytest-mock/) #[violation] pub struct PytestPatchWithLambda; From b65821359a94d9036e8480e50cfeeab534ed783a Mon Sep 17 00:00:00 2001 From: harupy Date: Thu, 10 Aug 2023 22:38:25 +0900 Subject: [PATCH 2/4] Fix --- crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs index e91e8657abdc7..ef3dbf754b6cf 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs @@ -27,7 +27,7 @@ use ruff_python_ast::{self as ast, Expr, Parameters, Ranged}; /// ``` /// /// ## References -/// - [`unittest.mock.patch`](https://docs.python.org/3/library/unittest.mock.html#unittest.mock.patch) +/// - [Python documentation: `unittest.mock.patch`](https://docs.python.org/3/library/unittest.mock.html#unittest.mock.patch) /// - [`pytest-mock`](https://pypi.org/project/pytest-mock/) #[violation] pub struct PytestPatchWithLambda; From a80d55dc3f8375fb3c1b5dd33449803be552e8ff Mon Sep 17 00:00:00 2001 From: harupy Date: Thu, 10 Aug 2023 22:59:42 +0900 Subject: [PATCH 3/4] Fix examples --- .../ruff/src/rules/flake8_pytest_style/rules/assertion.rs | 1 + crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) 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 a596c7c333397..b9cf5c71b39f1 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs @@ -153,6 +153,7 @@ impl Violation for PytestAssertAlwaysFalse { /// preferred over `unittest`'s assertion methods. /// /// ## Example +/// ```python /// class TestFoo(unittest.TestCase): /// def test_foo(self): /// self.assertEqual(a, b) diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs index ef3dbf754b6cf..d8d5ff703ff74 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs @@ -14,16 +14,17 @@ use ruff_python_ast::{self as ast, Expr, Parameters, Ranged}; /// (e.g., `assert_called_once_with`). /// /// ## Example +/// ```python /// def test_foo(mocker): -/// mocker.patch('module.target', lambda x, y: 7) +/// mocker.patch("module.target", lambda x, y: 7) /// ``` /// /// Use instead: /// ```python /// def test_foo(mocker): -/// mocker.patch('module.target', return_value=7) +/// mocker.patch("module.target", return_value=7) /// # if lambda parameters are used, it's not a violation -/// mocker.patch('module.other_target', lambda x, y: x) +/// mocker.patch("module.other_target", lambda x, y: x) /// ``` /// /// ## References From f00b99cdeb5d9d6a3a84827abe81ec8f62fcf8a5 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 10 Aug 2023 21:18:05 -0400 Subject: [PATCH 4/4] Tweak --- .../flake8_pytest_style/rules/assertion.rs | 9 +++++---- .../rules/flake8_pytest_style/rules/patch.rs | 18 +++++++++++++----- 2 files changed, 18 insertions(+), 9 deletions(-) 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 b9cf5c71b39f1..dbb571c5272be 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs @@ -146,11 +146,11 @@ impl Violation for PytestAssertAlwaysFalse { } /// ## What it does -/// Checks for `unittest`'s assertion methods. +/// Checks for uses of assertion methods from the `unittest` module. /// /// ## Why is this bad? -/// To make use of `pytest`'s assertion rewriting, a regular `assert` statement is -/// preferred over `unittest`'s assertion methods. +/// To make use of `pytest`'s assertion rewriting, a regular `assert` statement +/// is preferred over `unittest`'s assertion methods. /// /// ## Example /// ```python @@ -166,7 +166,8 @@ impl Violation for PytestAssertAlwaysFalse { /// assert a == b /// ``` /// -/// - [`pytest` documentation: `Assertion introspection details`](https://docs.pytest.org/en/7.1.x/how-to/assert.html#assertion-introspection-details) +/// ## References +/// - [`pytest` documentation: Assertion introspection details](https://docs.pytest.org/en/7.1.x/how-to/assert.html#assertion-introspection-details) #[violation] pub struct PytestUnittestAssertion { diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs index d8d5ff703ff74..cfd9e7d8d35a9 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs @@ -6,12 +6,19 @@ use ruff_python_ast::visitor::Visitor; use ruff_python_ast::{self as ast, Expr, Parameters, Ranged}; /// ## What it does -/// Checks for monkey patching calls that use `lambda` as the new value. +/// Checks for mocked calls that use a dummy `lambda` function instead of +/// `return_value`. /// /// ## Why is this bad? -/// `return_value` conveys the intent more clearly and allows using methods for -/// verifying the number of calls or the arguments passed to the patched function -/// (e.g., `assert_called_once_with`). +/// When patching calls, an explicit `return_value` better conveys the intent +/// than a `lambda` function, assuming the `lambda` does not use the arguments +/// passed to it. +/// +/// `return_value` is also robust to changes in the patched function's +/// signature, and enables additional assertions to verify behavior. For +/// example, `return_value` allows for verification of the number of calls or +/// the arguments passed to the patched function via `assert_called_once_with` +/// and related methods. /// /// ## Example /// ```python @@ -23,7 +30,8 @@ use ruff_python_ast::{self as ast, Expr, Parameters, Ranged}; /// ```python /// def test_foo(mocker): /// mocker.patch("module.target", return_value=7) -/// # if lambda parameters are used, it's not a violation +/// +/// # If the lambda makes use of the arguments, no diagnostic is emitted. /// mocker.patch("module.other_target", lambda x, y: x) /// ``` ///