Fix `pytest.mark.parametrize` rules to check calls instead of decorators #14515

harupy · 2024-11-21T12:57:13Z

Summary

Currently, rules for pytest.mark.parametrize (e.g. /pytest-parametrize-names-wrong-type (PT006)) only check decorators, but pytest.mark.parametrize can appear as a function call as show below.

# To parametrize all tests in a module, you can assign to pytest mark


import pytest

pytestmark = pytest.mark.parametrize("n,expected", [(1, 2), (3, 4)])


class TestClass:
    def test_simple_case(self, n, expected):
        assert n + 1 == expected

    def test_weird_simple_case(self, n, expected):
        assert (n * 1) + 1 == expected

how-to guide: https://docs.pytest.org/en/stable/how-to/parametrize.html
API reference: https://docs.pytest.org/en/stable/reference/reference.html#pytest.Metafunc.parametrize

Test Plan

Existing tests and a new test case to ensure the fix is valid.

harupy · 2024-11-21T13:17:22Z

The upstream rule also checks calls:

https://github.com/m-burst/flake8-pytest-style/blob/2addab8b533ea101e25d022a37dc32d89db6c688/flake8_pytest_style/visitors/parametrize.py#L130-L132

    def visit_Call(self, node: ast.Call) -> None:
        if is_parametrize_call(node):
            self._check_parametrize_call(node)

crates/ruff_linter/src/rules/flake8_pytest_style/rules/parametrize.rs

github-actions · 2024-11-21T13:22:13Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+50 -0 violations, +0 -0 fixes in 2 projects; 53 projects unchanged)

apache/airflow (+48 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ helm_tests/airflow_aux/test_annotations.py:249:5: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:157:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:168:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:178:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:201:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:216:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:235:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:254:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:273:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:294:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:315:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:334:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:356:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:388:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:423:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:453:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:458:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:512:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:534:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:559:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:586:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:603:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:624:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:692:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ providers/tests/dbt/cloud/hooks/test_dbt.py:711:18: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
... 23 additional changes omitted for rule PT006
+ providers/tests/microsoft/azure/hooks/test_data_factory.py:151:13: PT007 Wrong values type in `@pytest.mark.parametrize` expected `list` of `tuple`

pypa/setuptools (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pkg_resources/tests/test_working_set.py:107:9: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`
+ setuptools/tests/test_egg_info.py:308:17: PT006 Wrong type passed to first argument of `@pytest.mark.parametrize`; expected `tuple`

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PT006	49	49	0	0	0
PT007	1	1	0	0	0

crates/ruff_linter/src/rules/flake8_pytest_style/rules/parametrize.rs

…rize.rs Co-authored-by: Micha Reiser <micha@reiser.io>

harupy · 2024-11-23T10:29:50Z

crates/ruff_linter/src/rules/flake8_pytest_style/rules/parametrize.rs

-                }
-            }
+pub(crate) fn parametrize(checker: &mut Checker, call: &ExprCall) {
+    if !is_pytest_parametrize(call, checker.semantic()) {


Unrelated to this PR, but PT rules don't have a seen_module guard. Shoud we add it?

Yeah we could.

Sounds like a nice good first issue.

harupy · 2024-11-23T13:12:22Z

crates/ruff_linter/src/rules/flake8_pytest_style/rules/parametrize.rs

+    if checker.enabled(Rule::PytestParametrizeNamesWrongType) {
+        if let Some(names) = call.arguments.find_argument("argnames", 0) {
+            check_names(checker, call, names);
+        }
+    }
+    if checker.enabled(Rule::PytestParametrizeValuesWrongType) {
+        let names = call.arguments.find_argument("argnames", 0);
+        let values = call.arguments.find_argument("argvalues", 1);
+        if let (Some(names), Some(values)) = (names, values) {
+            check_values(checker, names, values);
+        }
+    }
+    if checker.enabled(Rule::PytestDuplicateParametrizeTestCases) {
+        if let Some(values) = call.arguments.find_argument("argvalues", 1) {
+            check_duplicates(checker, values);


This aligns with the upstream implementation:

https://github.com/m-burst/flake8-pytest-style/blob/2addab8b533ea101e25d022a37dc32d89db6c688/flake8_pytest_style/utils.py#L96

def extract_parametrize_call_args(node: ast.Call) -> Optional[ParametrizeArgs]: """Extracts argnames, argvalues and ids from a parametrize call.""" args = get_simple_call_args(node) names_arg = args.get_argument('argnames', 0) if names_arg is None: return None values_arg = args.get_argument('argvalues', 1) ids_arg = args.get_argument('ids') return ParametrizeArgs(names_arg, values_arg, ids_arg)

MichaReiser · 2024-11-25T10:05:07Z

Thanks for the how-to-guide and API reference. That helps a lot with building the required context to review the rule. I really appreciate it :)

MichaReiser

This is great. Did you already look through the ecosystem changes? I can do it, if you haven't.

I think we should gate this change behind preview mode because I consider this a "significant increase in scope" and we should only make such changes in minor releases.

harupy · 2024-11-25T10:38:21Z

@MichaReiser Thanks for the comment. This PR has the two changes. Do we want to gate both of them?

Only calls in decorators are covered -> Any calls are covered.
Only positional arguments are covered -> Both positional and keyword arguments are covered.

MichaReiser · 2024-11-25T11:08:57Z

Oh right. Making 1. preview-only for now seems more important to me than 2. We can have a look at the ecosystem change to understand how much impact 2 has but I think it's fine to not gate 2.

harupy · 2024-11-25T11:37:20Z

For Airflow, 2 has more impacts (more changes) than 1.

MichaReiser · 2024-11-25T11:56:29Z

Right, I should have taken a closer look at the ecosystem results. We should then probably make 2 a preview style change as well. It's a bit annoying codewise but feels most in line with our versioning policy

harupy added 3 commits November 21, 2024 21:50

Check calls

778177e

Fix

e791dca

Update snapshots

1ddef63

harupy commented Nov 21, 2024

View reviewed changes

crates/ruff_linter/src/rules/flake8_pytest_style/rules/parametrize.rs Outdated Show resolved Hide resolved

Use call

3f385b6

harupy force-pushed the fix-pytest-parametrize branch from fda25ed to 3f385b6 Compare November 21, 2024 14:09

MichaReiser reviewed Nov 21, 2024

View reviewed changes

crates/ruff_linter/src/rules/flake8_pytest_style/rules/parametrize.rs Outdated Show resolved Hide resolved

harupy and others added 2 commits November 22, 2024 13:48

Update crates/ruff_linter/src/rules/flake8_pytest_style/rules/paramet…

0d52662

…rize.rs Co-authored-by: Micha Reiser <micha@reiser.io>

fmt

b6f6d0f

harupy requested a review from MichaReiser November 22, 2024 13:23

harupy commented Nov 23, 2024

View reviewed changes

Restore snapshot_kind

2f39759

MichaReiser added the rule Implementing or modifying a lint rule label Nov 25, 2024

MichaReiser approved these changes Nov 25, 2024

View reviewed changes

Gate behind preview

509c5c6

harupy requested a review from MichaReiser November 25, 2024 12:29

harupy force-pushed the fix-pytest-parametrize branch from 2f3d825 to 509c5c6 Compare November 25, 2024 12:33

harupy added 2 commits November 25, 2024 21:44

Fix snapshot path

5d1f8dc

Remove plugin_settings

837f531

MichaReiser added the preview Related to preview mode features label Nov 25, 2024

MichaReiser merged commit fa22bd6 into astral-sh:main Nov 25, 2024
20 checks passed

harupy deleted the fix-pytest-parametrize branch November 25, 2024 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `pytest.mark.parametrize` rules to check calls instead of decorators #14515

Fix `pytest.mark.parametrize` rules to check calls instead of decorators #14515

harupy commented Nov 21, 2024 •

edited

Loading

harupy commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 •

edited

Loading

harupy Nov 23, 2024 •

edited

Loading

MichaReiser Nov 25, 2024

harupy Nov 25, 2024 •

edited

Loading

harupy Nov 23, 2024

MichaReiser commented Nov 25, 2024

MichaReiser left a comment

harupy commented Nov 25, 2024 •

edited

Loading

MichaReiser commented Nov 25, 2024

harupy commented Nov 25, 2024

MichaReiser commented Nov 25, 2024

Fix pytest.mark.parametrize rules to check calls instead of decorators #14515

Fix pytest.mark.parametrize rules to check calls instead of decorators #14515

Conversation

harupy commented Nov 21, 2024 • edited Loading

Summary

Test Plan

harupy commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

harupy Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

MichaReiser Nov 25, 2024

Choose a reason for hiding this comment

harupy Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

harupy Nov 23, 2024

Choose a reason for hiding this comment

MichaReiser commented Nov 25, 2024

MichaReiser left a comment

Choose a reason for hiding this comment

harupy commented Nov 25, 2024 • edited Loading

MichaReiser commented Nov 25, 2024

harupy commented Nov 25, 2024

MichaReiser commented Nov 25, 2024

Fix `pytest.mark.parametrize` rules to check calls instead of decorators #14515

Fix `pytest.mark.parametrize` rules to check calls instead of decorators #14515

harupy commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

`ruff-ecosystem` results

harupy Nov 23, 2024 •

edited

Loading

harupy Nov 25, 2024 •

edited

Loading

harupy commented Nov 25, 2024 •

edited

Loading