Fix `pytest.mark.parametrize` rules to check calls instead of decorators by harupy · Pull Request #14515 · astral-sh/ruff

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

…ze` calls" (`PT006`) (#15327) Co-authored-by: Micha Reiser <micha@reiser.io> Resolves #15324. Stabilizes the behavior changes introduced in #14515.

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

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

BrewTestBot mentioned this pull request Nov 29, 2024

ruff 0.8.1 Homebrew/homebrew-core#199449

Merged

MichaReiser mentioned this pull request Dec 2, 2024

Revert: [pyflakes] Avoid false positives in @no_type_check contexts (F821, F722) (#14615) #14726

Merged

This was referenced Dec 2, 2024

PT006: ignores parametrize markers on classes #2955

Closed

Remove @ in pytest.mark.parametrize rule messages #14770

Merged

MichaReiser mentioned this pull request Jan 8, 2025

[flake8-pytest-style] Stabilize "Detect more pytest.mark.parametrize calls" (PT006) #15327

Merged

Conversation

harupy commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

harupy commented Nov 21, 2024

Uh oh!

Uh oh!

github-actions bot commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

Uh oh!

harupy Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

harupy Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy Nov 23, 2024

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented Nov 25, 2024

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

harupy commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented Nov 25, 2024

Uh oh!

harupy commented Nov 25, 2024

Uh oh!

MichaReiser commented Nov 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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