diff --git a/changelog/11523.improvement.rst b/changelog/11523.improvement.rst index 91cd6ccc2..f7d8ff89d 100644 --- a/changelog/11523.improvement.rst +++ b/changelog/11523.improvement.rst @@ -1 +1,5 @@ -pytest.importorskip will now skip both ImportError and ModuleNotFoundError. +:func:`pytest.importorskip` will now issue a warning if the module could be found, but raised :class:`ImportError` instead of :class:`ModuleNotFoundError`. + +The warning can be suppressed by passing ``exc_type=ImportError`` to :func:`pytest.importorskip`. + +See :ref:`import-or-skip-import-error` for details. diff --git a/doc/en/deprecations.rst b/doc/en/deprecations.rst index cd6d1e60a..36b1e7d80 100644 --- a/doc/en/deprecations.rst +++ b/doc/en/deprecations.rst @@ -19,6 +19,44 @@ Below is a complete list of all pytest features which are considered deprecated. :class:`~pytest.PytestWarning` or subclasses, which can be filtered using :ref:`standard warning filters `. +.. _import-or-skip-import-error: + +``pytest.importorskip`` default behavior regarding :class:`ImportError` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +.. deprecated:: 8.2 + +Traditionally :func:`pytest.importorskip` will capture :class:`ImportError`, with the original intent being to skip +tests where a dependent module is not installed, for example testing with different available dependencies. + +However some packages might be installed in the system, but are not importable due to +some other issue (e.g., a compilation error or a broken installation). In those cases :func:`pytest.importorskip` +would still silently skip the test, but more often than not users would like to see the unexpected +error so the underlying issue can be fixed. + +In ``8.2`` the ``exc_type`` parameter has been added, giving users the ability of passing :class:`ModuleNotFoundError` +to only skip tests only if the module cannot really be found, and not because of some other error. + +Catching only :class:`ModuleNotFoundError` by default (and let other errors propagate) would be the best solution, +however for backward compatibility, pytest will keep the existing behavior but raise an warning if: + +1. The captured exception is of type :class:`ImportError`, and: +2. The user does not pass ``exc_type`` explicitly. + +If the import attempt raises :class:`ModuleNotFoundError` (the usual case), then the module is skipped and no +warning is emitted. + +This will maintain the normal cases working the same way, while unexpected errors will now issue a warning. +Users can supress the warning by passing ``exc_type=ImportError`` explicitly. + +In ``9.0``, the warning will turn into an error, and in ``9.1`` :func:`pytest.importorskip` will only capture +:class:`ModuleNotFoundError` by default and no warnings will be issued anymore -- but users can still capture +:class:`ImportError` by passing it to ``exc_type``. + +This roadmap should then be as little disruptive as possible: the intended case will continue to work as normal, +and the exceptional cases will issue a warning, while providing users with a escape hatch when needed. + + .. _node-ctor-fspath-deprecation: ``fspath`` argument for Node constructors replaced with ``pathlib.Path`` diff --git a/src/_pytest/outcomes.py b/src/_pytest/outcomes.py index 8f19f29e1..75fa1f086 100644 --- a/src/_pytest/outcomes.py +++ b/src/_pytest/outcomes.py @@ -211,7 +211,17 @@ def importorskip( If given, this reason is shown as the message when the module cannot be imported. :param exc_type: - If given, modules are skipped if this exception is raised. + The exception that should be captured in order to skip modules. + Must be :py:class:`ImportError` or a subclass. + + If the module can be imported but raises :class:`ImportError`, pytest will + issue a warning to the user, as often users expect the module not to be + found (which would raise :class:`ModuleNotFoundError` instead). + + This warning can be suppressed by passing ``exc_type=ImportError`` explicitly. + + See :ref:`import-or-skip-import-error` for details. + :returns: The imported module. This should be assigned to its canonical name. @@ -219,39 +229,61 @@ def importorskip( Example:: docutils = pytest.importorskip("docutils") + + .. versionadded:: 8.2 + + The ``exc_type`` parameter. """ import warnings __tracebackhide__ = True compile(modname, "", "eval") # to catch syntaxerrors + # Until pytest 9.1, we will warn the user if we catch ImportError (instead of ModuleNotFoundError), + # as this might be hiding an installation/environment problem, which is not usually what is intended + # when using importorskip() (#11523). + # In 9.1, to keep the function signature compatible, we just change the code below to: + # 1. Use `exc_type = ModuleNotFoundError` if `exc_type` is not given. + # 2. Remove `warn_on_import` and the warning handling. + if exc_type is None: + exc_type = ImportError + warn_on_import_error = True + else: + warn_on_import_error = False + + skipped: Optional[Skipped] = None + warning: Optional[Warning] = None + with warnings.catch_warnings(): # Make sure to ignore ImportWarnings that might happen because # of existing directories with the same name we're trying to # import but without a __init__.py file. warnings.simplefilter("ignore") - if exc_type is None: - exc_type = ImportError - warn_on_import_error = True - else: - warn_on_import_error = False - try: __import__(modname) except exc_type as exc: + # Do not raise or issue warnings inside the catch_warnings() block. if reason is None: reason = f"could not import {modname!r}: {exc}" - if warn_on_import_error and type(exc) is ImportError: - warnings.warn( - PytestDeprecationWarning( - f"""pytest.importorskip() caught {exc},but this will change in a future pytest release - to only capture ModuleNotFoundError exceptions by default.\nTo overwrite the future - behavior and silence this warning, pass exc_type=ImportError explicitly.""" - ) - ) + skipped = Skipped(reason, allow_module_level=True) - raise Skipped(reason, allow_module_level=True) from None + if warn_on_import_error and type(exc) is ImportError: + lines = [ + "", + f"Module '{modname}' was found, but when imported by pytest it raised:", + f" {exc!r}", + "In pytest 9.1 this warning will become an error by default.", + "You can fix the underlying problem, or alternatively overwrite this behavior and silence this " + "warning by passing exc_type=ImportError explicitly.", + "See https://docs.pytest.org/en/stable/deprecations.html#pytest-importorskip-default-behavior-regarding-importerror", + ] + warning = PytestDeprecationWarning("\n".join(lines)) + + if warning: + warnings.warn(warning, stacklevel=2) + if skipped: + raise skipped mod = sys.modules[modname] if minversion is None: diff --git a/testing/test_runner.py b/testing/test_runner.py index 69ff27ee4..6e034e945 100644 --- a/testing/test_runner.py +++ b/testing/test_runner.py @@ -9,6 +9,7 @@ from typing import Dict from typing import List from typing import Tuple from typing import Type +import warnings from _pytest import outcomes from _pytest import reports @@ -19,7 +20,6 @@ from _pytest.config import ExitCode from _pytest.monkeypatch import MonkeyPatch from _pytest.outcomes import OutcomeException from _pytest.pytester import Pytester -from _pytest.warning_types import PytestDeprecationWarning import pytest @@ -763,23 +763,71 @@ def test_importorskip_imports_last_module_part() -> None: assert os.path == ospath -def test_importorskip_importError_warning(pytester: Pytester) -> None: - """ - importorskip() will only skip modules by ImportError as well as ModuleNotFoundError - will give warning when using ImportError - #11523 - """ - fn = pytester.makepyfile("raise ImportError('some specific problem')") - pytester.syspathinsert() +class TestImportOrSkipExcType: + """Tests for #11523.""" - with pytest.raises(pytest.skip.Exception): - with pytest.warns(PytestDeprecationWarning): - pytest.importorskip(fn.stem) + def test_no_warning(self) -> None: + # An attempt on a module which does not exist will raise ModuleNotFoundError, so it will + # be skipped normally and no warning will be issued. + with warnings.catch_warnings(record=True) as captured: + warnings.simplefilter("always") + with pytest.raises(pytest.skip.Exception): + pytest.importorskip("TestImportOrSkipExcType_test_no_warning") -def test_importorskip_ModuleNotFoundError() -> None: - with pytest.raises(pytest.skip.Exception): - pytest.importorskip("abcdefgh") + assert captured == [] + + def test_import_error_with_warning(self, pytester: Pytester) -> None: + # Create a module which exists and can be imported, however it raises + # ImportError due to some other problem. In this case we will issue a warning + # about the future behavior change. + fn = pytester.makepyfile("raise ImportError('some specific problem')") + pytester.syspathinsert() + + with warnings.catch_warnings(record=True) as captured: + warnings.simplefilter("always") + + with pytest.raises(pytest.skip.Exception): + pytest.importorskip(fn.stem) + + [warning] = captured + assert warning.category is pytest.PytestDeprecationWarning + + def test_import_error_suppress_warning(self, pytester: Pytester) -> None: + # Same as test_import_error_with_warning, but we can suppress the warning + # by passing ImportError as exc_type. + fn = pytester.makepyfile("raise ImportError('some specific problem')") + pytester.syspathinsert() + + with warnings.catch_warnings(record=True) as captured: + warnings.simplefilter("always") + + with pytest.raises(pytest.skip.Exception): + pytest.importorskip(fn.stem, exc_type=ImportError) + + assert captured == [] + + def test_warning_integration(self, pytester: Pytester) -> None: + pytester.makepyfile( + """ + import pytest + def test_foo(): + pytest.importorskip("warning_integration_module") + """ + ) + pytester.makepyfile( + warning_integration_module=""" + raise ImportError("required library foobar not compiled properly") + """ + ) + result = pytester.runpytest() + result.stdout.fnmatch_lines( + [ + "*Module 'warning_integration_module' was found, but when imported by pytest it raised:", + "* ImportError('required library foobar not compiled properly')", + "*1 skipped, 1 warning*", + ] + ) def test_importorskip_dev_module(monkeypatch) -> None: