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

[BLOCKED] --import-mode=importlib: use sys.path relative path if present (wait for #7870) #7936

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions src/_pytest/pathlib.py
Original file line number Diff line number Diff line change
@@ -468,13 +468,19 @@ def import_path(
if mode is ImportMode.importlib:
module_name = path.stem

# If the module exists under a location in sys.path, just import it using that module name.
# This is as close to a "normal import" as possible - no need to modify sys.path and the
# module will have __package__ set correctly (so relative imports also work).
resolved_name = resolve_sys_path_module_name(path)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment above here, explaining with this code block is needed? IIUC, what it does is "If the path is importable as a name from the search path, then import it as such.". But the comment should say why it's done.

Also, AFAIK (not 100% sure) the previous importlib code doesn't insert the imported module into sys.modules, but importlib.import_module does. I think the idea was to not insert it, as mentioned in the --import-mode=importlib changelog entry:

Traditionally pytest used __import__ while changing sys.path to import test modules (which also changes sys.modules as a side-effect), which works but has a number of drawbacks, like requiring test modules that don't live in packages to have unique names (as they need to reside under a unique name in sys.modules).

--import-mode=importlib uses more fine grained import mechanisms from importlib which don't require pytest to change sys.path or sys.modules at all, eliminating much of the drawbacks of the previous mode.

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing, very happy to add a comment. I'll push it as a separate commit.

Also, AFAIK (not 100% sure) the previous importlib code doesn't insert the imported module into sys.modules, but importlib.import_module does.

This is true. I noticed that #7870 was actually proposing to change that though so that the imported module is added to sys.modules (to fix the issues noted there).

An interesting thing to note about this approach is that the entry that ends up in sys.modules will be the same as if import foo.bar.test_baz was executed - e.g. sys.modules["foo.bar.test_baz"].

With the approach in #7870 the concern is that test modules which have conflicting names will again have issues, as described in the changelog entry you quote. (The code in #7870 would add just test_baz to sys.modules, for example.)

This approach would add the fully qualified name foo.bar.test_baz to sys.modules, which may be interesting as an alternative approach to the fix in #7870.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation.

So the approach does sounds appealing, but it does bring us again to have a dependence on sys.path and how it is configured (e.g. if it contains the test directory, at what depth, etc.). Basically it becomes pretty similar to --import-mode=append, no? Just without keeping the sys.path dirty.

Copy link
Author

@davidhewitt davidhewitt Oct 27, 2020

Choose a reason for hiding this comment

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

Yes, that's a very fair assessment of this approach.

Given that it's perhaps markedly different from the importlib mode, perhaps it makes more sense to make this a new mode, maybe called path?

That way it'll have reliable behaviour - if the test file is not underneath a path entry, pytest will not be able to import it. So users who want to test using this mode will have to add the test files to their path (usually by setup.py develop).

The advantages of it are primarily for test files inside packages, so I don't think that's an unreasonable requirement.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @davidhewitt for the patch! This is complicated topic so we are very grateful for the attempt.

This is an interesting approach, but doesn't handle one of the main uses of --importmode=importlib, which is using it to collect test files which are not in sys.path at all. Consider this common layout:

src/
   analysis/

tests/
   io/
      test_measurement.py
   model/
      test_measurment.py 

In this case src is added to sys.path due to python setup.py develop (or adding to $PYTHONPATH manually), however we don't have any parent of tests/io/test_measurement.py and tests/model/test_measurement.py in sys.path, which wouldn't trigger your patch and would still cause the issues with pickle/dataclasses mentioned in the original issue.

Your patch would work fine though when the tests are siblings of an entry in sys.path, which is common for "in-source" test layouts:

src/
   analysis/
     /io
       /tests
         test_measurement.py
     /model
       /tests    
         test_measurment.py 

But again this is not one of the use cases that importlib mode attempts to solve.

Given that it's perhaps markedly different from the importlib mode, perhaps it makes more sense to make this a new mode, maybe called path?

That's sensible, but I'm not sure it is the best approach. Supposing we can fix #7870, what would be the advantage of path over importlib?

Copy link
Author

Choose a reason for hiding this comment

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

As well as #7870 I have a separate issue with importlib mode, which is that relative imports no longer work in importlib mode. This patch originated as a attempt to figure out how to get __package__ correctly set as suggested at #7245 (comment).

I'm happy to close this PR if it doesn't look like the right solution.

Copy link
Member

Choose a reason for hiding this comment

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

@davidhewitt again thanks for the patch!

Let's keep this open until we get #7870 working (even if only as a reference): we can then see if it also solves the relative import problem.

Copy link
Author

Choose a reason for hiding this comment

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

👍

Note that the test case I've written here is particularly awkward but is amazingly also a real-world use case I'm suffering with which motivated me to write the PR.

I'm attempting a relative import from a test file that is inside a pep 420 namespace package, where the first non-implicit package is called csv, and so collides with the builtin package of the same name.

Interestingly the append mode also cannot handle this case correctly, so I was wondering if this patch could also be relevant for the append mode.

if resolved_name is not None:
return importlib.import_module(resolved_name)

for meta_importer in sys.meta_path:
spec = meta_importer.find_spec(module_name, [str(path.parent)])
if spec is not None:
break
else:
spec = importlib.util.spec_from_file_location(module_name, str(path))

if spec is None:
raise ImportError(
"Can't find module {} at location {}".format(module_name, str(path))
@@ -486,10 +492,7 @@ def import_path(
pkg_path = resolve_package_path(path)
if pkg_path is not None:
pkg_root = pkg_path.parent
names = list(path.with_suffix("").relative_to(pkg_root).parts)
if names[-1] == "__init__":
names.pop()
module_name = ".".join(names)
module_name = module_name_from_relative_path(pkg_root, path)
else:
pkg_root = path.parent
module_name = path.stem
@@ -531,6 +534,34 @@ def import_path(
return mod


def resolve_sys_path_module_name(path: Path) -> Optional[str]:
"""Return the Python module name of path by looking in sys.path
for the first base path encompassing the target.

Returns None if it can not be determined.
"""
path = path.absolute()
for entry in sys.path:
try:
return module_name_from_relative_path(Path(entry).absolute(), path)
except ValueError:
pass
return None


def module_name_from_relative_path(base: Path, path: Path) -> str:
"""Return the Python module name of path as it would be imported
from base.

:raises ValueError:
If path is not a subpath of base.
"""
names = list(path.with_suffix("").relative_to(base).parts)
if names[-1] == "__init__":
names.pop()
return ".".join(names)


def resolve_package_path(path: Path) -> Optional[Path]:
"""Return the Python package path by looking for the last
directory upwards which still contains an __init__.py.
31 changes: 31 additions & 0 deletions testing/test_pathlib.py
Original file line number Diff line number Diff line change
@@ -295,6 +295,37 @@ def test_no_meta_path_found(self, simple_module, monkeypatch):
with pytest.raises(ImportError):
import_path(simple_module, mode="importlib")

@pytest.fixture
def implicit_namespace_module(self, tmpdir):
# Implicit namespace with a child package which clashes with
# the name of the builtin "csv" package.
p2 = tmpdir.mkdir("namespace").mkdir("csv")
p2.join("__init__.py")
fn = p2.join("tests.py")
fn.write(
dedent(
"""
# If __package__ is not set during import, this relative
# import will fail
from . import *

def foo(x): return 40 + x
"""
)
)
return fn

def test_importmode_importlib_implicit_namespace(
self, tmpdir, implicit_namespace_module
):
"""`importlib` mode can infer package from sys.path, which
works even for PEP 420 namespace modules."""
sys.path.insert(1, str(tmpdir))
module = import_path(implicit_namespace_module, mode="importlib")
sys.path.pop(1)
assert module.foo(2) == 42 # type: ignore[attr-defined]
assert module.__package__ == "namespace.csv"


def test_resolve_package_path(tmp_path):
pkg = tmp_path / "pkg1"