Add pytest ruff checks to test directory#12894
Merged
Conversation
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
06da6ec to
d9561e4
Compare
uranusjr
reviewed
Aug 6, 2024
uranusjr
approved these changes
Aug 6, 2024
d9561e4 to
05d79d8
Compare
uranusjr
approved these changes
Aug 7, 2024
This file contains hidden or 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
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.tomlfor 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.fooshould 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
listfor the value type, so I set lint.flake8-pytest-style.parametrize-values-type to that.Pip mostly uses
tuplefor value row type, so I set lint.flake8-pytest-style.parametrize-values-row-type to that.PT001 - pytest-fixture-incorrect-parentheses-style
Use
@pytest.fixtureshould 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
andstatements, 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.usefixturesrequires some tricky manual updating.I did however end up noticing
patch_distribution_lookups,patch_locale,scoped_global_tempdir_manager, andisolatefixtures 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.raisesshould 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.raisesnot an asset inside an exception, manually fixed.PT013 - pytest-incorrect-pytest-import
Use
import pytestnotfrom pytest import ...notimport pytest as pt, manually fixedPT012 - pytest-raises-with-multiple-statements
Only put 1 statement inside a pytest raises, manually fixed.