Conditionally ignore collection of setuptools dirnames #12918

sus-pe · 2024-10-25T12:52:55Z

testing/test_collection.py

src/_pytest/main.py

sus-pe · 2024-10-25T13:08:10Z

src/_pytest/main.py

@@ -423,6 +430,10 @@ def pytest_ignore_collect(collection_path: Path, config: Config) -> bool | None:
        if any(fnmatch_ex(pat, collection_path) for pat in norecursepatterns):
            return True

+        if any(fnmatch_ex(pat, collection_path) for pat in ("build", "dist")):


should this be wrapped by a "allow_in_setuptools" like they do for a venv?

I didn't understand this reference at first. But now I realize that this is what I've been thinking of when I wrote another comment @ #12918 (comment). So I'd say yes. This would improve the maintainability.

i have rewritten this in light of what I understood from our conversation, please let me know if this is the direction we want to go in

nicoddemus · 2024-10-25T17:41:22Z

src/_pytest/main.py

-    indicators = ("setup.py", "setup.cfg", "pyproject.toml")
-    return any((path / f).exists() for f in indicators)
+    indicators = ("setup.py", "setup.cfg")
+    if any((path / f).exists() for f in indicators):


Suggested change

if any((path / f).exists() for f in indicators):

if any((path / f).is_file() for f in indicators):

webknjaz · 2024-10-26T03:26:22Z

changelog/12625.improvement.rst

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


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.

webknjaz · 2024-10-26T03:28:19Z

src/_pytest/main.py

+        except tomllib.TOMLDecodeError as exc:
+            raise UsageError(f"{toml_file}: {exc}") from exc
+
+        result = config.get("build-system", {}).get("requires", {})


I recommend using a more verbose variable name, one that is not generic but corresponds to the data being assigned to it. This is because we should optimize for readability, since any code is read much more often than it's written.

Noted, for now this code has been removed

webknjaz · 2024-10-26T03:32:16Z

src/_pytest/main.py

@@ -423,6 +450,10 @@ def pytest_ignore_collect(collection_path: Path, config: Config) -> bool | None:
        if any(fnmatch_ex(pat, collection_path) for pat in norecursepatterns):
            return True

+        if any(fnmatch_ex(pat, collection_path) for pat in ("build", "dist")):
+            if _is_setuptools_project(collection_path.parent):


This case does not need a nested block. Instead, both conditions could be merged using and. The line would be quite long, though, so it might be a good idea to assign that to a variable that would also be an opportunity to assign semantics to this check through naming the var with something that corresponds to the high-level purpose of the check.

For example, using is_distributable_library would make that check if is_distributable_library:, which is both short and communicates what the list of low-level checks mean.

tried an allow_in_build approach, let me know what you think

webknjaz · 2024-10-26T17:04:53Z

src/_pytest/main.py

@@ -63,9 +63,7 @@ def pytest_addoption(parser: Parser) -> None:
            "*.egg",
            ".*",
            "_darcs",
-            "build",


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

webknjaz · 2024-10-26T17:06:08Z

src/_pytest/main.py

@@ -367,6 +365,22 @@ def pytest_runtestloop(session: Session) -> bool:
    return True


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


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

webknjaz · 2024-10-26T17:08:47Z

src/_pytest/main.py

+        return False
+
+    if any(fnmatch_ex(pat, path) for pat in ("build", "dist")):
+        indicators = ("setup.py", "setup.cfg", "pyproject.toml")


It is important of understand that setup.cfg can be an indicator only in case pyproject.toml with setuptools pointers exists, or setup.py is present.

So the check would need to be more involved.

I'd like to verify my understanding of the logic required. I've re-written it and the tests to reflect how I currently understood the discussion here and in #12625, basically, we first need to see setup.cfg, and then only if setup.py or pyproject.toml with appropriate setuptools pointers exist then we'll ignore collecting build and dist

webknjaz · 2024-10-26T17:09:46Z

src/_pytest/main.py

+
+    if any(fnmatch_ex(pat, path) for pat in ("build", "dist")):
+        indicators = ("setup.py", "setup.cfg", "pyproject.toml")
+        if any((path.parent / f).is_file() for f in indicators):


It feels like this is cheaper than fnmatch. Could you try adding a “guard expression” and bailing early if none of these exist?

See pytest-dev#12625 for context

nicoddemus · 2024-11-16T18:48:42Z

src/_pytest/main.py

+        build_system = parsed_toml.get("build-system", {}).get("requires")
+        if "setuptools" in build_system:
+            return True
+    except Exception:


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.

nicoddemus · 2024-11-16T18:49:42Z

src/_pytest/main.py

@@ -418,6 +457,10 @@ def pytest_ignore_collect(collection_path: Path, config: Config) -> bool | None:
    if not allow_in_venv and _in_venv(collection_path):
        return True

+    allow_in_build = False  # config.getoption("collect_in_build")


nicoddemus

Just so everyone is on the same page, marking the PR as "request changes" to make it clear there are a number of comments that need addressing.

Thanks again @sus-pe for the contribution. 👍

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Oct 25, 2024

sus-pe force-pushed the bugfix-12625-ignorebuild branch from 4f259e0 to 2ecca4e Compare October 25, 2024 13:01

sus-pe commented Oct 25, 2024

View reviewed changes

nicoddemus reviewed Oct 25, 2024

View reviewed changes

webknjaz reviewed Oct 26, 2024

View reviewed changes

sus-pe force-pushed the bugfix-12625-ignorebuild branch 2 times, most recently from 4e4ef2e to 71fc2ad Compare October 26, 2024 11:22

webknjaz reviewed Oct 26, 2024

View reviewed changes

sus-pe force-pushed the bugfix-12625-ignorebuild branch from 71fc2ad to 232d614 Compare October 28, 2024 17:40

root and others added 3 commits November 10, 2024 14:33

Recurse into build / dist directories

f5edd08

See pytest-dev#12625 for context

Add test for setup.cfg indicator only if setup.py

8d54d89

fix comment

c8f095b

sus-pe force-pushed the bugfix-12625-ignorebuild branch from 6d9cb54 to c8f095b Compare November 10, 2024 12:33

simplify

8875c82

nicoddemus reviewed Nov 16, 2024

View reviewed changes

Tusenka approved these changes Dec 6, 2024

View reviewed changes

nicoddemus requested changes Dec 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditionally ignore collection of setuptools dirnames #12918

Conditionally ignore collection of setuptools dirnames #12918

sus-pe commented Oct 25, 2024

sus-pe Oct 25, 2024

webknjaz Oct 26, 2024

sus-pe Oct 26, 2024

nicoddemus Oct 25, 2024

sus-pe Oct 26, 2024

webknjaz Oct 26, 2024

webknjaz Oct 26, 2024

sus-pe Oct 26, 2024

webknjaz Oct 26, 2024

sus-pe Oct 26, 2024

webknjaz Oct 26, 2024

webknjaz Oct 26, 2024

webknjaz Oct 26, 2024

sus-pe Nov 10, 2024

webknjaz Oct 26, 2024

nicoddemus Nov 16, 2024

nicoddemus Nov 16, 2024

nicoddemus left a comment

	if any((path / f).exists() for f in indicators):
	if any((path / f).is_file() for f in indicators):

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

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

Conditionally ignore collection of setuptools dirnames #12918

Are you sure you want to change the base?

Conditionally ignore collection of setuptools dirnames #12918

Conversation

sus-pe commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment