Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use shortest module name for importlib imports #11931

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ Pavel Karateev
Paweł Adamczak
Pedro Algarvio
Petter Strandmark
Philipp A.
Philipp Loose
Pierre Sassoulas
Pieter Mulder
Expand Down
1 change: 1 addition & 0 deletions changelog/11931.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix some instances of importing doctests’ parent modules when using `--import-mode=importlib`.
3 changes: 3 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ ignore = [
[tool.ruff.format]
docstring-code-format = true

[tool.ruff.lint]
allowed-confusables = ["’", "×"]

[tool.ruff.lint.pycodestyle]
# In order to be able to format for 88 char in ruff format
max-line-length = 120
Expand Down
13 changes: 11 additions & 2 deletions src/_pytest/pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -607,11 +607,20 @@ def _is_same(f1: str, f2: str) -> bool:

def module_name_from_path(path: Path, root: Path) -> str:
"""
Return a dotted module name based on the given path, anchored on root.
Return a dotted module name based on the given path,
anchored on root or the most likely entry in `sys.path`.

For example: path="projects/src/tests/test_foo.py" and root="/projects", the
resulting module name will be "src.tests.test_foo".
"""
candidates = (
_module_name_from_path(path, dir)
for dir in itertools.chain([root], map(Path, sys.path))
)
return ".".join(min(candidates, key=len)) # type: ignore[arg-type]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be somewhat slow I reckon. Pathlib is a hog (particularly relative_to) and sys.path can have many entries in some circumstances, and this is running for every import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, I just did the bare functional minimum.

I think this could be improved

  1. maybe by pre-filtering sys.path somehow?
  2. by having a cache that
    1. stores all paths that are ever requested as long as sys.path stays the same,
    2. otherwise the cache gets cleared
  3. using some faster string algorithm for this instead of pathlib.
  4. Maybe there is an alternative to the whole approach, some way to get the correct module name using some API I’m not thinking of

But there’s no alternative to the functionality. The test case should pass in the end.



def _module_name_from_path(path: Path, root: Path) -> "tuple[str, ...]":
flying-sheep marked this conversation as resolved.
Show resolved Hide resolved
path = path.with_suffix("")
try:
relative_path = path.relative_to(root)
Expand All @@ -628,7 +637,7 @@ def module_name_from_path(path: Path, root: Path) -> str:
if len(path_parts) >= 2 and path_parts[-1] == "__init__":
path_parts = path_parts[:-1]

return ".".join(path_parts)
return path_parts


def insert_missing_modules(modules: Dict[str, ModuleType], module_name: str) -> None:
Expand Down
28 changes: 28 additions & 0 deletions testing/test_pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,34 @@ def __init__(self) -> None:
mod = import_path(init, root=tmp_path, mode=ImportMode.importlib)
assert len(mod.instance.INSTANCES) == 1

def test_importlib_doctest(self, monkeypatch: MonkeyPatch, tmp_path: Path):
flying-sheep marked this conversation as resolved.
Show resolved Hide resolved
"""
Importing a package using --importmode=importlib should
import the package using the canonical name
"""
proj_dir = tmp_path / "proj"
proj_dir.mkdir()
pkgs_dir = tmp_path / "pkgs"
pkgs_dir.mkdir()
monkeypatch.chdir(proj_dir)
monkeypatch.syspath_prepend(pkgs_dir)
# this is also there, but shouldn’t be imported from
monkeypatch.syspath_prepend(proj_dir)

package_name = "importlib_doctest"
# pkgs_dir is second to set `init`
for directory in [proj_dir / "src", pkgs_dir]:
pkgdir = directory / package_name
pkgdir.mkdir(parents=True)
init = pkgdir / "__init__.py"
init.write_text("", encoding="ascii")

# the PyTest root is `proj_dir`, but the package is imported from `pkgs_dir`
mod = import_path(init, root=proj_dir, mode=ImportMode.importlib)
# assert that it’s imported with the canonical name, not “path.to.package.<name>”
mod_names = [n for n, m in sys.modules.items() if m is mod]
assert mod_names == ["importlib_doctest"]

def test_importlib_root_is_package(self, pytester: Pytester) -> None:
"""
Regression for importing a `__init__`.py file that is at the root
Expand Down