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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg_resources/tests/test_resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ def test_spaces_between_multiple_versions(self):
(req,) = parse_requirements('foo >= 1.0, < 3')

@pytest.mark.parametrize(
['lower', 'upper'],
'lower, upper',
[
('1.2-rc1', '1.2rc1'),
('0.4', '0.4.0'),
Expand All @@ -724,7 +724,7 @@ def testVersionEquality(self, lower, upper):
"""

@pytest.mark.parametrize(
['lower', 'upper'],
'lower, upper',
[
('2.1', '2.1.1'),
('2a1', '2b0'),
Expand Down
4 changes: 2 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ check = [

# local

# workaround for businho/pytest-ruff#28
"ruff >= 0.5.2; sys_platform != 'cygwin'",
# changed defaults for PT001 and PT023 astral-sh/ruff#13292
"ruff >= 0.7.0; sys_platform != 'cygwin'",
]

cover = [
Expand Down
9 changes: 9 additions & 0 deletions ruff.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ extend-select = [
"I", # isort
"PERF", # Perflint
"PGH", # pygrep-hooks (blanket-* rules)
"PT", # flake8-pytest-style
"PYI", # flake8-pyi
"RUF10", # unused-noqa & redirected-noqa
"TRY", # tryceratops
Expand All @@ -28,6 +29,11 @@ extend-select = [
]
ignore = [
"PERF203", # try-except-in-loop, micro-optimisation with many false-positive. Worth checking but don't block CI
"PT004", # deprecated https://github.com/astral-sh/ruff/issues/8796#issuecomment-2057143531
"PT005", # deprecated https://github.com/astral-sh/ruff/issues/8796#issuecomment-2057143531
"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

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

"TRY003", # raise-vanilla-args, avoid multitude of exception classes
"TRY301", # raise-within-try, it's handy
"UP015", # redundant-open-modes, explicit is preferred
Expand Down Expand Up @@ -75,6 +81,9 @@ sections.delayed = ["distutils"]
[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)


[format]
# Enable preview to get hugged parenthesis unwrapping and other nice surprises
# See https://github.com/jaraco/skeleton/pull/133#issuecomment-2239538373
Expand Down
4 changes: 3 additions & 1 deletion setuptools/command/install_lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@ def copy_tree(
preserve_symlinks: bool = False, # type: ignore[override]
level: object = 1,
) -> list[str]:
assert preserve_mode and preserve_times and not preserve_symlinks
assert preserve_mode
assert preserve_times
assert not preserve_symlinks
exclude = self.get_exclusions()

if not exclude:
Expand Down
2 changes: 1 addition & 1 deletion setuptools/tests/config/test_apply_pyprojecttoml.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ def test_no_explicit_content_type_for_missing_extension(tmp_path):


@pytest.mark.parametrize(
('pyproject_text', 'expected_maintainers_meta_value'),
'pyproject_text, expected_maintainers_meta_value',
(
pytest.param(
PEP621_EXAMPLE,
Expand Down
19 changes: 9 additions & 10 deletions setuptools/tests/integration/test_pip_install_sdist.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,23 +104,22 @@ def venv_python(tmp_path):


@pytest.fixture(autouse=True)
def _prepare(tmp_path, venv_python, monkeypatch, request):
def _prepare(tmp_path, venv_python, monkeypatch):
download_path = os.getenv("DOWNLOAD_PATH", str(tmp_path))
os.makedirs(download_path, exist_ok=True)

# Environment vars used for building some of the packages
monkeypatch.setenv("USE_MYPYC", "1")

def _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"])
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"])
Comment on lines +114 to +122
Copy link
Contributor Author

Choose a reason for hiding this comment

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



@pytest.mark.parametrize('package, version', EXAMPLES)
Expand Down
10 changes: 7 additions & 3 deletions setuptools/tests/test_bdist_egg.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"""


@pytest.fixture(scope='function')
@pytest.fixture
def setup_context(tmpdir):
with (tmpdir / 'setup.py').open('w') as f:
f.write(SETUP_PY)
Expand All @@ -28,7 +28,9 @@ def setup_context(tmpdir):


class Test:
def test_bdist_egg(self, setup_context, user_override):
@pytest.mark.usefixtures("user_override")
@pytest.mark.usefixtures("setup_context")
def test_bdist_egg(self):
dist = Distribution(
dict(
script_name='setup.py',
Expand All @@ -50,7 +52,9 @@ def test_bdist_egg(self, setup_context, user_override):
os.environ.get('PYTHONDONTWRITEBYTECODE', False),
reason="Byte code disabled",
)
def test_exclude_source_files(self, setup_context, user_override):
@pytest.mark.usefixtures("user_override")
@pytest.mark.usefixtures("setup_context")
def test_exclude_source_files(self):
dist = Distribution(
dict(
script_name='setup.py',
Expand Down
15 changes: 8 additions & 7 deletions setuptools/tests/test_easy_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -407,14 +407,14 @@ def test_multiproc_atexit(self):
logging.basicConfig(level=logging.INFO, stream=sys.stderr)
log.info('this should not break')

@pytest.fixture()
@pytest.fixture
def foo_package(self, tmpdir):
egg_file = tmpdir / 'foo-1.0.egg-info'
with egg_file.open('w') as f:
f.write('Name: foo\n')
return str(tmpdir)

@pytest.fixture()
@pytest.fixture
def install_target(self, tmpdir):
target = str(tmpdir)
with mock.patch('sys.path', sys.path + [target]):
Expand Down Expand Up @@ -472,6 +472,12 @@ def distutils_package():
yield


@pytest.mark.usefixtures("distutils_package")
class TestDistutilsPackage:
def test_bdist_egg_available_on_distutils_pkg(self):
run_setup('setup.py', ['bdist_egg'])


@pytest.fixture
def mock_index():
# set up a server which will simulate an alternate package index.
Expand All @@ -484,11 +490,6 @@ def mock_index():
return p_index


class TestDistutilsPackage:
def test_bdist_egg_available_on_distutils_pkg(self, distutils_package):
run_setup('setup.py', ['bdist_egg'])


class TestInstallRequires:
def test_setup_install_includes_dependencies(self, tmp_path, mock_index):
"""
Expand Down
23 changes: 6 additions & 17 deletions setuptools/tests/test_packageindex.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import http.client
import re
import urllib.error
import urllib.request
from inspect import cleandoc
Expand All @@ -24,11 +25,8 @@ def test_regex(self):
def test_bad_url_bad_port(self):
index = setuptools.package_index.PackageIndex()
url = 'http://127.0.0.1:0/nonesuch/test_package_index'
try:
with pytest.raises(Exception, match=re.escape(url)):
v = index.open_url(url)
except Exception as exc:
assert url in str(exc)
else:
assert isinstance(v, urllib.error.HTTPError)

def test_bad_url_typo(self):
Expand All @@ -37,15 +35,10 @@ def test_bad_url_typo(self):
# in its home URL
index = setuptools.package_index.PackageIndex(hosts=('www.example.com',))

url = (
'url:%20https://svn.plone.org/svn'
'/collective/inquant.contentmirror.plone/trunk'
)
try:
url = 'url:%20https://svn.plone.org/svn/collective/inquant.contentmirror.plone/trunk'

with pytest.raises(Exception, match=re.escape(url)):
v = index.open_url(url)
except Exception as exc:
assert url in str(exc)
else:
assert isinstance(v, urllib.error.HTTPError)

def test_bad_url_bad_status_line(self):
Expand All @@ -56,12 +49,8 @@ def _urlopen(*args):

index.opener = _urlopen
url = 'http://example.com'
try:
with pytest.raises(Exception, match=r'line'):
index.open_url(url)
except Exception as exc:
assert 'line' in str(exc)
else:
raise AssertionError('Should have raise here!')

def test_bad_url_double_scheme(self):
"""
Expand Down
5 changes: 3 additions & 2 deletions setuptools/tests/test_setuptools.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.



def makeSetup(**args):
Expand Down Expand Up @@ -257,7 +257,8 @@ def can_symlink(tmpdir):
os.remove(link_fn)


def test_findall_missing_symlink(tmpdir, can_symlink):
@pytest.mark.usefixtures("can_symlink")
def test_findall_missing_symlink(tmpdir):
with tmpdir.as_cwd():
os.symlink('foo', 'bar')
found = list(setuptools.findall())
Expand Down
2 changes: 1 addition & 1 deletion setuptools/tests/test_wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@


@pytest.mark.parametrize(
('filename', 'info'), WHEEL_INFO_TESTS, ids=[t[0] for t in WHEEL_INFO_TESTS]
'filename, info', WHEEL_INFO_TESTS, ids=[t[0] for t in WHEEL_INFO_TESTS]
)
def test_wheel_info(filename, info):
if inspect.isclass(info):
Expand Down
Loading