astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
33.1k stars 1.11k forks source link

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

Closed harupy closed 20 hours ago

harupy commented 4 days ago

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

Test Plan

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

harupy commented 4 days ago

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)
github-actions[bot] commented 4 days ago

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 |

MichaReiser commented 23 hours ago

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 :)

harupy commented 22 hours ago

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

  1. Only calls in decorators are covered -> Any calls are covered.
  2. Only positional arguments are covered -> Both positional and keyword arguments are covered.
MichaReiser commented 22 hours ago

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 commented 21 hours ago

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

MichaReiser commented 21 hours ago

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