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

Conditionally ignore collection of setuptools dirnames #12918

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
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
2 changes: 2 additions & 0 deletions changelog/12625.improvement.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Conditionally ignore collection of setuptools artifacts dirnames only if the
directories reside inside a setuptools project, i.e. `setup.cfg`, is present, etc.
Copy link
Member

Choose a reason for hiding this comment

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

Please, enumerate all the files taken into consideration in this logic. Change notes are targeting the end-users who would not be able to guess. It might also be a good idea to mention the same somewhere in the regular docs, additionally.

47 changes: 45 additions & 2 deletions src/_pytest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,7 @@
"*.egg",
".*",
"_darcs",
"build",
Copy link
Member

Choose a reason for hiding this comment

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

I think that the description string above should mention that these are still conditionally excluded by default.

"CVS",
"dist",
"node_modules",
"venv",
"{arch}",
Expand Down Expand Up @@ -367,6 +365,47 @@
return True


def _is_setuptools_in_pyproject_toml(toml: Path) -> bool:
"""Attempt to decode a toml file into a dict, returning None if fails."""
if sys.version_info >= (3, 11):
import tomllib

Check warning on line 371 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L371

Added line #L371 was not covered by tests
else:
import tomli as tomllib

try:
toml_text = toml.read_text(encoding="utf-8")
parsed_toml = tomllib.loads(toml_text)
build_system = parsed_toml.get("build-system", {}).get("requires")
if "setuptools" in build_system:
return True
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to avoid using a except Exception here unless absolute necessary -- from what I see on the code inside the try/except block, seems that we are taking in account missing keys already.

Did you account that tomlib.loads might fail due to a malformed TOML file? If so let's catch the precise exception here instead.

pass

Check warning on line 382 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L381-L382

Added lines #L381 - L382 were not covered by tests

return False

Check warning on line 384 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L384

Added line #L384 was not covered by tests


def _in_build(path: Path) -> bool:
"""Attempt to detect if ``path`` is the root of a buildsystem's artifacts
Copy link
Member

Choose a reason for hiding this comment

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

It feels like this isn't just an attempt but a complete piece of logic. So I'd just go with “Detect” since it seems more accurate.

Suggested change
"""Attempt to detect if ``path`` is the root of a buildsystem's artifacts
"""Detect if ``path`` is the root of a buildsystem's artifacts

by checking known dirnames patterns, and the presence of configuration in
the parent dir by checking for a setup.py, setup.cfg, or pyproject.toml.
"""
if not path.is_dir():
return False

if any(fnmatch_ex(pat, path) for pat in ("build", "dist")):
setup_cfg = path.parent / "setup.cfg"
if (setup_cfg).is_file():
setup_py = path.parent / "setup.py"
if setup_py.is_file():
return True

toml = path.parent / "pyproject.toml"
if toml.is_file() and _is_setuptools_in_pyproject_toml(toml):
return True

return False


def _in_venv(path: Path) -> bool:
"""Attempt to detect if ``path`` is the root of a Virtual Environment by
checking for the existence of the pyvenv.cfg file.
Expand Down Expand Up @@ -418,6 +457,10 @@
if not allow_in_venv and _in_venv(collection_path):
return True

allow_in_build = False # config.getoption("collect_in_build")
Copy link
Member

Choose a reason for hiding this comment

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

Left over?

if not allow_in_build and _in_build(collection_path):
return True

if collection_path.is_dir():
norecursepatterns = config.getini("norecursedirs")
if any(fnmatch_ex(pat, collection_path) for pat in norecursepatterns):
Expand Down
49 changes: 47 additions & 2 deletions testing/test_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,6 @@ def test_foo():
class TestCollectFS:
def test_ignored_certain_directories(self, pytester: Pytester) -> None:
sus-pe marked this conversation as resolved.
Show resolved Hide resolved
sus-pe marked this conversation as resolved.
Show resolved Hide resolved
tmp_path = pytester.path
ensure_file(tmp_path / "build" / "test_notfound.py")
ensure_file(tmp_path / "dist" / "test_notfound.py")
ensure_file(tmp_path / "_darcs" / "test_notfound.py")
ensure_file(tmp_path / "CVS" / "test_notfound.py")
ensure_file(tmp_path / "{arch}" / "test_notfound.py")
Expand Down Expand Up @@ -276,6 +274,53 @@ def test_missing_permissions_on_unselected_directory_doesnt_crash(
assert result.ret == ExitCode.OK
result.assert_outcomes(passed=1)

known_build_dirs = pytest.mark.parametrize("build_dir", ["build", "dist"])

@known_build_dirs
def test_build_dirs_collected_when_setuptools_setup_py_present(
self, pytester: Pytester, build_dir: str
) -> None:
tmp_path = pytester.path
ensure_file(tmp_path / build_dir / "test_module.py").write_text(
"def test_hello(): pass", encoding="utf-8"
)

result = pytester.runpytest("--collect-only").stdout.str()
assert "test_module" in result

ensure_file(tmp_path / "setup.cfg")

result = pytester.runpytest("--collect-only").stdout.str()
assert "test_module" in result

ensure_file(tmp_path / "setup.py")
result = pytester.runpytest("--collect-only").stdout.str()
assert "test_module" not in result

@known_build_dirs
def test_build_dirs_collected_when_setuptools_present_in_pyproject_toml(
self, pytester: Pytester, build_dir: str
) -> None:
tmp_path = pytester.path
ensure_file(tmp_path / build_dir / "test_module.py").write_text(
"def test_hello(): pass", encoding="utf-8"
)

result = pytester.runpytest("--collect-only").stdout.str()
assert "test_module" in result

ensure_file(tmp_path / "setup.cfg")

result = pytester.runpytest("--collect-only").stdout.str()
assert "test_module" in result

ensure_file(tmp_path / "pyproject.toml").write_text(
'[build-system]\nrequires = ["setuptools", "setuptools-scm"]\n',
encoding="utf-8",
)
result = pytester.runpytest("--collect-only").stdout.str()
assert "test_module" not in result


class TestCollectPluginHookRelay:
def test_pytest_collect_file(self, pytester: Pytester) -> None:
Expand Down
Loading