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.

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

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

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)

@@ -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

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

@Avasam Avasam Aug 11, 2024

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

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

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.

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.

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

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.

@@ -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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants