Skip to content

Commit

Permalink
Fix pytest-parametrize-names-wrong-type (PT006) to edit both `argna…
Browse files Browse the repository at this point in the history
…mes` and `argvalues` if both of them are single-element tuples/lists (#14699)

## Summary

Close #11243. Fix `pytest-parametrize-names-wrong-type (PT006)` to edit
both `argnames` and `argvalues` if both of them are single-element
tuples/lists.

```python
# Before fix
@pytest.mark.parametrize(("x",), [(1,), (2,)])
def test_foo(x):
    ...

# After fix:
@pytest.mark.parametrize("x", [1, 2])
def test_foo(x):
    ...
```

## Test Plan

New test cases
  • Loading branch information
harupy authored Dec 9, 2024
1 parent 8df4983 commit 3d9ac53
Show file tree
Hide file tree
Showing 9 changed files with 901 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,78 @@ def test_not_decorator(param1, param2):
@pytest.mark.parametrize(argnames=("param1,param2"), argvalues=[(1, 2), (3, 4)])
def test_keyword_arguments(param1, param2):
...


@pytest.mark.parametrize(("param",), [(1,), (2,)])
def test_single_element_tuple(param):
...


@pytest.mark.parametrize(("param",), [[1], [2]])
def test_single_element_list(param):
...


@pytest.mark.parametrize(("param",), [[1], [2]])
def test_single_element_list(param):
...


# Unsafe fix
@pytest.mark.parametrize(
(
# comment
"param",
),
[[1], [2]],
)
def test_comment_in_argnames(param):
...

# Unsafe fix
@pytest.mark.parametrize(
("param",),
[
(
# comment
1,
),
(2,),
],
)
def test_comment_in_argvalues(param):
...


# Safe fix
@pytest.mark.parametrize(
("param",),
[
(1,),
# comment
(2,),
],
)
def test_comment_between_argvalues_items(param):
...


# A fix should be suggested for `argnames`, but not for `argvalues`.
@pytest.mark.parametrize(
("param",),
[
(1,),
(2, 3),
],
)
def test_invalid_argvalues(param):
"""
pytest throws the following error for this test:
------------------------------------------------
a.py::test_comment_between_argvalues_items: in "parametrize" the number of names (1):
('param',)
must be equal to the number of values (2):
(2, 3)
------------------------------------------------
"""
...
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import pytest

@pytest.mark.parametrize(("param",), [[1], [2]])
def test_PT006_and_PT007_do_not_conflict(param):
...
20 changes: 20 additions & 0 deletions crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,26 @@ mod tests {
Ok(())
}

/// This test ensure that PT006 and PT007 don't conflict when both of them suggest a fix that
/// edits `argvalues` for `pytest.mark.parametrize`.
#[test]
fn test_pytest_style_pt006_and_pt007() -> Result<()> {
let diagnostics = test_path(
Path::new("flake8_pytest_style")
.join(Path::new("PT006_and_PT007.py"))
.as_path(),
&settings::LinterSettings {
preview: PreviewMode::Enabled,
..settings::LinterSettings::for_rules(vec![
Rule::PytestParametrizeNamesWrongType,
Rule::PytestParametrizeValuesWrongType,
])
},
)?;
assert_messages!("PT006_and_PT007", diagnostics);
Ok(())
}

#[test_case(Rule::PytestParametrizeNamesWrongType, Path::new("PT006.py"))]
fn test_pytest_style_preview(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ fn get_parametrize_name_range(
}

/// PT006
fn check_names(checker: &mut Checker, call: &ExprCall, expr: &Expr) {
fn check_names(checker: &mut Checker, call: &ExprCall, expr: &Expr, argvalues: &Expr) {
let names_type = checker.settings.flake8_pytest_style.parametrize_names_type;

match expr {
Expand Down Expand Up @@ -414,7 +414,7 @@ fn check_names(checker: &mut Checker, call: &ExprCall, expr: &Expr) {
}
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
if elts.len() == 1 {
handle_single_name(checker, expr, &elts[0]);
handle_single_name(checker, expr, &elts[0], argvalues);
} else {
match names_type {
types::ParametrizeNameType::Tuple => {}
Expand Down Expand Up @@ -458,7 +458,7 @@ fn check_names(checker: &mut Checker, call: &ExprCall, expr: &Expr) {
}
Expr::List(ast::ExprList { elts, .. }) => {
if elts.len() == 1 {
handle_single_name(checker, expr, &elts[0]);
handle_single_name(checker, expr, &elts[0], argvalues);
} else {
match names_type {
types::ParametrizeNameType::List => {}
Expand Down Expand Up @@ -678,23 +678,85 @@ fn check_duplicates(checker: &mut Checker, values: &Expr) {
}
}

fn handle_single_name(checker: &mut Checker, expr: &Expr, value: &Expr) {
fn handle_single_name(checker: &mut Checker, argnames: &Expr, value: &Expr, argvalues: &Expr) {
let mut diagnostic = Diagnostic::new(
PytestParametrizeNamesWrongType {
single_argument: true,
expected: types::ParametrizeNameType::Csv,
},
expr.range(),
argnames.range(),
);

let node = value.clone();
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
checker.generator().expr(&node),
expr.range(),
)));
// If `argnames` and all items in `argvalues` are single-element sequences,
// they all should be unpacked. Here's an example:
//
// ```python
// @pytest.mark.parametrize(("x",), [(1,), (2,)])
// def test_foo(x):
// assert isinstance(x, int)
// ```
//
// The code above should be transformed into:
//
// ```python
// @pytest.mark.parametrize("x", [1, 2])
// def test_foo(x):
// assert isinstance(x, int)
// ```
//
// Only unpacking `argnames` would break the test:
//
// ```python
// @pytest.mark.parametrize("x", [(1,), (2,)])
// def test_foo(x):
// assert isinstance(x, int) # fails because `x` is a tuple, not an int
// ```
let argvalues_edits = unpack_single_element_items(checker, argvalues);
let argnames_edit = Edit::range_replacement(checker.generator().expr(value), argnames.range());
let fix = if checker.comment_ranges().intersects(argnames_edit.range())
|| argvalues_edits
.iter()
.any(|edit| checker.comment_ranges().intersects(edit.range()))
{
Fix::unsafe_edits(argnames_edit, argvalues_edits)
} else {
Fix::safe_edits(argnames_edit, argvalues_edits)
};
diagnostic.set_fix(fix);
checker.diagnostics.push(diagnostic);
}

/// Generate [`Edit`]s to unpack single-element lists or tuples in the given [`Expr`].
/// For instance, `[(1,) (2,)]` will be transformed into `[1, 2]`.
fn unpack_single_element_items(checker: &Checker, expr: &Expr) -> Vec<Edit> {
let (Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. })) = expr
else {
return vec![];
};

let mut edits = Vec::with_capacity(elts.len());
for value in elts {
let (Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. })) =
value
else {
return vec![];
};

let [elt] = elts.as_slice() else {
return vec![];
};

if matches!(elt, Expr::Starred(_)) {
return vec![];
}

edits.push(Edit::range_replacement(
checker.generator().expr(elt),
value.range(),
));
}
edits
}

fn handle_value_rows(
checker: &mut Checker,
elts: &[Expr],
Expand Down Expand Up @@ -801,12 +863,18 @@ pub(crate) fn parametrize(checker: &mut Checker, call: &ExprCall) {
}

if checker.enabled(Rule::PytestParametrizeNamesWrongType) {
if let Some(names) = if checker.settings.preview.is_enabled() {
let names = if checker.settings.preview.is_enabled() {
call.arguments.find_argument("argnames", 0)
} else {
call.arguments.find_positional(0)
} {
check_names(checker, call, names);
};
let values = if checker.settings.preview.is_enabled() {
call.arguments.find_argument("argvalues", 1)
} else {
call.arguments.find_positional(1)
};
if let (Some(names), Some(values)) = (names, values) {
check_names(checker, call, names, values);
}
}
if checker.enabled(Rule::PytestParametrizeValuesWrongType) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
---
source: crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs
snapshot_kind: text
---
PT006_and_PT007.py:3:26: PT006 [*] Wrong type passed to first argument of `pytest.mark.parametrize`; expected `str`
|
1 | import pytest
2 |
3 | @pytest.mark.parametrize(("param",), [[1], [2]])
| ^^^^^^^^^^ PT006
4 | def test_PT006_and_PT007_do_not_conflict(param):
5 | ...
|
= help: Use a string for the first argument

Safe fix
1 1 | import pytest
2 2 |
3 |-@pytest.mark.parametrize(("param",), [[1], [2]])
3 |+@pytest.mark.parametrize("param", [1, 2])
4 4 | def test_PT006_and_PT007_do_not_conflict(param):
5 5 | ...

PT006_and_PT007.py:3:39: PT007 [*] Wrong values type in `pytest.mark.parametrize` expected `list` of `tuple`
|
1 | import pytest
2 |
3 | @pytest.mark.parametrize(("param",), [[1], [2]])
| ^^^ PT007
4 | def test_PT006_and_PT007_do_not_conflict(param):
5 | ...
|
= help: Use `list` of `tuple` for parameter values

Unsafe fix
1 1 | import pytest
2 2 |
3 |-@pytest.mark.parametrize(("param",), [[1], [2]])
3 |+@pytest.mark.parametrize(("param",), [(1,), [2]])
4 4 | def test_PT006_and_PT007_do_not_conflict(param):
5 5 | ...

PT006_and_PT007.py:3:44: PT007 [*] Wrong values type in `pytest.mark.parametrize` expected `list` of `tuple`
|
1 | import pytest
2 |
3 | @pytest.mark.parametrize(("param",), [[1], [2]])
| ^^^ PT007
4 | def test_PT006_and_PT007_do_not_conflict(param):
5 | ...
|
= help: Use `list` of `tuple` for parameter values

Unsafe fix
1 1 | import pytest
2 2 |
3 |-@pytest.mark.parametrize(("param",), [[1], [2]])
3 |+@pytest.mark.parametrize(("param",), [[1], (2,)])
4 4 | def test_PT006_and_PT007_do_not_conflict(param):
5 5 | ...
Loading

0 comments on commit 3d9ac53

Please sign in to comment.