Skip to content

Add pytest ruff checks to test directory#12894

Merged
uranusjr merged 16 commits intopypa:mainfrom
notatallshaw:add-pytest-ruff-checks
Aug 7, 2024
Merged

Add pytest ruff checks to test directory#12894
uranusjr merged 16 commits intopypa:mainfrom
notatallshaw:add-pytest-ruff-checks

Conversation

@notatallshaw
Copy link
Copy Markdown
Member

@notatallshaw notatallshaw commented Aug 4, 2024

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 main pyproject.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:

$ ruff check . --select PT --statistics
156 	PT023   [*] pytest-incorrect-mark-parentheses-style
146 	PT006   [*] pytest-parametrize-names-wrong-type
 51 	PT007   [*] pytest-parametrize-values-wrong-type
 35 	PT001   [*] pytest-fixture-incorrect-parentheses-style
 16 	PT018   [ ] pytest-composite-assertion
 11 	PT004   [ ] pytest-missing-fixture-name-underscore
  9 	PT011   [ ] pytest-raises-too-broad
  8 	PT015   [ ] pytest-assert-always-false
  7 	PT022   [*] pytest-useless-yield-fixture
  5 	PT003   [*] pytest-extraneous-scope-function
  2 	PT013   [ ] pytest-incorrect-pytest-import
  2 	PT017   [ ] pytest-assert-in-except
  1 	PT012   [ ] pytest-raises-with-multiple-statements

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 anyway

PT006 - pytest-parametrize-names-wrong-type

A consistency check of the type of parameter names passed to pytest.mark.parametrize, the choices are list, tuple, and csv.

Ruff's default value is tuple, but the large majority of pip's pytest are csv, 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, and isolate 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 of assert 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 not from pytest import ... not import pytest as pt, manually fixed

PT012 - pytest-raises-with-multiple-statements

Only put 1 statement inside a pytest raises, manually fixed.

@notatallshaw notatallshaw force-pushed the add-pytest-ruff-checks branch 4 times, most recently from c47b8bb to 0c6356a Compare August 4, 2024 23:09
Comment thread news/12894.trivial.rst Outdated
Comment thread tests/functional/test_fast_deps.py Outdated
Comment thread tests/functional/test_install_reqs.py Outdated
Comment thread tests/unit/test_self_check_outdated.py Outdated
@notatallshaw notatallshaw force-pushed the add-pytest-ruff-checks branch from d9561e4 to 05d79d8 Compare August 6, 2024 23:14
@uranusjr uranusjr merged commit e98cc5c into pypa:main Aug 7, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants