Improve tests and docs

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.

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 <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 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``

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:
+            skipped = Skipped(reason, allow_module_level=True)
-                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."""
-                    )
-                )
 
-            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:

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:
+class TestImportOrSkipExcType:
-    """
+    """Tests for #11523."""
-    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()
 
-    with pytest.raises(pytest.skip.Exception):
+    def test_no_warning(self) -> None:
-        with pytest.warns(PytestDeprecationWarning):
+        # An attempt on a module which does not exist will raise ModuleNotFoundError, so it will
-            pytest.importorskip(fn.stem)
+        # 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:
+        assert captured == []
-    with pytest.raises(pytest.skip.Exception):
+
-        pytest.importorskip("abcdefgh")
+    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: