From 68a4d9830afa602c2bfeba3b39eae96760268bff Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 30 Mar 2024 12:09:57 -0300 Subject: [PATCH 01/18] Add regression test for #12112 --- testing/test_pathlib.py | 69 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 4 deletions(-) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index 7f740a0607b..d123c18ff46 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1,5 +1,7 @@ # mypy: allow-untyped-defs import errno +import importlib.abc +import importlib.machinery import os.path from pathlib import Path import pickle @@ -10,6 +12,7 @@ from typing import Any from typing import Generator from typing import Iterator +from typing import Optional from typing import Tuple import unittest.mock @@ -1120,7 +1123,7 @@ class TestNamespacePackages: """Test import_path support when importing from properly namespace packages.""" def setup_directories( - self, tmp_path: Path, monkeypatch: MonkeyPatch, pytester: Pytester + self, tmp_path: Path, monkeypatch: Optional[MonkeyPatch], pytester: Pytester ) -> Tuple[Path, Path]: # Set up a namespace package "com.company", containing # two subpackages, "app" and "calc". @@ -1149,9 +1152,9 @@ def setup_directories( ) ) assert r.ret == 0 - - monkeypatch.syspath_prepend(tmp_path / "src/dist1") - monkeypatch.syspath_prepend(tmp_path / "src/dist2") + if monkeypatch is not None: + monkeypatch.syspath_prepend(tmp_path / "src/dist1") + monkeypatch.syspath_prepend(tmp_path / "src/dist2") return models_py, algorithms_py @pytest.mark.parametrize("import_mode", ["prepend", "append", "importlib"]) @@ -1235,3 +1238,61 @@ def test_incorrect_namespace_package( tmp_path / "src/dist1/com/company", "app.core.models", ) + + def test_detect_meta_path( + self, + tmp_path: Path, + monkeypatch: MonkeyPatch, + pytester: Pytester, + ) -> None: + """ + resolve_pkg_root_and_module_name() considers sys.meta_path when importing namespace packages. + + Regression test for #12112. + """ + + class CustomImporter(importlib.abc.MetaPathFinder): + """ + Imports the module name "com" as a namespace package. + + This ensures our namespace detection considers sys.meta_path, which is important + to support all possible ways a module can be imported (for example editable installs). + """ + + def find_spec( + self, name: str, path: Any = None, target: Any = None + ) -> Optional[importlib.machinery.ModuleSpec]: + if name == "com": + spec = importlib.machinery.ModuleSpec("com", loader=None) + spec.submodule_search_locations = [str(com_root_2), str(com_root_1)] + return spec + return None + + # Setup directories without configuring sys.path. + models_py, algorithms_py = self.setup_directories( + tmp_path, monkeypatch=None, pytester=pytester + ) + com_root_1 = tmp_path / "src/dist1/com" + com_root_2 = tmp_path / "src/dist2/com" + + # Because the namespace package is not setup correctly, we cannot resolve it as a namespace package. + pkg_root, module_name = resolve_pkg_root_and_module_name( + models_py, consider_namespace_packages=True + ) + assert (pkg_root, module_name) == ( + tmp_path / "src/dist1/com/company", + "app.core.models", + ) + + # Insert our custom importer, which will recognize the "com" directory as a namespace package. + new_meta_path = [CustomImporter(), *sys.meta_path] + monkeypatch.setattr(sys, "meta_path", new_meta_path) + + # Now we should be able to resolve the path as namespace package. + pkg_root, module_name = resolve_pkg_root_and_module_name( + models_py, consider_namespace_packages=True + ) + assert (pkg_root, module_name) == ( + tmp_path / "src/dist1", + "com.company.app.core.models", + ) From 8bd67cceaf7564adc17e760639a1f589fe95e2ed Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 30 Mar 2024 09:56:11 -0300 Subject: [PATCH 02/18] Refine how we detect namespace packages Previously we used a hand crafted approach to detect namespace packages, however we should rely on ``importlib`` to detect them for us. Fix #12112 --- changelog/12112.bugfix.rst | 1 + doc/en/reference/reference.rst | 3 +-- src/_pytest/pathlib.py | 43 +++++++++++++++++++++++++--------- 3 files changed, 34 insertions(+), 13 deletions(-) create mode 100644 changelog/12112.bugfix.rst diff --git a/changelog/12112.bugfix.rst b/changelog/12112.bugfix.rst new file mode 100644 index 00000000000..3f997b2af65 --- /dev/null +++ b/changelog/12112.bugfix.rst @@ -0,0 +1 @@ +Improve namespace packages detection when :confval:`consider_namespace_packages` is enabled, covering more situations (like editable installs). diff --git a/doc/en/reference/reference.rst b/doc/en/reference/reference.rst index 21890fbf63e..c9d7aeb552c 100644 --- a/doc/en/reference/reference.rst +++ b/doc/en/reference/reference.rst @@ -1279,8 +1279,7 @@ passed multiple times. The expected format is ``name=value``. For example:: Controls if pytest should attempt to identify `namespace packages `__ when collecting Python modules. Default is ``False``. - Set to ``True`` if you are testing namespace packages installed into a virtual environment and it is important for - your packages to be imported using their full namespace package name. + Set to ``True`` if the package you are testing is part of a namespace package. Only `native namespace packages `__ are supported, with no plans to support `legacy namespace packages `__. diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index e39f4772326..d6ad7e88fdf 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -771,19 +771,11 @@ def resolve_pkg_root_and_module_name( pkg_path = resolve_package_path(path) if pkg_path is not None: pkg_root = pkg_path.parent - # https://packaging.python.org/en/latest/guides/packaging-namespace-packages/ if consider_namespace_packages: - # Go upwards in the hierarchy, if we find a parent path included - # in sys.path, it means the package found by resolve_package_path() - # actually belongs to a namespace package. - for parent in pkg_root.parents: - # If any of the parent paths has a __init__.py, it means it is not - # a namespace package (see the docs linked above). - if (parent / "__init__.py").is_file(): - break - if str(parent) in sys.path: + for candidate in (pkg_root, *pkg_root.parents): + if _is_namespace_package(candidate): # Point the pkg_root to the root of the namespace package. - pkg_root = parent + pkg_root = candidate.parent break names = list(path.with_suffix("").relative_to(pkg_root).parts) @@ -795,6 +787,35 @@ def resolve_pkg_root_and_module_name( raise CouldNotResolvePathError(f"Could not resolve for {path}") +def _is_namespace_package(module_path: Path) -> bool: + # If the path has na __init__.py file, it means it is not + # a namespace package:. + # https://packaging.python.org/en/latest/guides/packaging-namespace-packages. + if (module_path / "__init__.py").is_file(): + return False + + module_name = module_path.name + + # Empty module names break find_spec. + if not module_name: + return False + + # Modules starting with "." indicate relative imports and break find_spec, and we are only attempting + # to find top-level namespace packages anyway. + if module_name.startswith("."): + return False + + spec = importlib.util.find_spec(module_name) + if spec is not None and spec.submodule_search_locations: + # Found a spec, however make sure the module_path is in one of the search locations -- + # this ensures common module name like "src" (which might be in sys.path under different locations) + # is only considered for the module_path we intend to. + # Make sure to compare Path(s) instead of strings, this normalizes them on Windows. + if module_path in [Path(x) for x in spec.submodule_search_locations]: + return True + return False + + class CouldNotResolvePathError(Exception): """Custom exception raised by resolve_pkg_root_and_module_name.""" From fedeb6b0109dcc262c29e9e8e68fe8c8a31433a1 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 30 Mar 2024 17:10:09 -0300 Subject: [PATCH 03/18] Update src/_pytest/pathlib.py Co-authored-by: Ran Benita --- src/_pytest/pathlib.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index d6ad7e88fdf..70479fe5abf 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -788,9 +788,9 @@ def resolve_pkg_root_and_module_name( def _is_namespace_package(module_path: Path) -> bool: - # If the path has na __init__.py file, it means it is not - # a namespace package:. - # https://packaging.python.org/en/latest/guides/packaging-namespace-packages. + # If the path has an __init__.py file, it means it is not + # a namespace package: + # https://packaging.python.org/en/latest/guides/packaging-namespace-packages if (module_path / "__init__.py").is_file(): return False From 7a25556ae56e0b8522f6990dce88df028069ba35 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 31 Mar 2024 09:35:48 -0300 Subject: [PATCH 04/18] Improve check for __init__ files --- src/_pytest/pathlib.py | 11 +++++------ testing/test_pathlib.py | 21 ++++++++++++++++++--- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 70479fe5abf..27504230ebe 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -773,6 +773,11 @@ def resolve_pkg_root_and_module_name( pkg_root = pkg_path.parent if consider_namespace_packages: for candidate in (pkg_root, *pkg_root.parents): + # If any of the parent paths has an __init__.py, it means it is not a namespace package: + # https://packaging.python.org/en/latest/guides/packaging-namespace-packages + if (candidate / "__init__.py").is_file(): + break + if _is_namespace_package(candidate): # Point the pkg_root to the root of the namespace package. pkg_root = candidate.parent @@ -788,12 +793,6 @@ def resolve_pkg_root_and_module_name( def _is_namespace_package(module_path: Path) -> bool: - # If the path has an __init__.py file, it means it is not - # a namespace package: - # https://packaging.python.org/en/latest/guides/packaging-namespace-packages - if (module_path / "__init__.py").is_file(): - return False - module_name = module_path.name # Empty module names break find_spec. diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index d123c18ff46..8a162d9ad48 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1226,11 +1226,26 @@ def test_incorrect_namespace_package( models_py, algorithms_py = self.setup_directories( tmp_path, monkeypatch, pytester ) - # Namespace packages must not have an __init__.py at any of its - # directories; if it does, we then fall back to importing just the - # part of the package containing the __init__.py files. + # Namespace packages must not have an __init__.py at its top-level + # directory; if it does, it is no longer a namespace package and we fall back + # to importing just the part of the package containing the __init__.py files. (tmp_path / "src/dist1/com/__init__.py").touch() + # Ensure Python no longer considers dist1/com a namespace package. + r = pytester.runpython_c( + dedent( + f""" + import sys + sys.path.append(r{str(tmp_path / "src/dist1")!r}) + sys.path.append(r{str(tmp_path / "src/dist2")!r}) + import com.company.app.core.models + import com.company.calc.algo.algorithms + """ + ) + ) + assert r.ret == 1 + r.stderr.fnmatch_lines("*No module named 'com.company.calc*") + pkg_root, module_name = resolve_pkg_root_and_module_name( models_py, consider_namespace_packages=True ) From 09643d9eba6fb1580b28172a832f0dbea099cbdf Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 31 Mar 2024 09:47:21 -0300 Subject: [PATCH 05/18] Drop explicit checks from is_namespace_package and add tests --- src/_pytest/pathlib.py | 10 ++++------ testing/test_pathlib.py | 13 +++++++++++++ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 27504230ebe..054c21445e2 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -795,16 +795,14 @@ def resolve_pkg_root_and_module_name( def _is_namespace_package(module_path: Path) -> bool: module_name = module_path.name - # Empty module names break find_spec. + # Empty module names (such as Path.cwd()) might break meta_path hooks (like our own assertion rewriter). if not module_name: return False - # Modules starting with "." indicate relative imports and break find_spec, and we are only attempting - # to find top-level namespace packages anyway. - if module_name.startswith("."): + try: + spec = importlib.util.find_spec(module_name) + except ImportError: return False - - spec = importlib.util.find_spec(module_name) if spec is not None and spec.submodule_search_locations: # Found a spec, however make sure the module_path is in one of the search locations -- # this ensures common module name like "src" (which might be in sys.path under different locations) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index 8a162d9ad48..a4cb77932c1 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -17,6 +17,7 @@ import unittest.mock from _pytest.monkeypatch import MonkeyPatch +from _pytest.pathlib import _is_namespace_package from _pytest.pathlib import bestrelpath from _pytest.pathlib import commonpath from _pytest.pathlib import CouldNotResolvePathError @@ -1311,3 +1312,15 @@ def find_spec( tmp_path / "src/dist1", "com.company.app.core.models", ) + + def test_is_namespace_package_bad_arguments(self, pytester: Pytester) -> None: + pytester.syspathinsert() + path = pytester.path / "bar.x" + path.mkdir() + assert _is_namespace_package(path) is False + + path = pytester.path / ".bar.x" + path.mkdir() + assert _is_namespace_package(path) is False + + assert _is_namespace_package(Path()) is False From 6e5d2e13bc23d6aae9886a02cef4baca50c081c6 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 31 Mar 2024 10:09:26 -0300 Subject: [PATCH 06/18] Add test for more complex ns with no __init__ file --- testing/test_pathlib.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index a4cb77932c1..fb367b653e4 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1324,3 +1324,40 @@ def test_is_namespace_package_bad_arguments(self, pytester: Pytester) -> None: assert _is_namespace_package(path) is False assert _is_namespace_package(Path()) is False + + def test_intermediate_init_file( + self, pytester: Pytester, tmp_path: Path, monkeypatch: MonkeyPatch + ) -> None: + (tmp_path / "src/dist1/ns/b/app/bar/test").mkdir(parents=True) + # (tmp_path / "src/dist1/ns/b/app/bar/test/__init__.py").touch() + (tmp_path / "src/dist1/ns/b/app/bar/m.py").touch() + + # The presence of this __init__.py is not a problem, ns.b.app is still part of the namespace package. + (tmp_path / "src/dist1/ns/b/app/__init__.py").touch() + + (tmp_path / "src/dist2/ns/a/core/foo/test").mkdir(parents=True) + # (tmp_path / "src/dist2/ns/a/core/foo/test/__init__.py").touch() + (tmp_path / "src/dist2/ns/a/core/foo/m.py").touch() + + # Validate the namespace package by importing it in a Python subprocess. + r = pytester.runpython_c( + dedent( + f""" + import sys + sys.path.append(r{str(tmp_path / "src/dist1")!r}) + sys.path.append(r{str(tmp_path / "src/dist2")!r}) + import ns.b.app.bar.m + import ns.a.core.foo.m + """ + ) + ) + assert r.ret == 0 + monkeypatch.syspath_prepend(tmp_path / "src/dist1") + monkeypatch.syspath_prepend(tmp_path / "src/dist2") + + assert resolve_pkg_root_and_module_name( + tmp_path / "src/dist1/ns/b/app/bar/m.py", consider_namespace_packages=True + ) == (tmp_path / "src/dist1", "ns.b.app.bar.m") + assert resolve_pkg_root_and_module_name( + tmp_path / "src/dist2/ns/a/core/foo/m.py", consider_namespace_packages=True + ) == (tmp_path / "src/dist2", "ns.a.core.foo.m") From 8fd733355498f063ce67cf31b34e72bdd8e923c3 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 31 Mar 2024 10:20:15 -0300 Subject: [PATCH 07/18] Reuse code to test the namespace packages --- testing/test_pathlib.py | 57 +++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index fb367b653e4..d5358a6cd09 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -13,6 +13,7 @@ from typing import Generator from typing import Iterator from typing import Optional +from typing import Sequence from typing import Tuple import unittest.mock @@ -37,6 +38,7 @@ from _pytest.pathlib import symlink_or_skip from _pytest.pathlib import visit from _pytest.pytester import Pytester +from _pytest.pytester import RunResult from _pytest.tmpdir import TempPathFactory import pytest @@ -1141,16 +1143,10 @@ def setup_directories( algorithms_py.touch() # Validate the namespace package by importing it in a Python subprocess. - r = pytester.runpython_c( - dedent( - f""" - import sys - sys.path.append(r{str(tmp_path / "src/dist1")!r}) - sys.path.append(r{str(tmp_path / "src/dist2")!r}) - import com.company.app.core.models - import com.company.calc.algo.algorithms - """ - ) + r = self.run_ns_imports( + pytester, + [tmp_path / "src/dist1", tmp_path / "src/dist2"], + ["com.company.app.core.models", "com.company.calc.algo.algorithms"], ) assert r.ret == 0 if monkeypatch is not None: @@ -1158,6 +1154,19 @@ def setup_directories( monkeypatch.syspath_prepend(tmp_path / "src/dist2") return models_py, algorithms_py + def run_ns_imports( + self, pytester: Pytester, paths: Sequence[Path], imports: Sequence[str] + ) -> RunResult: + """Validate a Python namespace package by importing modules from it in a Python subprocess""" + lines = [ + "import sys", + # Configure sys.path. + *[f"sys.path.append(r{str(x)!r})" for x in paths], + # Imports. + *[f"import {x}" for x in imports], + ] + return pytester.runpython_c("\n".join(lines)) + @pytest.mark.parametrize("import_mode", ["prepend", "append", "importlib"]) def test_resolve_pkg_root_and_module_name_ns_multiple_levels( self, @@ -1233,16 +1242,10 @@ def test_incorrect_namespace_package( (tmp_path / "src/dist1/com/__init__.py").touch() # Ensure Python no longer considers dist1/com a namespace package. - r = pytester.runpython_c( - dedent( - f""" - import sys - sys.path.append(r{str(tmp_path / "src/dist1")!r}) - sys.path.append(r{str(tmp_path / "src/dist2")!r}) - import com.company.app.core.models - import com.company.calc.algo.algorithms - """ - ) + r = self.run_ns_imports( + pytester, + [tmp_path / "src/dist1", tmp_path / "src/dist2"], + ["com.company.app.core.models", "com.company.calc.algo.algorithms"], ) assert r.ret == 1 r.stderr.fnmatch_lines("*No module named 'com.company.calc*") @@ -1340,16 +1343,10 @@ def test_intermediate_init_file( (tmp_path / "src/dist2/ns/a/core/foo/m.py").touch() # Validate the namespace package by importing it in a Python subprocess. - r = pytester.runpython_c( - dedent( - f""" - import sys - sys.path.append(r{str(tmp_path / "src/dist1")!r}) - sys.path.append(r{str(tmp_path / "src/dist2")!r}) - import ns.b.app.bar.m - import ns.a.core.foo.m - """ - ) + r = self.run_ns_imports( + pytester, + [tmp_path / "src/dist1", tmp_path / "src/dist2"], + ["ns.b.app.bar.m", "ns.a.core.foo.m"], ) assert r.ret == 0 monkeypatch.syspath_prepend(tmp_path / "src/dist1") From f52327953101b325b41d0c97a0ff908131db5676 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Apr 2024 10:40:26 -0300 Subject: [PATCH 08/18] Support full namespace packages without __init__ --- src/_pytest/pathlib.py | 64 ++++++++++++++++++++--------------- testing/test_pathlib.py | 75 ++++++++++++++++++++++++----------------- 2 files changed, 82 insertions(+), 57 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 054c21445e2..05f125d7d61 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -8,6 +8,7 @@ from errno import ENOTDIR import fnmatch from functools import partial +from importlib.machinery import ModuleSpec import importlib.util import itertools import os @@ -36,6 +37,8 @@ import uuid import warnings +from typing_extensions import TypeGuard + from _pytest.compat import assert_never from _pytest.outcomes import skip from _pytest.warning_types import PytestWarning @@ -628,11 +631,12 @@ def _import_module_using_spec( # such as our own assertion-rewrite hook. for meta_importer in sys.meta_path: spec = meta_importer.find_spec(module_name, [str(module_location)]) - if spec is not None: + if spec_matches_module_path(spec, module_path): break else: spec = importlib.util.spec_from_file_location(module_name, str(module_path)) - if spec is not None: + + if spec_matches_module_path(spec, module_path): mod = importlib.util.module_from_spec(spec) sys.modules[module_name] = mod spec.loader.exec_module(mod) # type: ignore[union-attr] @@ -643,6 +647,15 @@ def _import_module_using_spec( return None +def spec_matches_module_path( + module_spec: Optional[ModuleSpec], module_path: Path +) -> TypeGuard[ModuleSpec]: + if module_spec is not None and module_spec.origin is not None: + if Path(module_spec.origin) == module_path: + return True + return False + + # Implement a special _is_same function on Windows which returns True if the two filenames # compare equal, to circumvent os.path.samefile returning False for mounts in UNC (#7678). if sys.platform.startswith("win"): @@ -768,21 +781,19 @@ def resolve_pkg_root_and_module_name( Raises CouldNotResolvePathError if the given path does not belong to a package (missing any __init__.py files). """ + pkg_root: Optional[Path] = None pkg_path = resolve_package_path(path) if pkg_path is not None: pkg_root = pkg_path.parent - if consider_namespace_packages: - for candidate in (pkg_root, *pkg_root.parents): - # If any of the parent paths has an __init__.py, it means it is not a namespace package: - # https://packaging.python.org/en/latest/guides/packaging-namespace-packages - if (candidate / "__init__.py").is_file(): - break - - if _is_namespace_package(candidate): - # Point the pkg_root to the root of the namespace package. - pkg_root = candidate.parent - break + if consider_namespace_packages: + start = path.parent if pkg_path is None else pkg_path.parent + for candidate in (start, *start.parents): + if _is_importable(candidate, path): + # Point the pkg_root to the root of the namespace package. + pkg_root = candidate.parent + break + if pkg_root is not None: names = list(path.with_suffix("").relative_to(pkg_root).parts) if names[-1] == "__init__": names.pop() @@ -792,25 +803,24 @@ def resolve_pkg_root_and_module_name( raise CouldNotResolvePathError(f"Could not resolve for {path}") -def _is_namespace_package(module_path: Path) -> bool: - module_name = module_path.name - - # Empty module names (such as Path.cwd()) might break meta_path hooks (like our own assertion rewriter). - if not module_name: +def _is_importable(root: Path, path: Path) -> bool: + try: + path_without_suffix = path.with_suffix("") + except ValueError: + # Empty paths (such as Path.cwd()) might break meta_path hooks (like our own assertion rewriter). return False + names = list(path_without_suffix.relative_to(root.parent).parts) + if names[-1] == "__init__": + names.pop() + module_name = ".".join(names) + try: spec = importlib.util.find_spec(module_name) - except ImportError: + except (ImportError, ValueError, ImportWarning): return False - if spec is not None and spec.submodule_search_locations: - # Found a spec, however make sure the module_path is in one of the search locations -- - # this ensures common module name like "src" (which might be in sys.path under different locations) - # is only considered for the module_path we intend to. - # Make sure to compare Path(s) instead of strings, this normalizes them on Windows. - if module_path in [Path(x) for x in spec.submodule_search_locations]: - return True - return False + else: + return spec_matches_module_path(spec, path) class CouldNotResolvePathError(Exception): diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index d5358a6cd09..6c5b2fb57a1 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -18,7 +18,7 @@ import unittest.mock from _pytest.monkeypatch import MonkeyPatch -from _pytest.pathlib import _is_namespace_package +from _pytest.pathlib import _is_importable from _pytest.pathlib import bestrelpath from _pytest.pathlib import commonpath from _pytest.pathlib import CouldNotResolvePathError @@ -723,12 +723,13 @@ def test_module_name_from_path(self, tmp_path: Path) -> None: assert result == "_env_310.tests.test_foo" def test_resolve_pkg_root_and_module_name( - self, tmp_path: Path, monkeypatch: MonkeyPatch + self, tmp_path: Path, monkeypatch: MonkeyPatch, pytester: Pytester ) -> None: # Create a directory structure first without __init__.py files. (tmp_path / "src/app/core").mkdir(parents=True) models_py = tmp_path / "src/app/core/models.py" models_py.touch() + with pytest.raises(CouldNotResolvePathError): _ = resolve_pkg_root_and_module_name(models_py) @@ -744,6 +745,8 @@ def test_resolve_pkg_root_and_module_name( # If we add tmp_path to sys.path, src becomes a namespace package. monkeypatch.syspath_prepend(tmp_path) + validate_namespace_package(pytester, [tmp_path], ["src.app.core.models"]) + assert resolve_pkg_root_and_module_name( models_py, consider_namespace_packages=True ) == ( @@ -1143,7 +1146,7 @@ def setup_directories( algorithms_py.touch() # Validate the namespace package by importing it in a Python subprocess. - r = self.run_ns_imports( + r = validate_namespace_package( pytester, [tmp_path / "src/dist1", tmp_path / "src/dist2"], ["com.company.app.core.models", "com.company.calc.algo.algorithms"], @@ -1154,19 +1157,6 @@ def setup_directories( monkeypatch.syspath_prepend(tmp_path / "src/dist2") return models_py, algorithms_py - def run_ns_imports( - self, pytester: Pytester, paths: Sequence[Path], imports: Sequence[str] - ) -> RunResult: - """Validate a Python namespace package by importing modules from it in a Python subprocess""" - lines = [ - "import sys", - # Configure sys.path. - *[f"sys.path.append(r{str(x)!r})" for x in paths], - # Imports. - *[f"import {x}" for x in imports], - ] - return pytester.runpython_c("\n".join(lines)) - @pytest.mark.parametrize("import_mode", ["prepend", "append", "importlib"]) def test_resolve_pkg_root_and_module_name_ns_multiple_levels( self, @@ -1242,7 +1232,7 @@ def test_incorrect_namespace_package( (tmp_path / "src/dist1/com/__init__.py").touch() # Ensure Python no longer considers dist1/com a namespace package. - r = self.run_ns_imports( + r = validate_namespace_package( pytester, [tmp_path / "src/dist1", tmp_path / "src/dist2"], ["com.company.app.core.models", "com.company.calc.algo.algorithms"], @@ -1250,12 +1240,23 @@ def test_incorrect_namespace_package( assert r.ret == 1 r.stderr.fnmatch_lines("*No module named 'com.company.calc*") + # dist1 is not a namespace package, but its module is importable; not being a namespace + # package prevents "com.company.calc" from being importable. pkg_root, module_name = resolve_pkg_root_and_module_name( models_py, consider_namespace_packages=True ) assert (pkg_root, module_name) == ( - tmp_path / "src/dist1/com/company", - "app.core.models", + tmp_path / "src/dist1", + "com.company.app.core.models", + ) + + # dist2/com/company will contain a normal Python package. + pkg_root, module_name = resolve_pkg_root_and_module_name( + algorithms_py, consider_namespace_packages=True + ) + assert (pkg_root, module_name) == ( + tmp_path / "src/dist2/com/company", + "calc.algo.algorithms", ) def test_detect_meta_path( @@ -1316,34 +1317,34 @@ def find_spec( "com.company.app.core.models", ) - def test_is_namespace_package_bad_arguments(self, pytester: Pytester) -> None: + def test_is_importable_bad_arguments(self, pytester: Pytester) -> None: pytester.syspathinsert() path = pytester.path / "bar.x" path.mkdir() - assert _is_namespace_package(path) is False + assert _is_importable(path.parent, path) is False path = pytester.path / ".bar.x" path.mkdir() - assert _is_namespace_package(path) is False + assert _is_importable(path.parent, path) is False - assert _is_namespace_package(Path()) is False + assert _is_importable(Path(), Path()) is False - def test_intermediate_init_file( - self, pytester: Pytester, tmp_path: Path, monkeypatch: MonkeyPatch + @pytest.mark.parametrize("insert", [True, False]) + def test_full_ns_packages_without_init_files( + self, pytester: Pytester, tmp_path: Path, monkeypatch: MonkeyPatch, insert: bool ) -> None: (tmp_path / "src/dist1/ns/b/app/bar/test").mkdir(parents=True) - # (tmp_path / "src/dist1/ns/b/app/bar/test/__init__.py").touch() (tmp_path / "src/dist1/ns/b/app/bar/m.py").touch() - # The presence of this __init__.py is not a problem, ns.b.app is still part of the namespace package. - (tmp_path / "src/dist1/ns/b/app/__init__.py").touch() + if insert: + # The presence of this __init__.py is not a problem, ns.b.app is still part of the namespace package. + (tmp_path / "src/dist1/ns/b/app/__init__.py").touch() (tmp_path / "src/dist2/ns/a/core/foo/test").mkdir(parents=True) - # (tmp_path / "src/dist2/ns/a/core/foo/test/__init__.py").touch() (tmp_path / "src/dist2/ns/a/core/foo/m.py").touch() # Validate the namespace package by importing it in a Python subprocess. - r = self.run_ns_imports( + r = validate_namespace_package( pytester, [tmp_path / "src/dist1", tmp_path / "src/dist2"], ["ns.b.app.bar.m", "ns.a.core.foo.m"], @@ -1358,3 +1359,17 @@ def test_intermediate_init_file( assert resolve_pkg_root_and_module_name( tmp_path / "src/dist2/ns/a/core/foo/m.py", consider_namespace_packages=True ) == (tmp_path / "src/dist2", "ns.a.core.foo.m") + + +def validate_namespace_package( + pytester: Pytester, paths: Sequence[Path], imports: Sequence[str] +) -> RunResult: + """Validate a Python namespace package by importing modules from it in a Python subprocess""" + lines = [ + "import sys", + # Configure sys.path. + *[f"sys.path.append(r{str(x)!r})" for x in paths], + # Imports. + *[f"import {x}" for x in imports], + ] + return pytester.runpython_c("\n".join(lines)) From fddb7db189264e2d64fbf960e105e3293f9b8300 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Apr 2024 11:12:28 -0300 Subject: [PATCH 09/18] Refactor code and improve docs --- src/_pytest/pathlib.py | 53 ++++++++++++++++++++++++----------------- testing/test_pathlib.py | 34 ++++++++++++++++++++------ 2 files changed, 58 insertions(+), 29 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 05f125d7d61..b7b67f04d01 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -37,8 +37,6 @@ import uuid import warnings -from typing_extensions import TypeGuard - from _pytest.compat import assert_never from _pytest.outcomes import skip from _pytest.warning_types import PytestWarning @@ -637,6 +635,7 @@ def _import_module_using_spec( spec = importlib.util.spec_from_file_location(module_name, str(module_path)) if spec_matches_module_path(spec, module_path): + assert spec is not None mod = importlib.util.module_from_spec(spec) sys.modules[module_name] = mod spec.loader.exec_module(mod) # type: ignore[union-attr] @@ -649,7 +648,8 @@ def _import_module_using_spec( def spec_matches_module_path( module_spec: Optional[ModuleSpec], module_path: Path -) -> TypeGuard[ModuleSpec]: +) -> bool: + """Return true if the given ModuleSpec can be used to import the given module path.""" if module_spec is not None and module_spec.origin is not None: if Path(module_spec.origin) == module_path: return True @@ -775,7 +775,7 @@ def resolve_pkg_root_and_module_name( Passing the full path to `models.py` will yield Path("src") and "app.core.models". If consider_namespace_packages is True, then we additionally check upwards in the hierarchy - until we find a directory that is reachable from sys.path, which marks it as a namespace package: + for namespace packages: https://packaging.python.org/en/latest/guides/packaging-namespace-packages @@ -788,39 +788,48 @@ def resolve_pkg_root_and_module_name( if consider_namespace_packages: start = path.parent if pkg_path is None else pkg_path.parent for candidate in (start, *start.parents): - if _is_importable(candidate, path): + if is_importable(candidate, path): # Point the pkg_root to the root of the namespace package. - pkg_root = candidate.parent + pkg_root = candidate break if pkg_root is not None: - names = list(path.with_suffix("").relative_to(pkg_root).parts) - if names[-1] == "__init__": - names.pop() - module_name = ".".join(names) + module_name = compute_module_name(pkg_root, path) return pkg_root, module_name raise CouldNotResolvePathError(f"Could not resolve for {path}") -def _is_importable(root: Path, path: Path) -> bool: +def is_importable(root: Path, module_path: Path) -> bool: + """ + Return if the given module path could be imported normally by Python, akin to the user + entering the REPL and importing the corresponding module name directly. + """ + module_name = compute_module_name(root, module_path) + try: + # Note this is different from what we do in ``import_path``, where we also search sys.meta_path. + # Searching sys.meta_path will eventually find a spec which can load the file even if the interpreter would + # not find this module normally in the REPL, which is exactly what we want to be able to do in + # ``import_path``, but not here. + spec = importlib.util.find_spec(module_name) + except (ImportError, ValueError, ImportWarning): + return False + else: + return spec_matches_module_path(spec, module_path) + + +def compute_module_name(root: Path, path: Path) -> str: + """Compute a module name based on a path and a root anchor.""" try: path_without_suffix = path.with_suffix("") except ValueError: # Empty paths (such as Path.cwd()) might break meta_path hooks (like our own assertion rewriter). - return False + return "" - names = list(path_without_suffix.relative_to(root.parent).parts) - if names[-1] == "__init__": + names = list(path_without_suffix.relative_to(root).parts) + if names and names[-1] == "__init__": names.pop() - module_name = ".".join(names) - - try: - spec = importlib.util.find_spec(module_name) - except (ImportError, ValueError, ImportWarning): - return False - else: - return spec_matches_module_path(spec, path) + return ".".join(names) class CouldNotResolvePathError(Exception): diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index 6c5b2fb57a1..487d4725486 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -18,9 +18,9 @@ import unittest.mock from _pytest.monkeypatch import MonkeyPatch -from _pytest.pathlib import _is_importable from _pytest.pathlib import bestrelpath from _pytest.pathlib import commonpath +from _pytest.pathlib import compute_module_name from _pytest.pathlib import CouldNotResolvePathError from _pytest.pathlib import ensure_deletable from _pytest.pathlib import fnmatch_ex @@ -30,6 +30,7 @@ from _pytest.pathlib import ImportMode from _pytest.pathlib import ImportPathMismatchError from _pytest.pathlib import insert_missing_modules +from _pytest.pathlib import is_importable from _pytest.pathlib import maybe_delete_a_numbered_dir from _pytest.pathlib import module_name_from_path from _pytest.pathlib import resolve_package_path @@ -1321,13 +1322,25 @@ def test_is_importable_bad_arguments(self, pytester: Pytester) -> None: pytester.syspathinsert() path = pytester.path / "bar.x" path.mkdir() - assert _is_importable(path.parent, path) is False + assert is_importable(path.parent, path) is False path = pytester.path / ".bar.x" path.mkdir() - assert _is_importable(path.parent, path) is False + assert is_importable(path.parent, path) is False - assert _is_importable(Path(), Path()) is False + def test_compute_module_name(self, tmp_path: Path) -> None: + assert compute_module_name(tmp_path, tmp_path) == "" + assert compute_module_name(Path(), Path()) == "" + + assert compute_module_name(tmp_path, tmp_path / "mod.py") == "mod" + assert compute_module_name(tmp_path, tmp_path / "src/app/bar") == "src.app.bar" + assert ( + compute_module_name(tmp_path, tmp_path / "src/app/bar.py") == "src.app.bar" + ) + assert ( + compute_module_name(tmp_path, tmp_path / "src/app/bar/__init__.py") + == "src.app.bar" + ) @pytest.mark.parametrize("insert", [True, False]) def test_full_ns_packages_without_init_files( @@ -1362,14 +1375,21 @@ def test_full_ns_packages_without_init_files( def validate_namespace_package( - pytester: Pytester, paths: Sequence[Path], imports: Sequence[str] + pytester: Pytester, paths: Sequence[Path], modules: Sequence[str] ) -> RunResult: - """Validate a Python namespace package by importing modules from it in a Python subprocess""" + """ + Validate that a Python namespace package is set up correctly. + + In a sub interpreter, add 'paths' to sys.path and attempt to import the given modules. + + In this module many tests configure a set of files as a namespace package, this function + is used as sanity check that our files are configured correctly from the point of view of Python. + """ lines = [ "import sys", # Configure sys.path. *[f"sys.path.append(r{str(x)!r})" for x in paths], # Imports. - *[f"import {x}" for x in imports], + *[f"import {x}" for x in modules], ] return pytester.runpython_c("\n".join(lines)) From 24c7d2c2a02fe5d76f1e8caf44de3ca9cd78d4ca Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Apr 2024 12:11:00 -0300 Subject: [PATCH 10/18] Simplify spec_matches_module_path --- src/_pytest/pathlib.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index b7b67f04d01..90cd01a8423 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -650,10 +650,10 @@ def spec_matches_module_path( module_spec: Optional[ModuleSpec], module_path: Path ) -> bool: """Return true if the given ModuleSpec can be used to import the given module path.""" - if module_spec is not None and module_spec.origin is not None: - if Path(module_spec.origin) == module_path: - return True - return False + if module_spec is None or module_spec.origin is None: + return False + + return Path(module_spec.origin) == module_path # Implement a special _is_same function on Windows which returns True if the two filenames From 22afde54e77aec0f837c1c65230f6bb2deb9d9b1 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Apr 2024 12:13:46 -0300 Subject: [PATCH 11/18] Improve readability in resolve_pkg_root_and_module_name a bit This makes it more clear what is the purpose of the start variable. --- src/_pytest/pathlib.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 90cd01a8423..b83619f56c4 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -786,7 +786,7 @@ def resolve_pkg_root_and_module_name( if pkg_path is not None: pkg_root = pkg_path.parent if consider_namespace_packages: - start = path.parent if pkg_path is None else pkg_path.parent + start = pkg_root if pkg_root is not None else path.parent for candidate in (start, *start.parents): if is_importable(candidate, path): # Point the pkg_root to the root of the namespace package. From 34980949f39d72d7337cc1554de5adc301d299c0 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Apr 2024 12:24:58 -0300 Subject: [PATCH 12/18] Change issue to improvement --- ...12112.bugfix.rst => 12112.improvement.rst} | 0 testing/test_pathlib.py | 34 ++++++++++++++++--- 2 files changed, 29 insertions(+), 5 deletions(-) rename changelog/{12112.bugfix.rst => 12112.improvement.rst} (100%) diff --git a/changelog/12112.bugfix.rst b/changelog/12112.improvement.rst similarity index 100% rename from changelog/12112.bugfix.rst rename to changelog/12112.improvement.rst diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index 487d4725486..d83153d5659 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1129,22 +1129,46 @@ def test_safe_exists(tmp_path: Path) -> None: class TestNamespacePackages: """Test import_path support when importing from properly namespace packages.""" + @pytest.fixture(autouse=True) + def setup_imports_tracking(self, monkeypatch: MonkeyPatch) -> None: + monkeypatch.setattr(sys, "pytest_namespace_packages_test", [], raising=False) + def setup_directories( self, tmp_path: Path, monkeypatch: Optional[MonkeyPatch], pytester: Pytester ) -> Tuple[Path, Path]: + # Use a code to guard against modules being imported more than once. + # This is a safeguard in case future changes break this invariant. + code = dedent( + """ + import sys + imported = getattr(sys, "pytest_namespace_packages_test", []) + assert __name__ not in imported, f"{__name__} already imported" + imported.append(__name__) + sys.pytest_namespace_packages_test = imported + """ + ) + # Set up a namespace package "com.company", containing # two subpackages, "app" and "calc". (tmp_path / "src/dist1/com/company/app/core").mkdir(parents=True) - (tmp_path / "src/dist1/com/company/app/__init__.py").touch() - (tmp_path / "src/dist1/com/company/app/core/__init__.py").touch() + (tmp_path / "src/dist1/com/company/app/__init__.py").write_text( + code, encoding="UTF-8" + ) + (tmp_path / "src/dist1/com/company/app/core/__init__.py").write_text( + code, encoding="UTF-8" + ) models_py = tmp_path / "src/dist1/com/company/app/core/models.py" models_py.touch() (tmp_path / "src/dist2/com/company/calc/algo").mkdir(parents=True) - (tmp_path / "src/dist2/com/company/calc/__init__.py").touch() - (tmp_path / "src/dist2/com/company/calc/algo/__init__.py").touch() + (tmp_path / "src/dist2/com/company/calc/__init__.py").write_text( + code, encoding="UTF-8" + ) + (tmp_path / "src/dist2/com/company/calc/algo/__init__.py").write_text( + code, encoding="UTF-8" + ) algorithms_py = tmp_path / "src/dist2/com/company/calc/algo/algorithms.py" - algorithms_py.touch() + algorithms_py.write_text(code, encoding="UTF-8") # Validate the namespace package by importing it in a Python subprocess. r = validate_namespace_package( From 495a6a8d5fbca442755956ca635ea2316d1638d5 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Apr 2024 12:27:04 -0300 Subject: [PATCH 13/18] Raname path -> module_path for clarity --- src/_pytest/pathlib.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index b83619f56c4..ff12eb453d9 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -818,10 +818,10 @@ def is_importable(root: Path, module_path: Path) -> bool: return spec_matches_module_path(spec, module_path) -def compute_module_name(root: Path, path: Path) -> str: +def compute_module_name(root: Path, module_path: Path) -> str: """Compute a module name based on a path and a root anchor.""" try: - path_without_suffix = path.with_suffix("") + path_without_suffix = module_path.with_suffix("") except ValueError: # Empty paths (such as Path.cwd()) might break meta_path hooks (like our own assertion rewriter). return "" From 04cbe4f6409df0072148b6a03c6d655a8ff0387f Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Apr 2024 12:31:25 -0300 Subject: [PATCH 14/18] Cleanup test_pathlib a bit --- testing/test_pathlib.py | 57 ++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index d83153d5659..08395faa831 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1170,7 +1170,6 @@ def setup_directories( algorithms_py = tmp_path / "src/dist2/com/company/calc/algo/algorithms.py" algorithms_py.write_text(code, encoding="UTF-8") - # Validate the namespace package by importing it in a Python subprocess. r = validate_namespace_package( pytester, [tmp_path / "src/dist1", tmp_path / "src/dist2"], @@ -1252,11 +1251,13 @@ def test_incorrect_namespace_package( tmp_path, monkeypatch, pytester ) # Namespace packages must not have an __init__.py at its top-level - # directory; if it does, it is no longer a namespace package and we fall back + # directory; if it does, it is no longer a namespace package, and we fall back # to importing just the part of the package containing the __init__.py files. (tmp_path / "src/dist1/com/__init__.py").touch() - # Ensure Python no longer considers dist1/com a namespace package. + # Because of the __init__ file, 'com' is no longer a namespace package: + # 'com.company.app' is importable as a normal module. + # 'com.company.calc' is no longer importable because 'com' is not a namespace package anymore. r = validate_namespace_package( pytester, [tmp_path / "src/dist1", tmp_path / "src/dist2"], @@ -1265,8 +1266,6 @@ def test_incorrect_namespace_package( assert r.ret == 1 r.stderr.fnmatch_lines("*No module named 'com.company.calc*") - # dist1 is not a namespace package, but its module is importable; not being a namespace - # package prevents "com.company.calc" from being importable. pkg_root, module_name = resolve_pkg_root_and_module_name( models_py, consider_namespace_packages=True ) @@ -1342,30 +1341,6 @@ def find_spec( "com.company.app.core.models", ) - def test_is_importable_bad_arguments(self, pytester: Pytester) -> None: - pytester.syspathinsert() - path = pytester.path / "bar.x" - path.mkdir() - assert is_importable(path.parent, path) is False - - path = pytester.path / ".bar.x" - path.mkdir() - assert is_importable(path.parent, path) is False - - def test_compute_module_name(self, tmp_path: Path) -> None: - assert compute_module_name(tmp_path, tmp_path) == "" - assert compute_module_name(Path(), Path()) == "" - - assert compute_module_name(tmp_path, tmp_path / "mod.py") == "mod" - assert compute_module_name(tmp_path, tmp_path / "src/app/bar") == "src.app.bar" - assert ( - compute_module_name(tmp_path, tmp_path / "src/app/bar.py") == "src.app.bar" - ) - assert ( - compute_module_name(tmp_path, tmp_path / "src/app/bar/__init__.py") - == "src.app.bar" - ) - @pytest.mark.parametrize("insert", [True, False]) def test_full_ns_packages_without_init_files( self, pytester: Pytester, tmp_path: Path, monkeypatch: MonkeyPatch, insert: bool @@ -1398,6 +1373,30 @@ def test_full_ns_packages_without_init_files( ) == (tmp_path / "src/dist2", "ns.a.core.foo.m") +def test_is_importable_bad_arguments(pytester: Pytester) -> None: + pytester.syspathinsert() + path = pytester.path / "bar.x" + path.mkdir() + assert is_importable(path.parent, path) is False + + path = pytester.path / ".bar.x" + path.mkdir() + assert is_importable(path.parent, path) is False + + +def test_compute_module_name(tmp_path: Path) -> None: + assert compute_module_name(tmp_path, tmp_path) == "" + assert compute_module_name(Path(), Path()) == "" + + assert compute_module_name(tmp_path, tmp_path / "mod.py") == "mod" + assert compute_module_name(tmp_path, tmp_path / "src/app/bar") == "src.app.bar" + assert compute_module_name(tmp_path, tmp_path / "src/app/bar.py") == "src.app.bar" + assert ( + compute_module_name(tmp_path, tmp_path / "src/app/bar/__init__.py") + == "src.app.bar" + ) + + def validate_namespace_package( pytester: Pytester, paths: Sequence[Path], modules: Sequence[str] ) -> RunResult: From b6c0190f15df3e85fdc1ac3e3cad0bd0caeefcf4 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Apr 2024 18:41:40 -0300 Subject: [PATCH 15/18] Refactor is_importable and improve docs --- src/_pytest/pathlib.py | 25 +++++++++++++++++-------- testing/test_pathlib.py | 4 ++-- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index ff12eb453d9..b1768682fcc 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -788,7 +788,8 @@ def resolve_pkg_root_and_module_name( if consider_namespace_packages: start = pkg_root if pkg_root is not None else path.parent for candidate in (start, *start.parents): - if is_importable(candidate, path): + module_name = compute_module_name(candidate, path) + if is_importable(module_name, path): # Point the pkg_root to the root of the namespace package. pkg_root = candidate break @@ -800,17 +801,25 @@ def resolve_pkg_root_and_module_name( raise CouldNotResolvePathError(f"Could not resolve for {path}") -def is_importable(root: Path, module_path: Path) -> bool: +def is_importable(module_name: str, module_path: Path) -> bool: """ Return if the given module path could be imported normally by Python, akin to the user - entering the REPL and importing the corresponding module name directly. + entering the REPL and importing the corresponding module name directly, and corresponds + to the module_path specified. + + :param module_name: + Full module name that we want to check if is importable. + For example, "app.models". + + :param module_path: + Full path to the python module/package we want to check if is importable. + For example, "/projects/src/app/models.py". """ - module_name = compute_module_name(root, module_path) try: - # Note this is different from what we do in ``import_path``, where we also search sys.meta_path. - # Searching sys.meta_path will eventually find a spec which can load the file even if the interpreter would - # not find this module normally in the REPL, which is exactly what we want to be able to do in - # ``import_path``, but not here. + # Note this is different from what we do in ``_import_module_using_spec``, where we explicitly search through + # sys.meta_path to be able to pass the path of the module that we want to import (``meta_importer.find_spec``). + # Using importlib.util.find_spec() is different, it gives the same results as trying to import + # the module normally in the REPL. spec = importlib.util.find_spec(module_name) except (ImportError, ValueError, ImportWarning): return False diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index 08395faa831..dcfd109708e 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1377,11 +1377,11 @@ def test_is_importable_bad_arguments(pytester: Pytester) -> None: pytester.syspathinsert() path = pytester.path / "bar.x" path.mkdir() - assert is_importable(path.parent, path) is False + assert is_importable("bar.x", path) is False path = pytester.path / ".bar.x" path.mkdir() - assert is_importable(path.parent, path) is False + assert is_importable(".bar.x", path) is False def test_compute_module_name(tmp_path: Path) -> None: From 451a12c451200dbd0ed12e17a127d23bb51c5145 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Apr 2024 18:49:02 -0300 Subject: [PATCH 16/18] Improve is_importable tests --- testing/test_pathlib.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index dcfd109708e..dbdc03b5029 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1373,12 +1373,30 @@ def test_full_ns_packages_without_init_files( ) == (tmp_path / "src/dist2", "ns.a.core.foo.m") -def test_is_importable_bad_arguments(pytester: Pytester) -> None: +def test_is_importable(pytester: Pytester) -> None: pytester.syspathinsert() + + # Module package. + path = pytester.path / "is_importable/foo.py" + path.parent.mkdir() + path.touch() + assert is_importable("is_importable.foo", path) is True + + # Standalone module. + path = pytester.path / "is_importable_module.py" + path.touch() + assert is_importable("is_importable_module", path) is True + + # Ensure that the module that can be imported points to the path we expect. + path = pytester.path / "some/other/path/is_importable_module.py" + assert is_importable("is_importable_module", path) is False + + # Paths containing "." cannot be imported. path = pytester.path / "bar.x" path.mkdir() assert is_importable("bar.x", path) is False + # Pass starting with "." denote relative imports and cannot be checked using is_importable. path = pytester.path / ".bar.x" path.mkdir() assert is_importable(".bar.x", path) is False From 981e3996fb0c3be2f68666cd188e10b28fcf7004 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 7 Apr 2024 08:45:07 -0300 Subject: [PATCH 17/18] Change test_is_importable Removed the test for standalone module because for some reason it was flaky (could not figure why). --- testing/test_pathlib.py | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index dbdc03b5029..2f7d656b349 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1376,29 +1376,26 @@ def test_full_ns_packages_without_init_files( def test_is_importable(pytester: Pytester) -> None: pytester.syspathinsert() - # Module package. - path = pytester.path / "is_importable/foo.py" + path = pytester.path / "bar/foo.py" path.parent.mkdir() path.touch() - assert is_importable("is_importable.foo", path) is True - - # Standalone module. - path = pytester.path / "is_importable_module.py" - path.touch() - assert is_importable("is_importable_module", path) is True + assert is_importable("bar.foo", path) is True # Ensure that the module that can be imported points to the path we expect. - path = pytester.path / "some/other/path/is_importable_module.py" - assert is_importable("is_importable_module", path) is False + path = pytester.path / "some/other/path/bar/foo.py" + path.mkdir(parents=True, exist_ok=True) + assert is_importable("bar.foo", path) is False # Paths containing "." cannot be imported. - path = pytester.path / "bar.x" - path.mkdir() + path = pytester.path / "bar.x/__init__.py" + path.parent.mkdir() + path.touch() assert is_importable("bar.x", path) is False # Pass starting with "." denote relative imports and cannot be checked using is_importable. - path = pytester.path / ".bar.x" - path.mkdir() + path = pytester.path / ".bar.x/__init__.py" + path.parent.mkdir() + path.touch() assert is_importable(".bar.x", path) is False From 1e22e0f7058317cd7ef823d13392403a541d6c65 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 7 Apr 2024 09:19:00 -0300 Subject: [PATCH 18/18] Change compute_module_name to return Optional This is better to enforce callers to check for it instead of ending up with '' and possibly breaking later. --- src/_pytest/pathlib.py | 19 +++++++++++++------ testing/test_pathlib.py | 4 ++-- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index b1768682fcc..254d9d9468e 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -789,14 +789,15 @@ def resolve_pkg_root_and_module_name( start = pkg_root if pkg_root is not None else path.parent for candidate in (start, *start.parents): module_name = compute_module_name(candidate, path) - if is_importable(module_name, path): + if module_name and is_importable(module_name, path): # Point the pkg_root to the root of the namespace package. pkg_root = candidate break if pkg_root is not None: module_name = compute_module_name(pkg_root, path) - return pkg_root, module_name + if module_name: + return pkg_root, module_name raise CouldNotResolvePathError(f"Could not resolve for {path}") @@ -827,16 +828,22 @@ def is_importable(module_name: str, module_path: Path) -> bool: return spec_matches_module_path(spec, module_path) -def compute_module_name(root: Path, module_path: Path) -> str: +def compute_module_name(root: Path, module_path: Path) -> Optional[str]: """Compute a module name based on a path and a root anchor.""" try: path_without_suffix = module_path.with_suffix("") except ValueError: # Empty paths (such as Path.cwd()) might break meta_path hooks (like our own assertion rewriter). - return "" + return None - names = list(path_without_suffix.relative_to(root).parts) - if names and names[-1] == "__init__": + try: + relative = path_without_suffix.relative_to(root) + except ValueError: # pragma: no cover + return None + names = list(relative.parts) + if not names: + return None + if names[-1] == "__init__": names.pop() return ".".join(names) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index 2f7d656b349..f96151bdd44 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1400,8 +1400,8 @@ def test_is_importable(pytester: Pytester) -> None: def test_compute_module_name(tmp_path: Path) -> None: - assert compute_module_name(tmp_path, tmp_path) == "" - assert compute_module_name(Path(), Path()) == "" + assert compute_module_name(tmp_path, tmp_path) is None + assert compute_module_name(Path(), Path()) is None assert compute_module_name(tmp_path, tmp_path / "mod.py") == "mod" assert compute_module_name(tmp_path, tmp_path / "src/app/bar") == "src.app.bar"