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.

expanded the changelog, will add some to the regular doce soon

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?

okay, i've promoted a check of setup.cfg as a guard, i've kept the others inside for readability

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.

fixed to use the specific exception

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

See pytest-dev#12625 for context

nicoddemus

Just realized a problem: other build systems also use build and dist directories, for example, poetry uses dist, pyinstaller uses both build and dist -- I assume others might use those directories too (both or one or the other).

If we merge this, then build and dist in projects that use those build systems will start collecting from build and dist, which is of course a problem.

Looking at the #12625 original request, it only asks to avoid collecting build/ and dist/ from the root directory, but still collect from build/ and dist/ found in sub directories.

We might want to rethink this and only address the original concern, instead of trying to be smart here but then having to support/handle many different build systems and account for the directories they decide to use.

nicoddemus · 2025-02-01T11:50:30Z

doc/en/reference/reference.rst

@@ -1776,6 +1776,9 @@ passed multiple times. The expected format is ``name=value``. For example::
   *must* override ``norecursedirs`` in addition to using the
   ``--collect-in-virtualenv`` flag.

+   Similarly, pytest will attempt to intelligently identify and igmore build
+   artifacts of a setuptools project unless ``--collect-in-build`` is used.


Suggested change

artifacts of a setuptools project unless ``--collect-in-build`` is used.

artifacts of a ``setuptools`` project unless ``--collect-in-build`` is used.

nicoddemus · 2025-02-01T11:51:02Z

changelog/12625.improvement.rst

@@ -0,0 +1,4 @@
+Conditionally ignore collection of setuptools artifacts dirnames only if the


Suggested change

Conditionally ignore collection of setuptools artifacts dirnames only if the

Conditionally ignore collection of ``setuptools`` artifacts directory names (``build`` and ``dist``) only if the

nicoddemus · 2025-02-01T11:51:30Z

doc/en/reference/reference.rst

@@ -2163,6 +2166,8 @@ All the command-line flags can be obtained by running ``pytest --help``::
      --keep-duplicates     Keep duplicate tests
      --collect-in-virtualenv
                            Don't ignore tests in a local virtualenv directory
+      --collect-in-build
+                            Don't ignore tests in a local build directory


Suggested change

Don't ignore tests in a local build directory

Don't ignore tests in a local build directories (build/ and dist/)

nicoddemus · 2025-02-01T11:52:07Z

src/_pytest/main.py

@@ -371,6 +376,50 @@ def pytest_runtestloop(session: Session) -> bool:
    return True


+def _is_setuptools_in_pyproject_toml(toml: Path) -> bool:
+    """Attempt to decode a toml file into a dict, returning None if fails."""


Suggested change

"""Attempt to decode a toml file into a dict, returning None if fails."""

"""Checks if the given pyproject.toml configures setuptools as the build-system."""

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

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

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

root and others added 11 commits February 1, 2025 10:44

Recurse into build / dist directories

ad1b157

See pytest-dev#12625 for context

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

b45dc40

fix comment

c296f37

simplify

db9be29

Add description that norecursedirs ignores build

9f312aa

Fix docstring

22e4e95

Use specific toml decode error

961bb42

Bail if no setup.cfg present

44d31b6

Add collect-in-build option

77c630e

Fix lint in tests

9c61dd3

Add changelog

22d8879

sus-pe added 2 commits February 1, 2025 10:46

Add docs

d89d30f

fix

8dee4e9

sus-pe force-pushed the bugfix-12625-ignorebuild branch from 9cf63a9 to 8dee4e9 Compare February 1, 2025 08:51

sus-pe marked this pull request as ready for review February 1, 2025 08:53

nicoddemus requested changes Feb 1, 2025

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

sus-pe Feb 1, 2025

sus-pe Feb 1, 2025

webknjaz Oct 26, 2024

sus-pe Oct 26, 2024

webknjaz Oct 26, 2024

sus-pe Oct 26, 2024

webknjaz Oct 26, 2024

sus-pe Feb 1, 2025

webknjaz Oct 26, 2024

sus-pe Feb 1, 2025

webknjaz Oct 26, 2024

sus-pe Nov 10, 2024

webknjaz Oct 26, 2024

sus-pe Feb 1, 2025

nicoddemus Nov 16, 2024

sus-pe Feb 1, 2025

nicoddemus Nov 16, 2024

sus-pe Feb 1, 2025

nicoddemus left a comment

nicoddemus left a comment

nicoddemus Feb 1, 2025

nicoddemus Feb 1, 2025

nicoddemus Feb 1, 2025

nicoddemus Feb 1, 2025

	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

	artifacts of a setuptools project unless ``--collect-in-build`` is used.
	artifacts of a ``setuptools`` project unless ``--collect-in-build`` is used.

		@@ -0,0 +1,4 @@
		Conditionally ignore collection of setuptools artifacts dirnames only if the

	Conditionally ignore collection of setuptools artifacts dirnames only if the
	Conditionally ignore collection of ``setuptools`` artifacts directory names (``build`` and ``dist``) only if the

	Don't ignore tests in a local build directory
	Don't ignore tests in a local build directories (build/ and dist/)

	"""Attempt to decode a toml file into a dict, returning None if fails."""
	"""Checks if the given pyproject.toml configures setuptools as the build-system."""

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

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

nicoddemus left a 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