From ff4c3b2873296ee5bfffbcc86e728de830d5d409 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 1 Mar 2024 13:26:08 +0200 Subject: [PATCH 1/5] main: add missing `import importlib.util` Used in `resolve_collection_argument`. It's implicitly imported by some other import, but some type checkers don't recognize this. --- src/_pytest/main.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/_pytest/main.py b/src/_pytest/main.py index 639bf26c1cc..a5d8475150b 100644 --- a/src/_pytest/main.py +++ b/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 From 5e0d11746ce696af1ac754f102c1399aed846554 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 1 Mar 2024 11:47:30 +0200 Subject: [PATCH 2/5] main: model the result of `resolve_collection_arguments` as a dataclass In preparation of adding more info to it. --- src/_pytest/main.py | 50 +++++++++++++++++++++--------- testing/test_main.py | 72 ++++++++++++++++++++++++++++++-------------- 2 files changed, 86 insertions(+), 36 deletions(-) diff --git a/src/_pytest/main.py b/src/_pytest/main.py index a5d8475150b..8aa1d584aa5 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -564,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] = [] @@ -770,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) @@ -840,10 +840,13 @@ 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 + # resolve_collection_argument() ensures this. if argpath.is_dir(): assert not names, f"invalid arg {(argpath, names)!r}" @@ -862,7 +865,7 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]: 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() @@ -971,9 +974,17 @@ def search_pypath(module_name: str) -> str: return spec.origin +@dataclasses.dataclass(frozen=True) +class CollectionArgument: + """A resolved collection argument.""" + + path: Path + parts: Sequence[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 @@ -981,9 +992,12 @@ def resolve_collection_argument( "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"], + ) When as_pypath is True, expects that the command-line argument actually contains module paths instead of file-system paths: @@ -991,7 +1005,12 @@ def resolve_collection_argument( "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"], + ) If the path doesn't exist, raise UsageError. If the path is a directory and selection parts are present, raise UsageError. @@ -1018,4 +1037,7 @@ 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, + ) diff --git a/testing/test_main.py b/testing/test_main.py index d92fa21d365..d940dc51829 100644 --- a/testing/test_main.py +++ b/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,38 @@ 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=[], ) - 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=[""], ) 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"], + ) 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", ""], + ) 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=[], ) with pytest.raises( @@ -169,13 +182,21 @@ 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=[], + ) 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"], + ) + assert resolve_collection_argument( + invocation_path, "pkg", as_pypath=True + ) == CollectionArgument( + path=invocation_path / "src/pkg", + parts=[], ) with pytest.raises( @@ -186,10 +207,12 @@ 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]"], ) - 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,9 +232,11 @@ 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=[], ) # ensure full paths given in the command-line without the drive letter resolve @@ -219,7 +244,10 @@ def test_absolute_paths_are_resolved_correctly(self, invocation_path: Path) -> N 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=[], + ) def test_module_full_path_without_drive(pytester: Pytester) -> None: From 1612d4e393db42406a67430f848321999e0241b6 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 1 Mar 2024 11:59:26 +0200 Subject: [PATCH 3/5] main: add `module_name` to `CollectionArgument` This is available when the argument is a `--pyargs` argument (resolved from a python module path). Will be used in an upcoming commit. --- src/_pytest/main.py | 19 ++++++++++++++----- testing/test_main.py | 11 +++++++++++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/_pytest/main.py b/src/_pytest/main.py index 8aa1d584aa5..55bbf5b3984 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -957,17 +957,18 @@ 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: @@ -980,6 +981,7 @@ class CollectionArgument: path: Path parts: Sequence[str] + module_name: Optional[str] def resolve_collection_argument( @@ -997,6 +999,7 @@ def resolve_collection_argument( 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 @@ -1010,6 +1013,7 @@ def resolve_collection_argument( 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. @@ -1019,8 +1023,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): @@ -1040,4 +1048,5 @@ def resolve_collection_argument( return CollectionArgument( path=fspath, parts=parts, + module_name=module_name, ) diff --git a/testing/test_main.py b/testing/test_main.py index d940dc51829..345aa1e62cf 100644 --- a/testing/test_main.py +++ b/testing/test_main.py @@ -139,24 +139,28 @@ def test_file(self, invocation_path: Path) -> None: ) == CollectionArgument( path=invocation_path / "src/pkg/test.py", parts=[], + module_name=None, ) 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" ) == 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::" ) == CollectionArgument( path=invocation_path / "src/pkg/test.py", parts=["foo", "bar", ""], + module_name=None, ) def test_dir(self, invocation_path: Path) -> None: @@ -166,6 +170,7 @@ def test_dir(self, invocation_path: Path) -> None: ) == CollectionArgument( path=invocation_path / "src/pkg", parts=[], + module_name=None, ) with pytest.raises( @@ -185,18 +190,21 @@ def test_pypath(self, invocation_path: Path) -> None: ) == 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 ) == 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( @@ -212,6 +220,7 @@ def test_parametrized_name_with_colons(self, invocation_path: Path) -> None: ) == CollectionArgument( path=invocation_path / "src/pkg/test.py", parts=["test[a::b]"], + module_name=None, ) def test_does_not_exist(self, invocation_path: Path) -> None: @@ -237,6 +246,7 @@ def test_absolute_paths_are_resolved_correctly(self, invocation_path: Path) -> N ) == CollectionArgument( path=Path(os.path.abspath("src")), parts=[], + module_name=None, ) # ensure full paths given in the command-line without the drive letter resolve @@ -247,6 +257,7 @@ def test_absolute_paths_are_resolved_correctly(self, invocation_path: Path) -> N ) == CollectionArgument( path=Path(os.path.abspath("src")), parts=[], + module_name=None, ) From f20e32c98221c6d4cc22013f365e2e4eea475015 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 1 Mar 2024 14:17:03 +0200 Subject: [PATCH 4/5] main: slight refactor to collection argument parents logic No logical change, preparation for the next commit. --- src/_pytest/main.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/_pytest/main.py b/src/_pytest/main.py index 55bbf5b3984..faba6786de5 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -851,13 +851,14 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]: 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:] + # 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) # Start going over the parts from the root, collecting each level # and discarding all nodes which don't match the level's part. From 31026a2df2f05228887f99e902f5e6d861710451 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 1 Mar 2024 12:19:30 +0200 Subject: [PATCH 5/5] main: only include package parents in collection tree for --pyargs collection arguments (diff better viewed ignoring whitespace) In pytest<8, the collection tree for `pyargs` arguments in an invocation like this: pytest --collect-only --pyargs pyflakes.test.test_undefined_names looked like this: ``` ... snipped ... ``` The pytest 8 collection improvements changed it to this: ``` ... snipped ... ``` Besides being egregious (and potentially even worse than the above, going all the way to the root, for system-installed packages, as is apparently common in CI), this also caused permission errors when trying to probe some of those intermediate directories. This change makes `--pyargs` arguments no longer try to add parent directories to the collection tree according to the `--confcutdir` like they're regular arguments. Instead, only add the parents that are in the import path. This now looks like this: ``` ... snipped ... ``` Fix #11904. --- changelog/11904.bugfix.rst | 3 +++ src/_pytest/main.py | 20 ++++++++++++----- testing/test_collection.py | 45 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 changelog/11904.bugfix.rst diff --git a/changelog/11904.bugfix.rst b/changelog/11904.bugfix.rst new file mode 100644 index 00000000000..2aed9bcb059 --- /dev/null +++ b/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 --git a/src/_pytest/main.py b/src/_pytest/main.py index faba6786de5..b7ed72ddc3b 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -846,6 +846,7 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]: argpath = collection_argument.path names = collection_argument.parts + module_name = collection_argument.module_name # resolve_collection_argument() ensures this. if argpath.is_dir(): @@ -854,11 +855,20 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]: 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 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) + 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. diff --git a/testing/test_collection.py b/testing/test_collection.py index 0507400455c..fbc8543e9c4 100644 --- a/testing/test_collection.py +++ b/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( + [ + "", + " ", + " ", + " ", + ], + 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( + [ + "", + " ", + " ", + " ", + ], + consecutive=True, + )