From 4eb8b6d525b7704432f378cfa45ab9ec02bf4e39 Mon Sep 17 00:00:00 2001 From: Shekhar verma Date: Fri, 26 Apr 2024 13:18:57 +0530 Subject: [PATCH] Changed importError to ModuleNotFoundError (#12220) * Changed importError to ModuleNotFoundError * added testing for importorskip * added exc_types parameter in importorskip * Added warning and Test Cases * Improve tests and docs * Improve deprecation docs * Change exc_type to kw only * Apply suggestions from code review Co-authored-by: Florian Bruhin * Fix check --------- Co-authored-by: Bruno Oliveira Co-authored-by: Florian Bruhin --- changelog/11523.improvement.rst | 5 +++ doc/en/deprecations.rst | 35 +++++++++++++++++ src/_pytest/outcomes.py | 63 ++++++++++++++++++++++++++++-- testing/test_runner.py | 68 +++++++++++++++++++++++++++++++++ 4 files changed, 168 insertions(+), 3 deletions(-) create mode 100644 changelog/11523.improvement.rst diff --git a/changelog/11523.improvement.rst b/changelog/11523.improvement.rst new file mode 100644 index 00000000000..f7d8ff89df6 --- /dev/null +++ b/changelog/11523.improvement.rst @@ -0,0 +1,5 @@ +: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 cd6d1e60aef..5ac93f15144 100644 --- a/doc/en/deprecations.rst +++ b/doc/en/deprecations.rst @@ -19,6 +19,41 @@ 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 dependencies. + +However some packages might be installed in the system, but are not importable due to +some other issue, for example, 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 skip tests only if the module cannot really be found, and not because of some other error. + +Catching only :class:`ModuleNotFoundError` by default (and letting 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 way, the usual cases will keep working the same way, while unexpected errors will now issue a warning, with +users being able to 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``. + + .. _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 e2a816f5850..76d94accd0d 100644 --- a/src/_pytest/outcomes.py +++ b/src/_pytest/outcomes.py @@ -11,6 +11,8 @@ from typing import Type from typing import TypeVar +from .warning_types import PytestDeprecationWarning + class OutcomeException(BaseException): """OutcomeException and its subclass instances indicate and contain info @@ -192,7 +194,11 @@ def xfail(reason: str = "") -> NoReturn: def importorskip( - modname: str, minversion: Optional[str] = None, reason: Optional[str] = None + modname: str, + minversion: Optional[str] = None, + reason: Optional[str] = None, + *, + exc_type: Optional[Type[ImportError]] = None, ) -> Any: """Import and return the requested module ``modname``, or skip the current test if the module cannot be imported. @@ -205,6 +211,18 @@ def importorskip( :param reason: If given, this reason is shown as the message when the module cannot be imported. + :param exc_type: + 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. @@ -212,23 +230,62 @@ 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") + try: __import__(modname) - except ImportError as exc: + 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}" - raise Skipped(reason, allow_module_level=True) from None + skipped = Skipped(reason, allow_module_level=True) + + if warn_on_import_error and not isinstance(exc, ModuleNotFoundError): + 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: return mod diff --git a/testing/test_runner.py b/testing/test_runner.py index ffac0f2d92b..6e034e94532 100644 --- a/testing/test_runner.py +++ b/testing/test_runner.py @@ -9,6 +9,7 @@ from typing import List from typing import Tuple from typing import Type +import warnings from _pytest import outcomes from _pytest import reports @@ -762,6 +763,73 @@ def test_importorskip_imports_last_module_part() -> None: assert os.path == ospath +class TestImportOrSkipExcType: + """Tests for #11523.""" + + 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") + + 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: try: mod = types.ModuleType("mockmodule")