From 70dd4b08806436d0e93850a7faf8a0e751241781 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Fri, 12 Apr 2024 15:53:31 -0300 Subject: [PATCH 1/7] importlib: set module as attribute in its parent module This needs to be done manually, not being done as part of the import mechanism: https://github.com/python/cpython/blob/73906d5c908c1e0b73c5436faeff7d93698fc074/Lib/importlib/_bootstrap.py#L1335-L1342 Fix #12194 --- changelog/12194.bugfix.rst | 1 + src/_pytest/pathlib.py | 15 +++++++++++++++ testing/test_pathlib.py | 25 +++++++++++++++++++++++++ 3 files changed, 41 insertions(+) create mode 100644 changelog/12194.bugfix.rst diff --git a/changelog/12194.bugfix.rst b/changelog/12194.bugfix.rst new file mode 100644 index 00000000000..6983ba35a90 --- /dev/null +++ b/changelog/12194.bugfix.rst @@ -0,0 +1 @@ +Fixed a bug with ``--importmode=importlib`` and ``--doctest-modules`` where child modules did not appear as attributes in parent modules. diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 254d9d9468e..3cdaa0b6ec2 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -641,11 +641,26 @@ def _import_module_using_spec( spec.loader.exec_module(mod) # type: ignore[union-attr] if insert_modules: insert_missing_modules(sys.modules, module_name) + _set_name_in_parent(mod) return mod return None +def _set_name_in_parent(module: ModuleType) -> None: + """ + Sets an attribute in the module's parent pointing to the module itself (#12194). + + Based on https://github.com/python/cpython/blob/73906d5c908c1e0b73c5436faeff7d93698fc074/Lib/importlib/_bootstrap.py#L1335-L1342. + """ + parent, _, name = module.__name__.rpartition(".") + if not parent: + return + parent_module = sys.modules.get(parent) + if parent_module is not None: + setattr(sys.modules[parent], name, module) + + def spec_matches_module_path( module_spec: Optional[ModuleSpec], module_path: Path ) -> bool: diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index f96151bdd44..dbd93147437 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1126,6 +1126,31 @@ def test_safe_exists(tmp_path: Path) -> None: assert safe_exists(p) is False +def test_import_sets_module_as_attribute(pytester: Pytester) -> None: + """Regression test for #12194.""" + pytester.path.joinpath("foo/bar/baz").mkdir(parents=True) + pytester.path.joinpath("foo/__init__.py").touch() + pytester.path.joinpath("foo/bar/__init__.py").touch() + pytester.path.joinpath("foo/bar/baz/__init__.py").touch() + f = pytester.makepyfile( + """ + import foo + from foo.bar import baz + foo.bar.baz + + def test_foo() -> None: + pass + """ + ) + + pytester.syspathinsert() + result = pytester.runpython(f) + assert result.ret == 0 + + result = pytester.runpytest("--import-mode=importlib", "--doctest-modules") + assert result.ret == 0 + + class TestNamespacePackages: """Test import_path support when importing from properly namespace packages.""" From f4289181cb0ed651c8778eb247a7c15b1f3b20ad Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 13 Apr 2024 11:39:54 -0300 Subject: [PATCH 2/7] importlib: _import_module_using_spec also imports parent modules According to the Python import implementation: https://github.com/python/cpython/blob/73906d5c908c1e0b73c5436faeff7d93698fc074/Lib/importlib/_bootstrap.py#L1308-L1311 When we import a module we should also import its parents, and set the child as attribute of its parent. --- src/_pytest/pathlib.py | 29 ++++++++++++++--------------- testing/test_pathlib.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 15 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 3cdaa0b6ec2..e1a336719ee 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -636,31 +636,30 @@ def _import_module_using_spec( if spec_matches_module_path(spec, module_path): assert spec is not None + # Attempt to import the parent module, seems is our responsibility: + # https://github.com/python/cpython/blob/73906d5c908c1e0b73c5436faeff7d93698fc074/Lib/importlib/_bootstrap.py#L1308-L1311 + parent_module_name, _, name = module_name.rpartition(".") + parent_module: Optional[ModuleType] = sys.modules.get(parent_module_name) + if parent_module is None and parent_module_name: + with contextlib.suppress(ModuleNotFoundError, ImportWarning): + parent_module = importlib.import_module(parent_module_name) + + # Find spec and import this module. mod = importlib.util.module_from_spec(spec) sys.modules[module_name] = mod spec.loader.exec_module(mod) # type: ignore[union-attr] + + # Set this module as an attribute of the parent module (#12194). + if parent_module is not None: + setattr(parent_module, name, mod) + if insert_modules: insert_missing_modules(sys.modules, module_name) - _set_name_in_parent(mod) return mod return None -def _set_name_in_parent(module: ModuleType) -> None: - """ - Sets an attribute in the module's parent pointing to the module itself (#12194). - - Based on https://github.com/python/cpython/blob/73906d5c908c1e0b73c5436faeff7d93698fc074/Lib/importlib/_bootstrap.py#L1335-L1342. - """ - parent, _, name = module.__name__.rpartition(".") - if not parent: - return - parent_module = sys.modules.get(parent) - if parent_module is not None: - setattr(sys.modules[parent], name, module) - - def spec_matches_module_path( module_spec: Optional[ModuleSpec], module_path: Path ) -> bool: diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index dbd93147437..fda03b00369 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1127,6 +1127,41 @@ def test_safe_exists(tmp_path: Path) -> None: def test_import_sets_module_as_attribute(pytester: Pytester) -> None: + """Unittest test for #12194.""" + pytester.path.joinpath("foo/bar/baz").mkdir(parents=True) + pytester.path.joinpath("foo/__init__.py").touch() + pytester.path.joinpath("foo/bar/__init__.py").touch() + pytester.path.joinpath("foo/bar/baz/__init__.py").touch() + pytester.syspathinsert() + + # Import foo.bar.baz and ensure parent modules also ended up imported. + baz = import_path( + pytester.path.joinpath("foo/bar/baz/__init__.py"), + mode=ImportMode.importlib, + root=pytester.path, + consider_namespace_packages=False, + ) + assert baz.__name__ == "foo.bar.baz" + foo = sys.modules["foo"] + assert foo.__name__ == "foo" + bar = sys.modules["foo.bar"] + assert bar.__name__ == "foo.bar" + + # Check parent modules have an attribute pointing to their children. + assert bar.baz is baz + assert foo.bar is bar + + # Ensure we returned the "foo.bar" module cached in sys.modules. + bar_2 = import_path( + pytester.path.joinpath("foo/bar/__init__.py"), + mode=ImportMode.importlib, + root=pytester.path, + consider_namespace_packages=False, + ) + assert bar_2 is bar + + +def test_import_sets_module_as_attribute_regression(pytester: Pytester) -> None: """Regression test for #12194.""" pytester.path.joinpath("foo/bar/baz").mkdir(parents=True) pytester.path.joinpath("foo/__init__.py").touch() From 3c6caf65ebdaa843a8b122e0e2a263618c3317a3 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 13 Apr 2024 12:03:49 -0300 Subject: [PATCH 3/7] Use _import_module_using_spec to import parent modules Seems this is the right thing to do, as we will then also consider the parent modules for rewriting. --- src/_pytest/pathlib.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index e1a336719ee..4cda4501a5b 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -641,8 +641,24 @@ def _import_module_using_spec( parent_module_name, _, name = module_name.rpartition(".") parent_module: Optional[ModuleType] = sys.modules.get(parent_module_name) if parent_module is None and parent_module_name: - with contextlib.suppress(ModuleNotFoundError, ImportWarning): - parent_module = importlib.import_module(parent_module_name) + # Find the directory of this module's parent. + parent_dir = ( + module_path.parent.parent + if module_path.name == "__init__.py" + else module_path.parent + ) + # Consider the parent module path as its __init__.py file, if it has one. + parent_module_path = ( + parent_dir / "__init__.py" + if (parent_dir / "__init__.py").is_file() + else parent_dir + ) + parent_module = _import_module_using_spec( + parent_module_name, + parent_module_path, + parent_dir, + insert_modules=insert_modules, + ) # Find spec and import this module. mod = importlib.util.module_from_spec(spec) From 9dc41a74bd4ed83d965d0999c4dc6778948c18f0 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 13 Apr 2024 12:08:35 -0300 Subject: [PATCH 4/7] Remove outdated comment Seems we need to insert missing modules when the modules are not part of a package or namespace package. --- src/_pytest/pathlib.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 4cda4501a5b..83ad0152fe0 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -620,10 +620,6 @@ def _import_module_using_spec( :param insert_modules: If True, will call insert_missing_modules to create empty intermediate modules for made-up module names (when importing test files not reachable from sys.path). - Note: we can probably drop insert_missing_modules altogether: instead of - generating module names such as "src.tests.test_foo", which require intermediate - empty modules, we might just as well generate unique module names like - "src_tests_test_foo". """ # Checking with sys.meta_path first in case one of its hooks can import this module, # such as our own assertion-rewrite hook. From 91146eacfa54eba036b800114d7fc073636d8b4e Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 13 Apr 2024 12:14:27 -0300 Subject: [PATCH 5/7] Add test without __init__.py files --- testing/test_pathlib.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index fda03b00369..6c79f64c097 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1161,6 +1161,39 @@ def test_import_sets_module_as_attribute(pytester: Pytester) -> None: assert bar_2 is bar +def test_import_sets_module_as_attribute_without_init_files(pytester: Pytester) -> None: + """Similar to test_import_sets_module_as_attribute, but without __init__.py files.""" + pytester.path.joinpath("foo/bar").mkdir(parents=True) + pytester.path.joinpath("foo/bar/baz.py").touch() + pytester.syspathinsert() + + # Import foo.bar.baz and ensure parent modules also ended up imported. + baz = import_path( + pytester.path.joinpath("foo/bar/baz.py"), + mode=ImportMode.importlib, + root=pytester.path, + consider_namespace_packages=False, + ) + assert baz.__name__ == "foo.bar.baz" + foo = sys.modules["foo"] + assert foo.__name__ == "foo" + bar = sys.modules["foo.bar"] + assert bar.__name__ == "foo.bar" + + # Check parent modules have an attribute pointing to their children. + assert bar.baz is baz + assert foo.bar is bar + + # Ensure we returned the "foo.bar.baz" module cached in sys.modules. + baz_2 = import_path( + pytester.path.joinpath("foo/bar/baz.py"), + mode=ImportMode.importlib, + root=pytester.path, + consider_namespace_packages=False, + ) + assert baz_2 is baz + + def test_import_sets_module_as_attribute_regression(pytester: Pytester) -> None: """Regression test for #12194.""" pytester.path.joinpath("foo/bar/baz").mkdir(parents=True) From f62efb993495f8d07797a9d124e746f1b559cc1d Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 20 Apr 2024 08:03:41 -0300 Subject: [PATCH 6/7] review: do not search sys.modules if parent_module_name is empty --- src/_pytest/pathlib.py | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 83ad0152fe0..7f01c011ba6 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -635,26 +635,28 @@ def _import_module_using_spec( # Attempt to import the parent module, seems is our responsibility: # https://github.com/python/cpython/blob/73906d5c908c1e0b73c5436faeff7d93698fc074/Lib/importlib/_bootstrap.py#L1308-L1311 parent_module_name, _, name = module_name.rpartition(".") - parent_module: Optional[ModuleType] = sys.modules.get(parent_module_name) - if parent_module is None and parent_module_name: - # Find the directory of this module's parent. - parent_dir = ( - module_path.parent.parent - if module_path.name == "__init__.py" - else module_path.parent - ) - # Consider the parent module path as its __init__.py file, if it has one. - parent_module_path = ( - parent_dir / "__init__.py" - if (parent_dir / "__init__.py").is_file() - else parent_dir - ) - parent_module = _import_module_using_spec( - parent_module_name, - parent_module_path, - parent_dir, - insert_modules=insert_modules, - ) + parent_module: Optional[ModuleType] = None + if parent_module_name: + parent_module = sys.modules.get(parent_module_name) + if parent_module is None: + # Find the directory of this module's parent. + parent_dir = ( + module_path.parent.parent + if module_path.name == "__init__.py" + else module_path.parent + ) + # Consider the parent module path as its __init__.py file, if it has one. + parent_module_path = ( + parent_dir / "__init__.py" + if (parent_dir / "__init__.py").is_file() + else parent_dir + ) + parent_module = _import_module_using_spec( + parent_module_name, + parent_module_path, + parent_dir, + insert_modules=insert_modules, + ) # Find spec and import this module. mod = importlib.util.module_from_spec(spec) From 9cbbcf7cba5ad2912b205bff3b3d0bf6b9c29fd9 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 20 Apr 2024 08:15:04 -0300 Subject: [PATCH 7/7] Additional test --- testing/test_pathlib.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index 6c79f64c097..688d13f2f05 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -1219,6 +1219,46 @@ def test_foo() -> None: assert result.ret == 0 +def test_import_submodule_not_namespace(pytester: Pytester) -> None: + """ + Regression test for importing a submodule 'foo.bar' while there is a 'bar' directory + reachable from sys.path -- ensuring the top-level module does not end up imported as a namespace + package. + + #12194 + https://github.com/pytest-dev/pytest/pull/12208#issuecomment-2056458432 + """ + pytester.syspathinsert() + # Create package 'foo' with a submodule 'bar'. + pytester.path.joinpath("foo").mkdir() + foo_path = pytester.path.joinpath("foo/__init__.py") + foo_path.touch() + bar_path = pytester.path.joinpath("foo/bar.py") + bar_path.touch() + # Create top-level directory in `sys.path` with the same name as that submodule. + pytester.path.joinpath("bar").mkdir() + + # Import `foo`, then `foo.bar`, and check they were imported from the correct location. + foo = import_path( + foo_path, + mode=ImportMode.importlib, + root=pytester.path, + consider_namespace_packages=False, + ) + bar = import_path( + bar_path, + mode=ImportMode.importlib, + root=pytester.path, + consider_namespace_packages=False, + ) + assert foo.__name__ == "foo" + assert bar.__name__ == "foo.bar" + assert foo.__file__ is not None + assert bar.__file__ is not None + assert Path(foo.__file__) == foo_path + assert Path(bar.__file__) == bar_path + + class TestNamespacePackages: """Test import_path support when importing from properly namespace packages."""