Merge pull request #12043 from bluetech/pyargs-root

Fix collection failures due to permission errors when using `--pyargs`

Author: bluetech

Diff for: changelog/11904.bugfix.rst

@@ -0,0 +1,3 @@
+Fixed a regression in pytest 8.0.0 that would cause test collection to fail due to permission errors when using ``--pyargs``.
+
+This change improves the collection tree for tests specified using ``--pyargs``, see :pull:`12043` for a comparison with pytest 8.0 and <8.

Diff for: src/_pytest/main.py

@@ -5,6 +5,7 @@
 import fnmatch
 import functools
 import importlib
+import importlib.util
 import os
 from pathlib import Path
 import sys
@@ -563,7 +564,7 @@ def __init__(self, config: Config) -> None:
         self._initialpaths: FrozenSet[Path] = frozenset()
         self._initialpaths_with_parents: FrozenSet[Path] = frozenset()
         self._notfound: List[Tuple[str, Sequence[nodes.Collector]]] = []
-        self._initial_parts: List[Tuple[Path, List[str]]] = []
+        self._initial_parts: List[CollectionArgument] = []
         self._collection_cache: Dict[nodes.Collector, CollectReport] = {}
         self.items: List[nodes.Item] = []
 
@@ -769,15 +770,15 @@ def perform_collect(
             initialpaths: List[Path] = []
             initialpaths_with_parents: List[Path] = []
             for arg in args:
-                fspath, parts = resolve_collection_argument(
+                collection_argument = resolve_collection_argument(
                     self.config.invocation_params.dir,
                     arg,
                     as_pypath=self.config.option.pyargs,
                 )
-                self._initial_parts.append((fspath, parts))
-                initialpaths.append(fspath)
-                initialpaths_with_parents.append(fspath)
-                initialpaths_with_parents.extend(fspath.parents)
+                self._initial_parts.append(collection_argument)
+                initialpaths.append(collection_argument.path)
+                initialpaths_with_parents.append(collection_argument.path)
+                initialpaths_with_parents.extend(collection_argument.path.parents)
             self._initialpaths = frozenset(initialpaths)
             self._initialpaths_with_parents = frozenset(initialpaths_with_parents)
 
@@ -839,29 +840,43 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]:
 
         pm = self.config.pluginmanager
 
-        for argpath, names in self._initial_parts:
-            self.trace("processing argument", (argpath, names))
+        for collection_argument in self._initial_parts:
+            self.trace("processing argument", collection_argument)
             self.trace.root.indent += 1
 
+            argpath = collection_argument.path
+            names = collection_argument.parts
+            module_name = collection_argument.module_name
+
             # resolve_collection_argument() ensures this.
             if argpath.is_dir():
                 assert not names, f"invalid arg {(argpath, names)!r}"
 
-            # Match the argpath from the root, e.g.
+            paths = [argpath]
+            # Add relevant parents of the path, from the root, e.g.
             #   /a/b/c.py -> [/, /a, /a/b, /a/b/c.py]
-            paths = [*reversed(argpath.parents), argpath]
-            # Paths outside of the confcutdir should not be considered, unless
-            # it's the argpath itself.
-            while len(paths) > 1 and not pm._is_in_confcutdir(paths[0]):
-                paths = paths[1:]
+            if module_name is None:
+                # Paths outside of the confcutdir should not be considered.
+                for path in argpath.parents:
+                    if not pm._is_in_confcutdir(path):
+                        break
+                    paths.insert(0, path)
+            else:
+                # For --pyargs arguments, only consider paths matching the module
+                # name. Paths beyond the package hierarchy are not included.
+                module_name_parts = module_name.split(".")
+                for i, path in enumerate(argpath.parents, 2):
+                    if i > len(module_name_parts) or path.stem != module_name_parts[-i]:
+                        break
+                    paths.insert(0, path)
 
             # Start going over the parts from the root, collecting each level
             # and discarding all nodes which don't match the level's part.
             any_matched_in_initial_part = False
             notfound_collectors = []
             work: List[
                 Tuple[Union[nodes.Collector, nodes.Item], List[Union[Path, str]]]
-            ] = [(self, paths + names)]
+            ] = [(self, [*paths, *names])]
             while work:
                 matchnode, matchparts = work.pop()
 
@@ -953,44 +968,64 @@ def genitems(
                 node.ihook.pytest_collectreport(report=rep)
 
 
-def search_pypath(module_name: str) -> str:
-    """Search sys.path for the given a dotted module name, and return its file system path."""
+def search_pypath(module_name: str) -> Optional[str]:
+    """Search sys.path for the given a dotted module name, and return its file
+    system path if found."""
     try:
         spec = importlib.util.find_spec(module_name)
     # AttributeError: looks like package module, but actually filename
     # ImportError: module does not exist
     # ValueError: not a module name
     except (AttributeError, ImportError, ValueError):
-        return module_name
+        return None
     if spec is None or spec.origin is None or spec.origin == "namespace":
-        return module_name
+        return None
     elif spec.submodule_search_locations:
         return os.path.dirname(spec.origin)
     else:
         return spec.origin
 
 
+@dataclasses.dataclass(frozen=True)
+class CollectionArgument:
+    """A resolved collection argument."""
+
+    path: Path
+    parts: Sequence[str]
+    module_name: Optional[str]
+
+
 def resolve_collection_argument(
     invocation_path: Path, arg: str, *, as_pypath: bool = False
-) -> Tuple[Path, List[str]]:
+) -> CollectionArgument:
     """Parse path arguments optionally containing selection parts and return (fspath, names).
 
     Command-line arguments can point to files and/or directories, and optionally contain
     parts for specific tests selection, for example:
 
         "pkg/tests/test_foo.py::TestClass::test_foo"
 
-    This function ensures the path exists, and returns a tuple:
+    This function ensures the path exists, and returns a resolved `CollectionArgument`:
 
-        (Path("/full/path/to/pkg/tests/test_foo.py"), ["TestClass", "test_foo"])
+        CollectionArgument(
+            path=Path("/full/path/to/pkg/tests/test_foo.py"),
+            parts=["TestClass", "test_foo"],
+            module_name=None,
+        )
 
     When as_pypath is True, expects that the command-line argument actually contains
     module paths instead of file-system paths:
 
         "pkg.tests.test_foo::TestClass::test_foo"
 
     In which case we search sys.path for a matching module, and then return the *path* to the
-    found module.
+    found module, which may look like this:
+
+        CollectionArgument(
+            path=Path("/home/u/myvenv/lib/site-packages/pkg/tests/test_foo.py"),
+            parts=["TestClass", "test_foo"],
+            module_name="pkg.tests.test_foo",
+        )
 
     If the path doesn't exist, raise UsageError.
     If the path is a directory and selection parts are present, raise UsageError.
@@ -999,8 +1034,12 @@ def resolve_collection_argument(
     strpath, *parts = base.split("::")
     if parts:
         parts[-1] = f"{parts[-1]}{squacket}{rest}"
+    module_name = None
     if as_pypath:
-        strpath = search_pypath(strpath)
+        pyarg_strpath = search_pypath(strpath)
+        if pyarg_strpath is not None:
+            module_name = strpath
+            strpath = pyarg_strpath
     fspath = invocation_path / strpath
     fspath = absolutepath(fspath)
     if not safe_exists(fspath):
@@ -1017,4 +1056,8 @@ def resolve_collection_argument(
             else "directory argument cannot contain :: selection parts: {arg}"
         )
         raise UsageError(msg.format(arg=arg))
-    return fspath, parts
+    return CollectionArgument(
+        path=fspath,
+        parts=parts,
+        module_name=module_name,
+    )

Diff for: testing/test_collection.py

@@ -1787,3 +1787,48 @@ def test_collect_short_file_windows(pytester: Pytester) -> None:
     test_file.write_text("def test(): pass", encoding="UTF-8")
     result = pytester.runpytest(short_path)
     assert result.parseoutcomes() == {"passed": 1}
+
+
+def test_pyargs_collection_tree(pytester: Pytester, monkeypatch: MonkeyPatch) -> None:
+    """When using `--pyargs`, the collection tree of a pyargs collection
+    argument should only include parents in the import path, not up to confcutdir.
+
+    Regression test for #11904.
+    """
+    site_packages = pytester.path / "venv/lib/site-packages"
+    site_packages.mkdir(parents=True)
+    monkeypatch.syspath_prepend(site_packages)
+    pytester.makepyfile(
+        **{
+            "venv/lib/site-packages/pkg/__init__.py": "",
+            "venv/lib/site-packages/pkg/sub/__init__.py": "",
+            "venv/lib/site-packages/pkg/sub/test_it.py": "def test(): pass",
+        }
+    )
+
+    result = pytester.runpytest("--pyargs", "--collect-only", "pkg.sub.test_it")
+    assert result.ret == ExitCode.OK
+    result.stdout.fnmatch_lines(
+        [
+            "<Package venv/lib/site-packages/pkg>",
+            "  <Package sub>",
+            "    <Module test_it.py>",
+            "      <Function test>",
+        ],
+        consecutive=True,
+    )
+
+    # Now with an unrelated rootdir with unrelated files.
+    monkeypatch.chdir(tempfile.gettempdir())
+
+    result = pytester.runpytest("--pyargs", "--collect-only", "pkg.sub.test_it")
+    assert result.ret == ExitCode.OK
+    result.stdout.fnmatch_lines(
+        [
+            "<Package *pkg>",
+            "  <Package sub>",
+            "    <Module test_it.py>",
+            "      <Function test>",
+        ],
+        consecutive=True,
+    )

Diff for: testing/test_main.py

@@ -8,6 +8,7 @@
 
 from _pytest.config import ExitCode
 from _pytest.config import UsageError
+from _pytest.main import CollectionArgument
 from _pytest.main import resolve_collection_argument
 from _pytest.main import validate_basetemp
 from _pytest.pytester import Pytester
@@ -133,26 +134,43 @@ def invocation_path(self, pytester: Pytester) -> Path:
 
     def test_file(self, invocation_path: Path) -> None:
         """File and parts."""
-        assert resolve_collection_argument(invocation_path, "src/pkg/test.py") == (
-            invocation_path / "src/pkg/test.py",
-            [],
+        assert resolve_collection_argument(
+            invocation_path, "src/pkg/test.py"
+        ) == CollectionArgument(
+            path=invocation_path / "src/pkg/test.py",
+            parts=[],
+            module_name=None,
         )
-        assert resolve_collection_argument(invocation_path, "src/pkg/test.py::") == (
-            invocation_path / "src/pkg/test.py",
-            [""],
+        assert resolve_collection_argument(
+            invocation_path, "src/pkg/test.py::"
+        ) == CollectionArgument(
+            path=invocation_path / "src/pkg/test.py",
+            parts=[""],
+            module_name=None,
         )
         assert resolve_collection_argument(
             invocation_path, "src/pkg/test.py::foo::bar"
-        ) == (invocation_path / "src/pkg/test.py", ["foo", "bar"])
+        ) == CollectionArgument(
+            path=invocation_path / "src/pkg/test.py",
+            parts=["foo", "bar"],
+            module_name=None,
+        )
         assert resolve_collection_argument(
             invocation_path, "src/pkg/test.py::foo::bar::"
-        ) == (invocation_path / "src/pkg/test.py", ["foo", "bar", ""])
+        ) == CollectionArgument(
+            path=invocation_path / "src/pkg/test.py",
+            parts=["foo", "bar", ""],
+            module_name=None,
+        )
 
     def test_dir(self, invocation_path: Path) -> None:
         """Directory and parts."""
-        assert resolve_collection_argument(invocation_path, "src/pkg") == (
-            invocation_path / "src/pkg",
-            [],
+        assert resolve_collection_argument(
+            invocation_path, "src/pkg"
+        ) == CollectionArgument(
+            path=invocation_path / "src/pkg",
+            parts=[],
+            module_name=None,
         )
 
         with pytest.raises(
@@ -169,13 +187,24 @@ def test_pypath(self, invocation_path: Path) -> None:
         """Dotted name and parts."""
         assert resolve_collection_argument(
             invocation_path, "pkg.test", as_pypath=True
-        ) == (invocation_path / "src/pkg/test.py", [])
+        ) == CollectionArgument(
+            path=invocation_path / "src/pkg/test.py",
+            parts=[],
+            module_name="pkg.test",
+        )
         assert resolve_collection_argument(
             invocation_path, "pkg.test::foo::bar", as_pypath=True
-        ) == (invocation_path / "src/pkg/test.py", ["foo", "bar"])
-        assert resolve_collection_argument(invocation_path, "pkg", as_pypath=True) == (
-            invocation_path / "src/pkg",
-            [],
+        ) == CollectionArgument(
+            path=invocation_path / "src/pkg/test.py",
+            parts=["foo", "bar"],
+            module_name="pkg.test",
+        )
+        assert resolve_collection_argument(
+            invocation_path, "pkg", as_pypath=True
+        ) == CollectionArgument(
+            path=invocation_path / "src/pkg",
+            parts=[],
+            module_name="pkg",
         )
 
         with pytest.raises(
@@ -186,10 +215,13 @@ def test_pypath(self, invocation_path: Path) -> None:
             )
 
     def test_parametrized_name_with_colons(self, invocation_path: Path) -> None:
-        ret = resolve_collection_argument(
+        assert resolve_collection_argument(
             invocation_path, "src/pkg/test.py::test[a::b]"
+        ) == CollectionArgument(
+            path=invocation_path / "src/pkg/test.py",
+            parts=["test[a::b]"],
+            module_name=None,
         )
-        assert ret == (invocation_path / "src/pkg/test.py", ["test[a::b]"])
 
     def test_does_not_exist(self, invocation_path: Path) -> None:
         """Given a file/module that does not exist raises UsageError."""
@@ -209,17 +241,24 @@ def test_does_not_exist(self, invocation_path: Path) -> None:
     def test_absolute_paths_are_resolved_correctly(self, invocation_path: Path) -> None:
         """Absolute paths resolve back to absolute paths."""
         full_path = str(invocation_path / "src")
-        assert resolve_collection_argument(invocation_path, full_path) == (
-            Path(os.path.abspath("src")),
-            [],
+        assert resolve_collection_argument(
+            invocation_path, full_path
+        ) == CollectionArgument(
+            path=Path(os.path.abspath("src")),
+            parts=[],
+            module_name=None,
         )
 
         # ensure full paths given in the command-line without the drive letter resolve
         # to the full path correctly (#7628)
         drive, full_path_without_drive = os.path.splitdrive(full_path)
         assert resolve_collection_argument(
             invocation_path, full_path_without_drive
-        ) == (Path(os.path.abspath("src")), [])
+        ) == CollectionArgument(
+            path=Path(os.path.abspath("src")),
+            parts=[],
+            module_name=None,
+        )
 
 
 def test_module_full_path_without_drive(pytester: Pytester) -> None: