-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ruff: lint pytest #4557
Ruff: lint pytest #4557
Conversation
@@ -59,6 +63,13 @@ ignore = [ | |||
[lint.flake8-annotations] | |||
ignore-fully-untyped = true | |||
|
|||
[lint.flake8-pytest-style] | |||
parametrize-names-type = "csv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with "csv"
because it's the most used in the project currently. But I think I'd prefer the default of "tuple"
See https://docs.astral.sh/ruff/rules/pytest-parametrize-names-wrong-type/ & https://docs.astral.sh/ruff/settings/#lint_flake8-pytest-style_parametrize-names-type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be OK to change to tuple
if we have the linter to enforce it in future PRs.
Can ruff
automatically fix that?
If I understand correctly the maintenance policy in the repository is that implicit config is better than explicit config, so it would be nice if we can rely on defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can autofix it, it's simply marked as an "unsafe fix". Happy to provide a follow-up PR or add it here (there's 41 hits)
"PYI", # flake8-pyi | ||
"UP", # pyupgrade | ||
"TRY", | ||
"YTT", # flake8-2020 | ||
] | ||
ignore = [ | ||
"PT007", # temporarily disabled, TODO: configure and standardize to preference | ||
"PT011", # temporarily disabled, TODO: tighten expected error | ||
"PT012", # pytest-raises-with-multiple-statements, avoid extra dummy methods for a few lines, sometimes we explicitly assert in case of no error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt like the changes PT012
was asking for didn't fit the style of a handful of current tests, would've added more boilerplate without helping that much prevent false-negatives.
d390533
to
a72efef
Compare
Patch coverage failure because coverage is not aggregated (jaraco/skeleton#130) AND integration tests are not run when modifying integration tests. To be considered ? |
"PYI", # flake8-pyi | ||
"UP", # pyupgrade | ||
"TRY", | ||
"YTT", # flake8-2020 | ||
] | ||
ignore = [ | ||
"PT007", # temporarily disabled, TODO: configure and standardize to preference | ||
"PT011", # temporarily disabled, TODO: tighten expected error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to see astral-sh/ruff#5157 or astral-sh/ruff#6840 before enabling PT011
yield | ||
|
||
request.addfinalizer(_debug_info) | ||
# Let's provide the maximum amount of information possible in the case | ||
# it is necessary to debug the tests directly from the CI logs. | ||
print("~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~") | ||
print("Temporary directory:") | ||
map(print, tmp_path.glob("*")) | ||
print("Virtual environment:") | ||
run([venv_python, "-m", "pip", "freeze"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setuptools/tests/test_integration.py
Outdated
|
||
yield cmd | ||
|
||
# undo the monkeypatch, particularly needed under | ||
# windows because of kept handle on cwd | ||
monkeypatch.undo() | ||
new_cwd.remove() | ||
user_base.remove() | ||
user_site.remove() | ||
install_dir.remove() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6cb42ab
to
48a741d
Compare
@@ -21,7 +21,7 @@ | |||
|
|||
@pytest.fixture(autouse=True) | |||
def isolated_dir(tmpdir_cwd): | |||
yield | |||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d93e6e1
to
017a828
Compare
017a828
to
bafdf65
Compare
bafdf65
to
35873f1
Compare
b83320b
to
62b0419
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much.
@@ -59,6 +63,13 @@ ignore = [ | |||
[lint.flake8-annotations] | |||
ignore-fully-untyped = true | |||
|
|||
[lint.flake8-pytest-style] | |||
parametrize-names-type = "csv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be OK to change to tuple
if we have the linter to enforce it in future PRs.
Can ruff
automatically fix that?
If I understand correctly the maintenance policy in the repository is that implicit config is better than explicit config, so it would be nice if we can rely on defaults.
The integration tests are not included on purpose to reduce the pressure on the CI resources of the organization. For the aggregation, let's see what comes out of |
Integration test was also excluded from coverage since I originally wrote that comment: #4589 |
59611f7
to
baf6042
Compare
baf6042
to
bf21018
Compare
bf21018
to
14efd5f
Compare
14efd5f
to
9fc7bbc
Compare
Thank you very much! |
Summary of changes
In this PR I've enabled https://docs.astral.sh/ruff/rules/#flake8-pytest-style-pt linting in Ruff and configured to the current preferences.
Pull Request Checklist
newsfragments/
. (no user facing changes)(See documentation for details)