-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add pytest ruff checks to test directory #12894
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
notatallshaw
force-pushed
the
add-pytest-ruff-checks
branch
4 times, most recently
from
August 4, 2024 23:09
c47b8bb
to
0c6356a
Compare
uranusjr
reviewed
Aug 5, 2024
uranusjr
reviewed
Aug 5, 2024
uranusjr
reviewed
Aug 5, 2024
This was referenced Aug 5, 2024
notatallshaw
force-pushed
the
add-pytest-ruff-checks
branch
4 times, most recently
from
August 6, 2024 00:16
06da6ec
to
d9561e4
Compare
uranusjr
reviewed
Aug 6, 2024
uranusjr
approved these changes
Aug 6, 2024
notatallshaw
force-pushed
the
add-pytest-ruff-checks
branch
from
August 6, 2024 23:14
d9561e4
to
05d79d8
Compare
uranusjr
approved these changes
Aug 7, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ruff has a lot of Pytest rules, to apply them only to the test directory I created a
ruff.toml
for the test directory itself, another solution could have been to add per-file ignore rules in the mainpyproject.toml
.Here are the current violations in the pip test directory. I have gone through each one of these rules and you can see my analysis below:
PT023 - pytest-incorrect-mark-parentheses-style
Use
@pytest.mark.foo
should be@pytest.mark.foo()
.I have set lint.flake8-pytest-style.mark-parentheses to
false
, which will be the default value in a future version of ruff anywayPT006 - pytest-parametrize-names-wrong-type
A consistency check of the type of parameter names passed to pytest.mark.parametrize, the choices are
list
,tuple
, andcsv
.Ruff's default value is
tuple
, but the large majority of pip's pytest arecsv
, so I have set lint.flake8-pytest-style.parametrize-names-type to that. Required manual fixing of 1 test as giving a single element csv and a single element tuple does not quite produce quite the same argument type.PT007 - pytest-parametrize-values-wrong-type
A consistency check for the type of parameter values passed to pytest.mark.parametrize.
Pip mostly uses
list
for the value type, so I set lint.flake8-pytest-style.parametrize-values-type to that.Pip mostly uses
tuple
for value row type, so I set lint.flake8-pytest-style.parametrize-values-row-type to that.PT001 - pytest-fixture-incorrect-parentheses-style
Use
@pytest.fixture
should be@pytest.fixture()
, this is a style choice.I have set lint.flake8-pytest-style.fixture-parentheses to false, which will be the default value in a future version of ruff anyway
PT018 - pytest-composite-assertion
That assertions should be broken out instead of composite with
and
statements, this certainly makes failures easier to debug.Fixes were not automatic when there was a comment, so this required additional manual fixes
PT004 - pytest-missing-fixture-name-underscore
A convention that fixtures that don't return anything should start with an
_
, required manually fixing.I have excluded this rule as it was not clear what the convention purpose is to me, and because of the dynamic nature of
pytest.mark.usefixtures
requires some tricky manual updating.I did however end up noticing
patch_distribution_lookups
,patch_locale
,scoped_global_tempdir_manager
, andisolate
fixtures never appear to be used, should I attempt removing them?PT011 - pytest-raises-too-broad
Rule expects a subset of exceptions to specify an error message that
pytest.raises
should always match on. For relatively narrow tests this seemed overly prescriptive to me, so I added PT011 to the ignore list, but let me know and I will add these in.PT015 - pytest-assert-always-false
Convention to use
pytest.fail(msg)
instead ofassert False, msg
, manually fixed.PT022 - pytest-useless-yield-fixture
A yield in a fixture so that fixture can have a teardown, with no teardown the yield is pointless, automatically fixed.
PT003 - pytest-extraneous-scope-function
A "function" scope in a fixture is default, automatically fixed.
PT017 - pytest-assert-in-except
Use
pytest.raises
not an asset inside an exception, manually fixed.PT013 - pytest-incorrect-pytest-import
Use
import pytest
notfrom pytest import ...
notimport pytest as pt
, manually fixedPT012 - pytest-raises-with-multiple-statements
Only put 1 statement inside a pytest raises, manually fixed.