Introduce --import-mode=importlib #7246

nicoddemus · 2020-05-23T14:21:23Z

This introduces --import-mode=importlib, which uses fine-grained facilities
from importlib to import test modules and conftest files, bypassing
the need to change sys.path and sys.modules as side-effect of that.

I've also opened #7245 to gather feedback on the new import mode.

Fix #5821

nicoddemus · 2020-05-23T14:24:40Z

Unfortunately importing using importlib is bypassing our import hook (I updated a test specifically for that, which is failing).

Here's the code for pyimport which uses importlib:

https://github.com/pytest-dev/py/blob/f7b20d2cc71c96f2d0daba6ad2e9e83c49f60f1b/py/_path/local.py#L670-L685

Our own AssertionRewritingHook implements importlib.abc.MetaPathFinder and importlib.abc.Loader, so far as I understand things should "just work", but AssertionRewritingHook.find_spec is never called when using importlib to import test modules and conftest files.

@asottile, which is our in-house importlib expert, do you have any thoughts/hints here?

testing/acceptance_test.py

asottile · 2020-05-23T19:06:41Z

the difference in assertion rewriting comes down to the call to importlib.util.spec_from_file_location -- pylib is calling this with the default loader (None) which'll always result in SourceFileLoader (avoiding sys.meta_path) -- what we want is for that to pass the loader= argument with our assertion rewriter. Here's an example from a debugging session where I used pytest --import-mode=importlib t.py. Notice how I was able to retrieve the pytest rewriter from sys.meta_path[0] and construct a module spec with the right loader.

(Pdb) importlib.util.spec_from_file_location(modname, str(self))
ModuleSpec(name='t', loader=<_frozen_importlib_external.SourceFileLoader object at 0x7f94e6790ac0>, origin='/tmp/pytest/t.py')
(Pdb) importlib.util.spec_from_file_location(modname, str(self), loader=sys.meta_path[0])
ModuleSpec(name='t', loader=<_pytest.assertion.rewrite.AssertionRewritingHook object at 0x7f94e7023040>, origin='/tmp/pytest/t.py')

bluetech

The changes plumb the importmode into py's pyimport. I didn't dig into what this does, but from the added docs it certainty sounds good. And since it's not on by default, there isn't any risk to experiment with it. So LGTM.

bluetech · 2020-05-24T13:09:29Z

src/_pytest/config/__init__.py

@@ -512,7 +512,7 @@ def _importconftest(self, conftestpath):
            _ensure_removed_sysmodule(conftestpath.purebasename)

        try:
-            mod = conftestpath.pyimport()
+            mod = conftestpath.pyimport(ensuresyspath=importmode)


One question about this: previously this pyimport always used the default, even if the importmode was append (non-default). Now also append is plumbed into here, which changes the existing behavior. Does that make any difference?

It might change things indeed, but I'm inclined that this was actually an oversight.

What do you think @RonnyPfannschmidt?

nicoddemus · 2020-05-30T17:23:58Z

@asottile:

what we want is for that to pass the loader= argument with our assertion rewriter.

Thanks, I was suspecting that.

But passing our assertion rewriter as loader won't interfere with other import hooks installed in sys.meta_path? I'm actually surprised importlib doesn't take in account sys.meta_path here at all.

If I'm right, should we explicitly loop over sys.meta_path instead?

asottile · 2020-05-30T17:42:19Z

@asottile:

what we want is for that to pass the loader= argument with our assertion rewriter.

Thanks, I was suspecting that.

But passing our assertion rewriter as loader won't interfere with other import hooks installed in sys.meta_path? I'm actually surprised importlib doesn't take in account sys.meta_path here at all.

If I'm right, should we explicitly loop over sys.meta_path instead?

I'll have to think about this some more 🤔 but I suspect you're right that we'd need to implement the sys.meta_path lookup ourselves yeah

nicoddemus · 2020-05-30T18:08:31Z

I suspect you're right that we'd need to implement the sys.meta_path lookup ourselves yeah

How does this look like to you?

import importlib.util
import sys


def test_foo():
    print()
    modname = "test_assert_foo"
    path = "test_assert_foo.py"
    for meta_hook in sys.meta_path:
        print('meta_hook=', meta_hook)
        x = meta_hook.find_module(modname, path)
        if x is not None:
            print('x=', x)
            spec = importlib.util.spec_from_file_location(modname, path, loader=meta_hook)
            print('spec=', spec)
            break

nicoddemus · 2020-05-31T14:11:01Z

So I went ahead and my sys.meta_path handling above seems to work.

But I would need to change py.path.local.pyimport, but instead of doing that, decided to go ahead and copy it over to pytest as _pytest.pathlib.import_module. This is aligned with our desired to eventually remove py as a dependency, so it would have to be made eventually so I decided I might as well do it now. The tests for it can be improved in style and new tests should be added too, but I thought it better to first gather feedback here.

I suggest reviewing the individual commits, as there are a lot of changes I'm afraid.

src/_pytest/pathlib.py

bluetech

I get a couple of failures running pytest's own tests with --import-mode=importlib:

FAILED testing/code/test_source.py::test_getfslineno - TypeError: <class 'test_source.test_getfslineno.<locals>.A'> is a built-in class
FAILED testing/test_assertrewrite.py::test_rewrite_infinite_recursion - assert None is not None

doc/en/pythonpath.rst

src/_pytest/pathlib.py

asottile · 2020-05-31T19:40:19Z

I suspect you're right that we'd need to implement the sys.meta_path lookup ourselves yeah

How does this look like to you?

import importlib.util
import sys


def test_foo():
    print()
    modname = "test_assert_foo"
    path = "test_assert_foo.py"
    for meta_hook in sys.meta_path:
        print('meta_hook=', meta_hook)
        x = meta_hook.find_module(modname, path)
        if x is not None:
            print('x=', x)
            spec = importlib.util.spec_from_file_location(modname, path, loader=meta_hook)
            print('spec=', spec)
            break

hmmm actually, if we're importing a test we always want to use the pytest loader right? then we don't need to search through sys.meta_path

RonnyPfannschmidt

im a bit onflicted on this one, the changes needed to port to pathlib are bi enough to warrnt a folloup instead of a "now"

src/_pytest/pathlib.py

nicoddemus · 2020-05-31T22:18:53Z

hmmm actually, if we're importing a test we always want to use the pytest loader right? then we don't need to search through sys.meta_path

Not sure TBH... in append and prepend modes, other loaders are being considered by the high-level mechanism (AFAIU), so I don't know the consequences of ignoring them here. For example, I can think of a loader which adds itself to sys.meta_path[0] after pytest, but then uses pytest's loader to get assertion rewriting, and then does something else with the module. Plus seems "the right thing to do".

I might be misunderstanding how importlib works though.

I get a couple of failures running pytest's own tests with --import-mode=importlib

Ahh that's a great idea, I should have done that already, thanks. I will take a look at those later.

Thanks about all the other comments regarding the import_module. I did notice many of the same problems mentioned while importing pyimport, but decided to postpone doing them until I got feedback that importing that function into pytest was the way to go.

So I understand I will continue on that route. 👍

im a bit onflicted on this one, the changes needed to port to pathlib are bi enough to warrnt a folloup instead of a "now"

I'm OK with doing them in this PR, actually. But of course I can do a follow up right after merging this if you folks prefer.

nicoddemus · 2020-06-07T16:29:23Z

Did some cleanups/refactorings:

Removed import_module's modname parameter as pytest itself doesn't use it.
Ported ImportMismatchError (as ImportPathMismatchError to avoid confusion)
Use an enum ImportMode instead of magic ensuresyspath=True/False/"importlib".
Added more tests, including for mode=ImportMode.importlib.
Ported pypkpath from py (as resolve_package_path)
import_module now uses Path internally;

bluetech · 2020-06-07T17:36:52Z

src/_pytest/pathlib.py

+        if str(pkgroot) not in sys.path:
+            sys.path.append(str(pkgroot))
+    else:
+        assert mode == ImportMode.prepend, "unexpected mode: {}".format(mode)


There's a nicer way to handle this, if you feel inclined: python/mypy#5818 (works for enums too, though might need to compare the enum values using is, IIRC).

Seems nice; I did implement it locally but I get:

src\_pytest\pathlib.py:495: error: Argument 1 to "assert_never" has incompatible type "ImportMode"; expected "NoReturn" [arg-type] Found 1 error in 1 file (checked 2 source files)

I implemented the solution from your original post verbatim, didn't really read the whole thread though.

Here's a diff that works for me. (Again, it's ok if you prefer not to apply this)

diff --git a/src/_pytest/compat.py b/src/_pytest/compat.py index 84f9609a7..c19beb505 100644 --- a/src/_pytest/compat.py +++ b/src/_pytest/compat.py @@ -33,6 +33,7 @@ else: if TYPE_CHECKING: + from typing import NoReturn from typing import Type from typing_extensions import Final @@ -401,3 +402,38 @@ else: from collections import OrderedDict order_preserving_dict = OrderedDict + + +# Perform exhaustiveness checking. +# +# Consider this example: +# +# MyUnion = Union[int, str] +# +# def handle(x: MyUnion) -> int { +# if isinstance(x, int): +# return 1 +# elif isinstance(x, str): +# return 2 +# else: +# raise Exception('unreachable') +# +# Now suppose we add a new variant: +# +# MyUnion = Union[int, str, bytes] +# +# After doing this, we must remember ourselves to go and update the handle +# function to handle the new variant. +# +# With `assert_never` we can do better: +# +# // throw new Error('unreachable'); +# return assert_never(x) +# +# Now, if we forget to handle the new variant, the type-checker will emit a +# compile-time error, instead of the runtime error we would have gotten +# previously. +# +# This also work for Enums (if you use `is` to compare) and Literals. +def assert_never(value: NoReturn) -> NoReturn: + assert False, "Unhandled value: {} ({})".format(value, type(value).__name__) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 907e9223d..da7f0221c 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -23,6 +23,7 @@ from typing import Union import py +from _pytest.compat import assert_never from _pytest.warning_types import PytestWarning if sys.version_info[:2] >= (3, 6): @@ -453,7 +454,7 @@ def import_path( if not path.exists(): raise ImportError(path) - if mode == ImportMode.importlib: + if mode is ImportMode.importlib: import importlib.util module_name = path.stem @@ -484,13 +485,14 @@ def import_path( pkg_root = path.parent module_name = path.stem - if mode == ImportMode.append: + if mode is ImportMode.append: if str(pkg_root) not in sys.path: sys.path.append(str(pkg_root)) - else: - assert mode == ImportMode.prepend, "unexpected mode: {}".format(mode) + elif mode is ImportMode.prepend: if str(pkg_root) != sys.path[0]: sys.path.insert(0, str(pkg_root)) + else: + assert_never(mode) __import__(module_name)

doc/en/pythonpath.rst

src/_pytest/pathlib.py

bluetech · 2020-06-08T07:36:08Z

src/_pytest/pathlib.py

+        if str(pkgroot) not in sys.path:
+            sys.path.append(str(pkgroot))
+    else:
+        assert mode == ImportMode.prepend, "unexpected mode: {}".format(mode)


Here's a diff that works for me. (Again, it's ok if you prefer not to apply this)

diff --git a/src/_pytest/compat.py b/src/_pytest/compat.py index 84f9609a7..c19beb505 100644 --- a/src/_pytest/compat.py +++ b/src/_pytest/compat.py @@ -33,6 +33,7 @@ else: if TYPE_CHECKING: + from typing import NoReturn from typing import Type from typing_extensions import Final @@ -401,3 +402,38 @@ else: from collections import OrderedDict order_preserving_dict = OrderedDict + + +# Perform exhaustiveness checking. +# +# Consider this example: +# +# MyUnion = Union[int, str] +# +# def handle(x: MyUnion) -> int { +# if isinstance(x, int): +# return 1 +# elif isinstance(x, str): +# return 2 +# else: +# raise Exception('unreachable') +# +# Now suppose we add a new variant: +# +# MyUnion = Union[int, str, bytes] +# +# After doing this, we must remember ourselves to go and update the handle +# function to handle the new variant. +# +# With `assert_never` we can do better: +# +# // throw new Error('unreachable'); +# return assert_never(x) +# +# Now, if we forget to handle the new variant, the type-checker will emit a +# compile-time error, instead of the runtime error we would have gotten +# previously. +# +# This also work for Enums (if you use `is` to compare) and Literals. +def assert_never(value: NoReturn) -> NoReturn: + assert False, "Unhandled value: {} ({})".format(value, type(value).__name__) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 907e9223d..da7f0221c 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -23,6 +23,7 @@ from typing import Union import py +from _pytest.compat import assert_never from _pytest.warning_types import PytestWarning if sys.version_info[:2] >= (3, 6): @@ -453,7 +454,7 @@ def import_path( if not path.exists(): raise ImportError(path) - if mode == ImportMode.importlib: + if mode is ImportMode.importlib: import importlib.util module_name = path.stem @@ -484,13 +485,14 @@ def import_path( pkg_root = path.parent module_name = path.stem - if mode == ImportMode.append: + if mode is ImportMode.append: if str(pkg_root) not in sys.path: sys.path.append(str(pkg_root)) - else: - assert mode == ImportMode.prepend, "unexpected mode: {}".format(mode) + elif mode is ImportMode.prepend: if str(pkg_root) != sys.path[0]: sys.path.insert(0, str(pkg_root)) + else: + assert_never(mode) __import__(module_name)

nicoddemus · 2020-06-13T14:29:25Z

Thanks everyone! 👍

The initial implementation (in pytest-dev#7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. That proved problematic, so we started adding the imported module to `sys.modules` in pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then pytest-dev#10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix pytest-dev#10811, pytest-dev#10341

The initial implementation (in pytest-dev#7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then pytest-dev#10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix pytest-dev#10811, pytest-dev#10341

The initial implementation (in #7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in #7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then #10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix #10811 Fix #10341

nicoddemus requested review from bluetech, asottile and RonnyPfannschmidt and removed request for bluetech May 23, 2020 14:24

nicoddemus commented May 23, 2020

View reviewed changes

testing/acceptance_test.py Show resolved Hide resolved

RonnyPfannschmidt approved these changes May 23, 2020

View reviewed changes

bluetech approved these changes May 24, 2020

View reviewed changes

bluetech reviewed May 24, 2020

View reviewed changes

nicoddemus commented May 31, 2020

View reviewed changes

src/_pytest/pathlib.py Show resolved Hide resolved

nicoddemus force-pushed the import-mode-import-lib branch from 00f8f52 to 0ad23a7 Compare May 31, 2020 15:48

nicoddemus requested review from RonnyPfannschmidt and bluetech May 31, 2020 15:49

bluetech reviewed May 31, 2020

View reviewed changes

RonnyPfannschmidt reviewed May 31, 2020

View reviewed changes

src/_pytest/pathlib.py Outdated Show resolved Hide resolved

src/_pytest/pathlib.py Outdated Show resolved Hide resolved

nicoddemus force-pushed the import-mode-import-lib branch from 725d3ce to c943991 Compare June 7, 2020 16:50

nicoddemus marked this pull request as ready for review June 7, 2020 17:15

nicoddemus requested review from bluetech and RonnyPfannschmidt June 7, 2020 17:15

bluetech reviewed Jun 7, 2020

View reviewed changes

bluetech approved these changes Jun 8, 2020

View reviewed changes

nicoddemus and others added 18 commits June 13, 2020 11:00

Small improvements in import_module

9c89a6f

Remove 'modname' support from import_module (unused by pytest)

3149417

Use an enum for 'mode' in import_module

c6c9872

Add tests for mode=importlib

a95ba58

Cleanup tests

23f90fd

Port pypkgpath from py

fcd20a5

import_module uses pathlib internally

c6a5d88

Fix tests in py35

f671388

Fix linting (mypy errors)

4b2e725

Fix typo

802a83c

Typo and small code rearrangement

cc4999b

Small adjustments asked during code review

41a056c

Simplify code in import_path a bit

fe428ca

Add comment about never restoring sys.path

dbee6c7

Fix linting after rebase

09bb194

Use importlib also for prepend and append import modes

11185dc

Check ImportMode exhaustively

4c4d0a0

Fix linting

29aa8eb

nicoddemus force-pushed the import-mode-import-lib branch from 7009cad to 29aa8eb Compare June 13, 2020 14:05

nicoddemus merged commit ab6dacf into pytest-dev:master Jun 13, 2020

nicoddemus deleted the import-mode-import-lib branch June 13, 2020 14:29

The-Compiler mentioned this pull request Jul 29, 2020

pytest 6.0.0 raises a lot of errors related to importing #7560

Closed

nicoddemus mentioned this pull request Jun 29, 2023

Fix duplicated imports with importlib mode and doctest-modules #11148

Merged

bluetech mentioned this pull request Feb 17, 2024

Support external tests, with __init__, WITHOUT forcing src/ layout #4714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce --import-mode=importlib #7246

Introduce --import-mode=importlib #7246

nicoddemus commented May 23, 2020 •

edited

Loading

nicoddemus commented May 23, 2020

asottile commented May 23, 2020

bluetech left a comment

bluetech May 24, 2020

nicoddemus May 30, 2020

nicoddemus commented May 30, 2020

asottile commented May 30, 2020

nicoddemus commented May 30, 2020

nicoddemus commented May 31, 2020

bluetech left a comment •

edited

Loading

asottile commented May 31, 2020

RonnyPfannschmidt left a comment

nicoddemus commented May 31, 2020

nicoddemus commented Jun 7, 2020 •

edited

Loading

bluetech Jun 7, 2020

nicoddemus Jun 7, 2020

bluetech Jun 8, 2020

bluetech Jun 8, 2020

nicoddemus commented Jun 13, 2020

Introduce --import-mode=importlib #7246

Introduce --import-mode=importlib #7246

Conversation

nicoddemus commented May 23, 2020 • edited Loading

nicoddemus commented May 23, 2020

asottile commented May 23, 2020

bluetech left a comment

Choose a reason for hiding this comment

bluetech May 24, 2020

Choose a reason for hiding this comment

nicoddemus May 30, 2020

Choose a reason for hiding this comment

nicoddemus commented May 30, 2020

asottile commented May 30, 2020

nicoddemus commented May 30, 2020

nicoddemus commented May 31, 2020

bluetech left a comment • edited Loading

Choose a reason for hiding this comment

asottile commented May 31, 2020

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

nicoddemus commented May 31, 2020

nicoddemus commented Jun 7, 2020 • edited Loading

bluetech Jun 7, 2020

Choose a reason for hiding this comment

nicoddemus Jun 7, 2020

Choose a reason for hiding this comment

bluetech Jun 8, 2020

Choose a reason for hiding this comment

bluetech Jun 8, 2020

Choose a reason for hiding this comment

nicoddemus commented Jun 13, 2020

nicoddemus commented May 23, 2020 •

edited

Loading

bluetech left a comment •

edited

Loading

nicoddemus commented Jun 7, 2020 •

edited

Loading