Introduce --import-mode=importlib (#7246)

Fix #5821

Co-authored-by: Ran Benita <[email protected]>

Author: nicoddemus

changelog/7245.feature.rst

@@ -0,0 +1,14 @@
+New ``--import-mode=importlib`` option that uses `importlib <https://docs.python.org/3/library/importlib.html>`__ to import test modules.
+
+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.
+
+We intend to make ``--import-mode=importlib`` the default in future versions, so users are encouraged
+to try the new mode and provide feedback (both positive or negative) in issue `#7245 <https://github.com/pytest-dev/pytest/issues/7245>`__.
+
+You can read more about this option in `the documentation <https://docs.pytest.org/en/latest/pythonpath.html#import-modes>`__.

doc/en/goodpractices.rst

@@ -91,7 +91,8 @@ This has the following benefits:
     See :ref:`pytest vs python -m pytest` for more information about the difference between calling ``pytest`` and
     ``python -m pytest``.
 
-Note that using this scheme your test files must have **unique names**, because
+Note that this scheme has a drawback if you are using ``prepend`` :ref:`import mode <import-modes>`
+(which is the default): your test files must have **unique names**, because
 ``pytest`` will import them as *top-level* modules since there are no packages
 to derive a full package name from. In other words, the test files in the example above will
 be imported as ``test_app`` and ``test_view`` top-level modules by adding ``tests/`` to
@@ -118,9 +119,12 @@ Now pytest will load the modules as ``tests.foo.test_view`` and ``tests.bar.test
 you to have modules with the same name. But now this introduces a subtle problem: in order to load
 the test modules from the ``tests`` directory, pytest prepends the root of the repository to
 ``sys.path``, which adds the side-effect that now ``mypkg`` is also importable.
+
 This is problematic if you are using a tool like `tox`_ to test your package in a virtual environment,
 because you want to test the *installed* version of your package, not the local code from the repository.
 
+.. _`src-layout`:
+
 In this situation, it is **strongly** suggested to use a ``src`` layout where application root package resides in a
 sub-directory of your root:
 
@@ -145,6 +149,15 @@ sub-directory of your root:
 This layout prevents a lot of common pitfalls and has many benefits, which are better explained in this excellent
 `blog post by Ionel Cristian Mărieș <https://blog.ionelmc.ro/2014/05/25/python-packaging/#the-structure>`_.
 
+.. note::
+    The new ``--import-mode=importlib`` (see :ref:`import-modes`) doesn't have
+    any of the drawbacks above because ``sys.path`` and ``sys.modules`` are not changed when importing
+    test modules, so users that run
+    into this issue are strongly encouraged to try it and report if the new option works well for them.
+
+    The ``src`` directory layout is still strongly recommended however.
+
+
 Tests as part of application code
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -190,8 +203,8 @@ Note that this layout also works in conjunction with the ``src`` layout mentione
 
 .. note::
 
-    If ``pytest`` finds an "a/b/test_module.py" test file while
-    recursing into the filesystem it determines the import name
+    In ``prepend`` and ``append`` import-modes, if pytest finds a ``"a/b/test_module.py"``
+    test file while recursing into the filesystem it determines the import name
     as follows:
 
     * determine ``basedir``: this is the first "upward" (towards the root)
@@ -212,6 +225,10 @@ Note that this layout also works in conjunction with the ``src`` layout mentione
     from each other and thus deriving a canonical import name helps
     to avoid surprises such as a test module getting imported twice.
 
+    With ``--import-mode=importlib`` things are less convoluted because
+    pytest doesn't need to change ``sys.path`` or ``sys.modules``, making things
+    much less surprising.
+
 
 .. _`virtualenv`: https://pypi.org/project/virtualenv/
 .. _`buildout`: http://www.buildout.org/

doc/en/pythonpath.rst

@@ -3,11 +3,65 @@
 pytest import mechanisms and ``sys.path``/``PYTHONPATH``
 ========================================================
 
-Here's a list of scenarios where pytest may need to change ``sys.path`` in order
-to import test modules or ``conftest.py`` files.
+.. _`import-modes`:
+
+Import modes
+------------
+
+pytest as a testing framework needs to import test modules and ``conftest.py`` files for execution.
+
+Importing files in Python (at least until recently) is a non-trivial processes, often requiring
+changing `sys.path <https://docs.python.org/3/library/sys.html#sys.path>`__. Some aspects of the
+import process can be controlled through the ``--import-mode`` command-line flag, which can assume
+these values:
+
+* ``prepend`` (default): the directory path containing each module will be inserted into the *beginning*
+  of ``sys.path`` if not already there, and then imported with the `__import__ <https://docs.python.org/3/library/functions.html#__import__>`__ builtin.
+
+  This requires test module names to be unique when the test directory tree is not arranged in
+  packages, because the modules will put in ``sys.modules`` after importing.
+
+  This is the classic mechanism, dating back from the time Python 2 was still supported.
+
+* ``append``: the directory containing each module is appended to the end of ``sys.path`` if not already
+  there, and imported with ``__import__``.
+
+  This better allows to run test modules against installed versions of a package even if the
+  package under test has the same import root. For example:
+
+  ::
+
+        testing/__init__.py
+        testing/test_pkg_under_test.py
+        pkg_under_test/
+
+  the tests will run against the installed version
+  of ``pkg_under_test`` when ``--import-mode=append`` is used whereas
+  with ``prepend`` they would pick up the local version. This kind of confusion is why
+  we advocate for using :ref:`src <src-layout>` layouts.
+
+  Same as ``prepend``, requires test module names to be unique when the test directory tree is
+  not arranged in packages, because the modules will put in ``sys.modules`` after importing.
+
+* ``importlib``: new in pytest-6.0, this mode uses `importlib <https://docs.python.org/3/library/importlib.html>`__ to import test modules. This gives full control over the import process, and doesn't require
+  changing ``sys.path`` or ``sys.modules`` at all.
+
+  For this reason this doesn't require test module names to be unique at all, but also makes test
+  modules non-importable by each other. This was made possible in previous modes, for tests not residing
+  in Python packages, because of the side-effects of changing ``sys.path`` and ``sys.modules``
+  mentioned above. Users which require this should turn their tests into proper packages instead.
+
+  We intend to make ``importlib`` the default in future releases.
+
+``prepend`` and ``append`` import modes scenarios
+-------------------------------------------------
+
+Here's a list of scenarios when using ``prepend`` or ``append`` import modes where pytest needs to
+change ``sys.path`` in order to import test modules or ``conftest.py`` files, and the issues users
+might encounter because of that.
 
 Test modules / ``conftest.py`` files inside packages
-----------------------------------------------------
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Consider this file and directory layout::
 
@@ -28,8 +82,6 @@ When executing:
 
     pytest root/
 
-
-
 pytest will find ``foo/bar/tests/test_foo.py`` and realize it is part of a package given that
 there's an ``__init__.py`` file in the same folder. It will then search upwards until it can find the
 last folder which still contains an ``__init__.py`` file in order to find the package *root* (in
@@ -44,7 +96,7 @@ and allow test modules to have duplicated names. This is also discussed in detai
 :ref:`test discovery`.
 
 Standalone test modules / ``conftest.py`` files
------------------------------------------------
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Consider this file and directory layout::
 

src/_pytest/_code/code.py

@@ -1204,7 +1204,10 @@ def getfslineno(obj: Any) -> Tuple[Union[str, py.path.local], int]:
 
 
 def filter_traceback(entry: TracebackEntry) -> bool:
-    """Return True if a TracebackEntry instance should be removed from tracebacks:
+    """Return True if a TracebackEntry instance should be included in tracebacks.
+
+    We hide traceback entries of:
+
     * dynamically generated code (no code to show up for it);
     * internal traceback from pytest or its internal libraries, py and pluggy.
     """

src/_pytest/compat.py

@@ -33,6 +33,7 @@
 
 
 if TYPE_CHECKING:
+    from typing import NoReturn
     from typing import Type
     from typing_extensions import Final
 
@@ -401,3 +402,38 @@ def __get__(self, instance, owner=None):  # noqa: F811
     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__)

src/_pytest/config/__init__.py

@@ -41,6 +41,7 @@
 from _pytest.compat import TYPE_CHECKING
 from _pytest.outcomes import fail
 from _pytest.outcomes import Skipped
+from _pytest.pathlib import import_path
 from _pytest.pathlib import Path
 from _pytest.store import Store
 from _pytest.warning_types import PytestConfigWarning
@@ -98,6 +99,15 @@ def __str__(self):
         )
 
 
+def filter_traceback_for_conftest_import_failure(entry) -> bool:
+    """filters tracebacks entries which point to pytest internals or importlib.
+
+    Make a special case for importlib because we use it to import test modules and conftest files
+    in _pytest.pathlib.import_path.
+    """
+    return filter_traceback(entry) and "importlib" not in str(entry.path).split(os.sep)
+
+
 def main(args=None, plugins=None) -> Union[int, ExitCode]:
     """ return exit code, after performing an in-process test run.
 
@@ -115,7 +125,9 @@ def main(args=None, plugins=None) -> Union[int, ExitCode]:
             tw.line(
                 "ImportError while loading conftest '{e.path}'.".format(e=e), red=True
             )
-            exc_info.traceback = exc_info.traceback.filter(filter_traceback)
+            exc_info.traceback = exc_info.traceback.filter(
+                filter_traceback_for_conftest_import_failure
+            )
             exc_repr = (
                 exc_info.getrepr(style="short", chain=False)
                 if exc_info.traceback
@@ -450,21 +462,21 @@ def _set_initial_conftests(self, namespace):
                 path = path[:i]
             anchor = current.join(path, abs=1)
             if anchor.exists():  # we found some file object
-                self._try_load_conftest(anchor)
+                self._try_load_conftest(anchor, namespace.importmode)
                 foundanchor = True
         if not foundanchor:
-            self._try_load_conftest(current)
+            self._try_load_conftest(current, namespace.importmode)
 
-    def _try_load_conftest(self, anchor):
-        self._getconftestmodules(anchor)
+    def _try_load_conftest(self, anchor, importmode):
+        self._getconftestmodules(anchor, importmode)
         # let's also consider test* subdirs
         if anchor.check(dir=1):
             for x in anchor.listdir("test*"):
                 if x.check(dir=1):
-                    self._getconftestmodules(x)
+                    self._getconftestmodules(x, importmode)
 
     @lru_cache(maxsize=128)
-    def _getconftestmodules(self, path):
+    def _getconftestmodules(self, path, importmode):
         if self._noconftest:
             return []
 
@@ -482,21 +494,21 @@ def _getconftestmodules(self, path):
                 continue
             conftestpath = parent.join("conftest.py")
             if conftestpath.isfile():
-                mod = self._importconftest(conftestpath)
+                mod = self._importconftest(conftestpath, importmode)
                 clist.append(mod)
         self._dirpath2confmods[directory] = clist
         return clist
 
-    def _rget_with_confmod(self, name, path):
-        modules = self._getconftestmodules(path)
+    def _rget_with_confmod(self, name, path, importmode):
+        modules = self._getconftestmodules(path, importmode)
         for mod in reversed(modules):
             try:
                 return mod, getattr(mod, name)
             except AttributeError:
                 continue
         raise KeyError(name)
 
-    def _importconftest(self, conftestpath):
+    def _importconftest(self, conftestpath, importmode):
         # Use a resolved Path object as key to avoid loading the same conftest twice
         # with build systems that create build directories containing
         # symlinks to actual files.
@@ -512,7 +524,7 @@ def _importconftest(self, conftestpath):
             _ensure_removed_sysmodule(conftestpath.purebasename)
 
         try:
-            mod = conftestpath.pyimport()
+            mod = import_path(conftestpath, mode=importmode)
         except Exception as e:
             raise ConftestImportFailure(conftestpath, sys.exc_info()) from e
 
@@ -1213,7 +1225,9 @@ def _getini(self, name: str) -> Any:
 
     def _getconftest_pathlist(self, name, path):
         try:
-            mod, relroots = self.pluginmanager._rget_with_confmod(name, path)
+            mod, relroots = self.pluginmanager._rget_with_confmod(
+                name, path, self.getoption("importmode")
+            )
         except KeyError:
             return None
         modpath = py.path.local(mod.__file__).dirpath()

src/_pytest/doctest.py

@@ -33,6 +33,7 @@
 from _pytest.config.argparsing import Parser
 from _pytest.fixtures import FixtureRequest
 from _pytest.outcomes import OutcomeException
+from _pytest.pathlib import import_path
 from _pytest.python_api import approx
 from _pytest.warning_types import PytestWarning
 
@@ -530,10 +531,12 @@ def _find(
                     )
 
         if self.fspath.basename == "conftest.py":
-            module = self.config.pluginmanager._importconftest(self.fspath)
+            module = self.config.pluginmanager._importconftest(
+                self.fspath, self.config.getoption("importmode")
+            )
         else:
             try:
-                module = self.fspath.pyimport()
+                module = import_path(self.fspath)
             except ImportError:
                 if self.config.getvalue("doctest_ignore_import_errors"):
                     pytest.skip("unable to import module %r" % self.fspath)

src/_pytest/main.py

@@ -173,6 +173,14 @@ def pytest_addoption(parser: Parser) -> None:
         default=False,
         help="Don't ignore tests in a local virtualenv directory",
     )
+    group.addoption(
+        "--import-mode",
+        default="prepend",
+        choices=["prepend", "append", "importlib"],
+        dest="importmode",
+        help="prepend/append to sys.path when importing test modules and conftest files, "
+        "default is to prepend.",
+    )
 
     group = parser.getgroup("debugconfig", "test session debugging and configuration")
     group.addoption(

src/_pytest/nodes.py

@@ -547,7 +547,9 @@ def _gethookproxy(self, fspath: py.path.local):
         # check if we have the common case of running
         # hooks with all conftest.py files
         pm = self.config.pluginmanager
-        my_conftestmodules = pm._getconftestmodules(fspath)
+        my_conftestmodules = pm._getconftestmodules(
+            fspath, self.config.getoption("importmode")
+        )
         remove_mods = pm._conftest_plugins.difference(my_conftestmodules)
         if remove_mods:
             # one or more conftests are not in use at this fspath