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 <[email protected]>

* Fix check

---------

Co-authored-by: Bruno Oliveira <[email protected]>
Co-authored-by: Florian Bruhin <[email protected]>

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.

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 <warnings>`.
 
 
+.. _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``

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,30 +211,81 @@ 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.
 
     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

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")