Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ruff: lint pytest #4557

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Ruff: lint pytest #4557

merged 1 commit into from
Nov 4, 2024

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Aug 11, 2024 •
edited
Loading

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

  • Changes have tests (these are test changes, all existing tests should pass)
  • News fragment added in newsfragments/. (no user facing changes)
    (See documentation for details)

Avasam
Avasam commented Aug 11, 2024
View reviewed changes
ruff.toml
@@ -59,6 +63,13 @@ ignore = [
[lint.flake8-annotations]
ignore-fully-untyped = true

[lint.flake8-pytest-style]
parametrize-names-type = "csv"
Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

@Avasam Avasam Oct 31, 2024 •
edited
Loading

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)

Avasam
Avasam commented Aug 11, 2024
View reviewed changes
ruff.toml
"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
Copy link
Contributor Author

@Avasam Avasam Aug 11, 2024 •
edited
Loading

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.

@Avasam Avasam force-pushed the Ruff--lint-pytest branch 2 times, most recently from d390533 to a72efef Compare August 11, 2024 23:26
@Avasam
Copy link
Contributor Author

Avasam commented Aug 12, 2024 •
edited
Loading

Patch coverage failure because coverage is not aggregated (jaraco/skeleton#130) AND integration tests are not run when modifying integration tests. To be considered ?

Avasam
Avasam commented Aug 12, 2024
View reviewed changes
ruff.toml
"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
Copy link
Contributor Author

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

Avasam
Avasam commented Aug 16, 2024
View reviewed changes
setuptools/tests/integration/test_pip_install_sdist.py
Comment on lines +114 to +122
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"])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.astral.sh/ruff/rules/pytest-fixture-finalizer-callback/

Avasam
Avasam commented Aug 16, 2024
View reviewed changes
setuptools/tests/test_integration.py Outdated
Comment on lines 61 to 70

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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.astral.sh/ruff/rules/pytest-fixture-finalizer-callback/

Avasam
Avasam commented Aug 21, 2024
View reviewed changes
setuptools/tests/test_setuptools.py
@@ -21,7 +21,7 @@

@pytest.fixture(autouse=True)
def isolated_dir(tmpdir_cwd):
yield
return
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.astral.sh/ruff/rules/pytest-useless-yield-fixture/

@Avasam Avasam force-pushed the Ruff--lint-pytest branch 4 times, most recently from d93e6e1 to 017a828 Compare August 27, 2024 15:57
abravalheri
abravalheri approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@abravalheri abravalheri left a 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.

ruff.toml
@@ -59,6 +63,13 @@ ignore = [
[lint.flake8-annotations]
ignore-fully-untyped = true

[lint.flake8-pytest-style]
parametrize-names-type = "csv"
Copy link
Contributor

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.

@abravalheri
Copy link
Contributor

Patch coverage failure because coverage is not aggregated (jaraco/skeleton#130) AND integration tests are not run when modifying integration tests. To be considered ?

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 skeleton.

@Avasam
Copy link
Contributor Author

Avasam commented Oct 31, 2024

Patch coverage failure because coverage is not aggregated (jaraco/skeleton#130) AND integration tests are not run when modifying integration tests. To be considered ?

The integration tests are not included on purpose to reduce the pressure on the CI resources of the organization.

Integration test was also excluded from coverage since I originally wrote that comment: #4589

@abravalheri abravalheri merged commit b27393a into pypa:main Nov 4, 2024
18 of 19 checks passed
@abravalheri
Copy link
Contributor

Thank you very much!

@Avasam Avasam deleted the Ruff--lint-pytest branch November 4, 2024 18:15
@Avasam Avasam mentioned this pull request Nov 4, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abravalheri abravalheri abravalheri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Avasam @abravalheri