[`flake8-pytest-style`] Implement duplicate parameterized fixture detection (`PT014`) by harupy · Pull Request #6598 · astral-sh/ruff

harupy · 2023-08-15T13:58:32Z

Summary

#6597

Test Plan

Unit tests

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

github-actions · 2023-08-15T14:31:58Z

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+23, -0, 0 error(s))

airflow (+12, -0)

+ tests/always/test_secrets_local_filesystem.py:103:13: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize`
+ tests/models/test_dagrun.py:2537:9: PT014 Duplicate of test case at index 5 in `@pytest_mark.parametrize`
+ tests/operators/test_bash.py:181:13: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize`
+ tests/providers/cncf/kubernetes/operators/test_pod.py:1214:13: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize`
+ tests/providers/databricks/operators/test_databricks_sql.py:233:9: PT014 Duplicate of test case at index 5 in `@pytest_mark.parametrize`
+ tests/providers/docker/operators/test_docker.py:533:13: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize`
+ tests/providers/google/cloud/hooks/test_cloud_storage_transfer_service.py:488:13: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametrize`
+ tests/providers/google/cloud/hooks/test_cloud_storage_transfer_service.py:489:13: PT014 Duplicate of test case at index 1 in `@pytest_mark.parametrize`
+ tests/providers/google/cloud/hooks/test_cloud_storage_transfer_service.py:493:13: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize`
+ tests/providers/presto/hooks/test_presto.py:197:13: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize`
+ tests/providers/trino/hooks/test_trino.py:216:13: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize`
+ tests/www/views/test_views.py:534:9: PT014 Duplicate of test case at index 4 in `@pytest_mark.parametrize`

securedrop (+1, -0)

+ molecule/testinfra/common/test_fpf_apt_repo.py:64:9: PT014 Duplicate of test case at index 1 in `@pytest_mark.parametrize`

scikit-build (+1, -0)

+ tests/test_setup.py:545:9: PT014 Duplicate of test case at index 3 in `@pytest_mark.parametrize`

sphinx (+9, -0)

+ 
+    |
+    |     ^^^^^^^^^^^^^^^^ PT014
+ 35 |     ("a * b", "a * b"),                         # Mult
+ 36 |     ("sys", "sys"),                             # Name, NameConstant
+ 37 |     ("1234", "1234"),                           # Num
+ 38 |     ("not a", "not a"),                         # Not
+ 39 |     ("a or b", "a or b"),                       # Or
+ tests/test_pycode_ast.py:37:5: PT014 Duplicate of test case at index 10 in `@pytest_mark.parametrize`

Rules changed: 1

Rule	Changes	Additions	Removals
PT014	15	15	0

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.8±0.03ms    10.8 MB/sec    1.04      3.9±0.02ms    10.4 MB/sec
formatter/numpy/ctypeslib.py               1.00   731.9±11.11µs    22.8 MB/sec    1.03   754.4±11.32µs    22.1 MB/sec
formatter/numpy/globals.py                 1.00     75.1±0.29µs    39.3 MB/sec    1.06     79.9±8.66µs    37.0 MB/sec
formatter/pydantic/types.py                1.00  1477.4±11.35µs    17.3 MB/sec    1.04  1530.4±12.56µs    16.7 MB/sec
linter/all-rules/large/dataset.py          1.01     10.7±0.09ms     3.8 MB/sec    1.00     10.7±0.05ms     3.8 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.9±0.01ms     5.7 MB/sec    1.00      2.9±0.01ms     5.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    327.7±0.78µs     9.0 MB/sec    1.00    326.4±1.65µs     9.0 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.5±0.04ms     4.6 MB/sec    1.00      5.5±0.03ms     4.6 MB/sec
linter/default-rules/large/dataset.py      1.01      5.7±0.05ms     7.2 MB/sec    1.00      5.6±0.04ms     7.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1192.1±3.23µs    14.0 MB/sec    1.00   1184.2±2.31µs    14.1 MB/sec
linter/default-rules/numpy/globals.py      1.00    124.2±0.67µs    23.7 MB/sec    1.00    124.3±0.53µs    23.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.5±0.01ms    10.1 MB/sec    1.00      2.5±0.01ms    10.2 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      4.3±0.07ms     9.4 MB/sec    1.00      4.3±0.06ms     9.4 MB/sec
formatter/numpy/ctypeslib.py               1.01   845.4±11.96µs    19.7 MB/sec    1.00   837.0±12.85µs    19.9 MB/sec
formatter/numpy/globals.py                 1.01     85.9±1.64µs    34.4 MB/sec    1.00     85.2±3.03µs    34.6 MB/sec
formatter/pydantic/types.py                1.00  1688.0±23.15µs    15.1 MB/sec    1.02  1714.8±28.04µs    14.9 MB/sec
linter/all-rules/large/dataset.py          1.01     12.7±0.13ms     3.2 MB/sec    1.00     12.6±0.11ms     3.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.5±0.04ms     4.8 MB/sec    1.00      3.5±0.04ms     4.8 MB/sec
linter/all-rules/numpy/globals.py          1.01   438.1±11.54µs     6.7 MB/sec    1.00    434.8±4.60µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.6±0.08ms     3.9 MB/sec    1.00      6.6±0.13ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.00      6.9±0.06ms     5.9 MB/sec    1.00      6.9±0.06ms     5.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01  1482.2±17.84µs    11.2 MB/sec    1.00  1473.3±18.77µs    11.3 MB/sec
linter/default-rules/numpy/globals.py      1.00    175.2±4.52µs    16.8 MB/sec    1.01    176.4±5.08µs    16.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.1±0.03ms     8.2 MB/sec    1.00      3.1±0.04ms     8.2 MB/sec

harupy · 2023-08-15T14:43:29Z

crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py

flake8_pytest_style result:

> flake8 crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py:4:2: PT014 found duplicate test cases (1, 2) in @pytest.mark.parametrize crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py:14:2: PT014 found duplicate test cases (1, 2) in @pytest.mark.parametrize crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py:14:2: PT014 found duplicate test cases (3, 4) in @pytest.mark.parametrize crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py:19:2: PT014 found duplicate test cases (1, 2) in @pytest.mark.parametrize

Signed-off-by: harupy <hkawamura0130@gmail.com>

zanieb

Great work :) some minor comments

zanieb · 2023-08-15T16:16:16Z

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

+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let PytestDuplicateParametrizeTestCases { indices } = self;
+        format!("Found duplicate test cases {indices:?} in `@pytest.mark.parametrize`")


should we say "at indices ..."? I wasn't sure what the message meant at first.

Can we raise PT014 against items that should be removed?

# Example [1, 1, 2] ^ PT014 ...

This allows us to remove indices.

I think it makes sense to use the range of the duplicated item, so that we underline the duplicated item specifically

# Example [100, 100, 100, 200] ^^^ ^^^

If we have multiple duplicated items, we underline them?

I don't think we support highlighting multiple ranges. In that case, they'd each need to be a new violation which seems okay.

I think in the duplicate value rule, we just highlight the second value (i.e., the one that is a duplicate). That seems reasonable to me. (We could also mention the index of which it's a duplicate in the message.)

# Example [100, 100, 100, 200] ^^^ PT014: duplicate of item at {0}

so it should look like this?

Yeah, and the same for the next 100 -- one violation for each duplicate (but no violation for the first 100), would be my suggestion.

zanieb · 2023-08-15T16:17:33Z

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

 }

+fn find_duplicates(elts: &[Expr]) -> Vec<(usize, usize)> {
+    let mut duplicates: Vec<(usize, usize)> = Vec::new();


Should we use a SmallVec since it's unlikely for there to be many duplicates?

I tend to prefer Vec unless we can demonstrate that SmallVec has better performance. In this case, we probably don't expect any duplicates in general, in which case this will never allocate anyway, so seems okay in my opinion.

zanieb · 2023-08-15T16:18:50Z

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

+}
+
+/// PT014
+fn check_duplicates(checker: &mut Checker, values: &Expr) {


Is it feasible to factor this so the type of values is more meaningful/narrow?

charliermarsh · 2023-08-15T17:14:58Z

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

+        if ComparableExpr::from(elt1) == ComparableExpr::from(elt2) {
+            duplicates.push((idx1 + 1, idx2 + 1));
+        }
+    }


I think I'd suggest using an FxHashSet or FxHashMap here... As-is, this is quadratic, since we're checking every value against every other value. It's sometimes better to use a vector if you know the number of items is really small, but this also means we're re-computing the hash many times over.

PT014

5073847

harupy commented Aug 15, 2023

View reviewed changes

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

harupy added 3 commits August 15, 2023 23:08

Fix

17a8bdc

Fix doc

4b8529e

Remove 'at indices'

e5cd579

Copy original

0d96a04

harupy commented Aug 15, 2023

View reviewed changes

harupy marked this pull request as ready for review August 15, 2023 14:43

harupy added 2 commits August 15, 2023 23:53

Remove with_capacity

c6eb566

Signed-off-by: harupy <hkawamura0130@gmail.com>

Fix clippy error

2efb740

harupy changed the title ~~Implement PT014~~ Implement PT014 Aug 15, 2023

zanieb approved these changes Aug 15, 2023

View reviewed changes

charliermarsh reviewed Aug 15, 2023

View reviewed changes

Address comments

ba46c21

harupy requested review from charliermarsh and zanieb August 16, 2023 02:02

Tweaks

9d5dce4

charliermarsh added the rule Implementing or modifying a lint rule label Aug 16, 2023

charliermarsh changed the title ~~Implement PT014~~ [flake8-pytest-style] Implement duplicate parameterized fixture detection (PT014) Aug 16, 2023

charliermarsh enabled auto-merge (squash) August 16, 2023 03:27

charliermarsh merged commit d9a81f4 into astral-sh:main Aug 16, 2023

harupy deleted the PT014 branch August 16, 2023 03:40

chenrui333 mentioned this pull request Aug 17, 2023

ruff 0.0.285 Homebrew/homebrew-core#139823

Merged

Conversation

harupy commented Aug 15, 2023

Summary

Test Plan

Uh oh!

Uh oh!

github-actions bot commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Uh oh!

harupy Aug 15, 2023 • 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!

zanieb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy Aug 15, 2023 • 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!

charliermarsh Aug 15, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Aug 15, 2023 •

edited

Loading

harupy Aug 15, 2023 •

edited

Loading

harupy Aug 15, 2023 •

edited

Loading

charliermarsh Aug 15, 2023 •

edited

Loading