From 386f6fef7017731a9b3d983745e58c4b90851d23 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 23 May 2020 11:17:48 -0300 Subject: [PATCH] Introduce --import-mode=importlib 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. --- changelog/7245.feature.rst | 14 ++++++++ doc/en/goodpractices.rst | 23 +++++++++++-- doc/en/pythonpath.rst | 61 ++++++++++++++++++++++++++++++---- src/_pytest/config/__init__.py | 26 ++++++++------- src/_pytest/nodes.py | 4 ++- src/_pytest/python.py | 2 +- testing/acceptance_test.py | 5 +-- testing/test_collection.py | 38 +++++++++++++++++++++ 8 files changed, 148 insertions(+), 25 deletions(-) create mode 100644 changelog/7245.feature.rst diff --git a/changelog/7245.feature.rst b/changelog/7245.feature.rst new file mode 100644 index 00000000000..05c3a6c0469 --- /dev/null +++ b/changelog/7245.feature.rst @@ -0,0 +1,14 @@ +New ``--import-mode=importlib`` option that uses `importlib `__ 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 `__. + +You can read more about this option in `the documentation `__. diff --git a/doc/en/goodpractices.rst b/doc/en/goodpractices.rst index 16b41eda4d8..ee5674fd6d8 100644 --- a/doc/en/goodpractices.rst +++ b/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 ` +(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ș `_. +.. 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/ diff --git a/doc/en/pythonpath.rst b/doc/en/pythonpath.rst index f2c86fab967..8b3d7b2b3f3 100644 --- a/doc/en/pythonpath.rst +++ b/doc/en/pythonpath.rst @@ -3,11 +3,62 @@ 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 moduels and ``conftest.py`` files for execution. + +Importing files in Python (at least until recently) is a non-trivial processes, often requiring +changing `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__ `__ 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 ` 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 `__ 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. + + 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 +79,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 +93,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:: diff --git a/src/_pytest/config/__init__.py b/src/_pytest/config/__init__.py index 27083900dfa..77315a56d83 100644 --- a/src/_pytest/config/__init__.py +++ b/src/_pytest/config/__init__.py @@ -451,21 +451,21 @@ def _set_initial_conftests(self, namespace): path = path[:i] anchor = current.join(path, abs=1) if exists(anchor): # 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 [] @@ -483,13 +483,13 @@ 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) @@ -497,7 +497,7 @@ def _rget_with_confmod(self, name, path): 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. @@ -513,7 +513,7 @@ def _importconftest(self, conftestpath): _ensure_removed_sysmodule(conftestpath.purebasename) try: - mod = conftestpath.pyimport() + mod = conftestpath.pyimport(ensuresyspath=importmode) except Exception as e: raise ConftestImportFailure(conftestpath, sys.exc_info()) from e @@ -1166,7 +1166,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() diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py index 3757e0b2717..27dcdb01a32 100644 --- a/src/_pytest/nodes.py +++ b/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 diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 4b716c616b6..4f29db6349b 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -118,7 +118,7 @@ def pytest_addoption(parser: Parser) -> None: group.addoption( "--import-mode", default="prepend", - choices=["prepend", "append"], + choices=["prepend", "append", "importlib"], dest="importmode", help="prepend/append to sys.path when importing test modules, " "default is to prepend.", diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index e2df92d8091..0adc5e792c2 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -147,7 +147,8 @@ def my_dists(): else: assert loaded == ["myplugin1", "myplugin2", "mycov"] - def test_assertion_magic(self, testdir): + @pytest.mark.parametrize("import_mode", ["prepend", "append", "importlib"]) + def test_assertion_rewrite(self, testdir, import_mode): p = testdir.makepyfile( """ def test_this(): @@ -155,7 +156,7 @@ def test_this(): assert x """ ) - result = testdir.runpytest(p) + result = testdir.runpytest(p, "--import-mode={}".format(import_mode)) result.stdout.fnmatch_lines(["> assert x", "E assert 0"]) assert result.ret == 1 diff --git a/testing/test_collection.py b/testing/test_collection.py index 8e5d5aaccb0..b7a1bf05c1b 100644 --- a/testing/test_collection.py +++ b/testing/test_collection.py @@ -1354,3 +1354,41 @@ def from_parent(cls, parent, *, fspath, x): parent=request.session, fspath=tmpdir / "foo", x=10 ) assert collector.x == 10 + + +class TestImportModeImportlib: + def test_collect_duplicate_names(self, testdir): + """--import-mode=importlib can import modules with same names that are not in packages.""" + testdir.makepyfile( + **{ + "tests_a/test_foo.py": "def test_foo1(): pass", + "tests_b/test_foo.py": "def test_foo2(): pass", + } + ) + result = testdir.runpytest("-v", "--import-mode=importlib") + result.stdout.fnmatch_lines( + [ + "tests_a/test_foo.py::test_foo1 *", + "tests_b/test_foo.py::test_foo2 *", + "* 2 passed in *", + ] + ) + + def test_conftest(self, testdir): + """Directory containing conftest modules are not put in sys.path as a side-effect of + importing them.""" + tests_dir = testdir.tmpdir.join("tests") + testdir.makepyfile( + **{ + "tests/conftest.py": "", + "tests/test_foo.py": """ + import sys + def test_check(): + assert r"{tests_dir}" not in sys.path + """.format( + tests_dir=tests_dir + ), + } + ) + result = testdir.runpytest("-v", "--import-mode=importlib") + result.stdout.fnmatch_lines(["* 1 passed in *"])