Repository: pytest-dev/pytest
Pull request: Changed importError to ModuleNotFoundError
Closes #11523

Changed ImportError to ModuleNotFoundError in importorskip()``

Problem statement:
importorskip variant that uses ModuleNotFoundError
<!--
Thanks for suggesting a feature!

Quick check-list while suggesting features:
-->

#### What's the problem this feature will solve?
sometimes I install a package eg PyTables and it fails to compile properly and fails with an ImportError with an undefined symbol, however I don't see this in CI because I only do `pytest.importorskip("pytables")`

#### Describe the solution you'd like
a new kwarg to importorskip or a new function that catches ModuleNotFoundError instead of ImportError eg
`importorskip(exc=ModuleNotFoundError)` or `findmoduleorskip(...)`

#### Alternative Solutions
N/A

#### Additional context
Originally asked on discord

Hints from issue discussion:
Hey @graingert thanks for the proposal.

> a new kwarg to importorskip or a new function that catches ModuleNotFoundError instead of ImportError eg
importorskip(exc=ModuleNotFoundError) or findmoduleorskip(...)

I personally like `importorskip(exc=ModuleNotFoundError)` 👍 
i think it would be very nice if a requirement could be passed to allow checking for versions
The alternative to `exc=` is to just narrow the current `except ImportError` to `except ModuleNotFoundError`. This will a breaking change, but I wonder if anyone really wants to skip on non-`ModuleNotFoundError` import errors?
We could also check the exception message matches the module being imported
> This will a breaking change, but I wonder if anyone really wants to skip on non-ModuleNotFoundError import errors?

Thought the same, I for one would always add `exc=ModuleNotFoundError`.

Perhaps it is worthwhile to go through a deprecation period, where we issue a warning in case a `importorskip` skipped a module but the message did not include the actual module name? Later we can apply the change to only catch `ModuleNotFoundError` then.
I don't understand the rationale of doing string matching against the exception message here. Why would we need to check that if we can just check the exception type? Why would it be useful to catch an `ImportError` which is not a `ModuleNotFoundError` but just happens to include the module name, vs. one which does not?
> Why would we need to check that if we can just check the exception type? Why would it be useful to catch an ImportError which is not a ModuleNotFoundError but just happens to include the module name, vs. one which does not?

Just to issue a warning that the behavior will change in the future, where we will only catch `ModuleNotFoundError`:

```python
except ModuleNotFoundError:
    skip()
except ImportError as e:
    if module_name not in str(e):
        warning(f"importorskip('{modulename}') caught an ImportError, but the message:\n{e}\n does not contain the module name. This behavior will change in the future to only catch ModuleNotFoundError")
   skip()
```

The idea is that we remove the `except ImportError` clause in a future release, as this is a breaking change (for the better IMHO).


I still don't follow. Why would we not want to warn if the message contains the module name?
To let users know that this might break their test suite in the future, suppose a user has this in their code base:

```python
def test_numpy_support() -> None:
    pytest.importorskip("numpy")
    ...
```

Currently, the test is skipped, but not because `numpy` is not installed, but because it is failing to import due to something else entirely, for example due to an undefined symbol.

If we just change `importorskip` to catch `ModuleNotFoundError`, this would cause the test suite to break.

We might decide instead to introduce a warning, so users would see a warning like this currently:

```
PytestDeprecationWarning: importorskip caught ImportError with this message:

    ImportError: undefined symbol ...

Seems like the reason for the failure was not that the module was missing, but another cause.
pytest X will only catch `ModuleNotFoundError`, so this will fail in the future.
```

Then in a future release we change the code to only catch `ModuleNotFoundError`.

--- 

But now that I write this, perhaps does not make sense to introduce this warning at all? Seems like users would like for this to fail right away?
But in any case the module actually isn't present, we already will get a `ModuleNotFoundError` from Python. Why not warn on *any* `ImportError`, if anything? I don't see a reason why we should warn if, say, the undefined symbol in your example happens to be `SomeLib::bla()` but should not warn if the symbol happens to be called `NumpyCExt::bla()`.

Commit message: Change exc_type to kw only

Patch to review:
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..9b5a80e9520 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 <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 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 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..69822272538 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 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:
         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")


Code review comments:
diff_hunk: @@ -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 type(exc) is ImportError:
path: src/_pytest/outcomes.py
text: @user1:
How do we want to handle custom subclasses of `ImportError`? Wouldn't a

```suggestion
            if warn_on_import_error and not issubclass(exc, ModuleNotFoundError):
```

be more appropriate here?

@user2:
Hmm good point. If some library/user out there is raising an `ImportError` subclass (but not subclassing `ModuleNotFoundError`) we probably want to warn as well about the incoming behavior change, good catch.

diff_hunk: @@ -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 let other errors propagate) would be the best solution,
path: doc/en/deprecations.rst
text: @user1:
nitpick:

```suggestion
Catching only :class:`ModuleNotFoundError` by default (and letting other errors propagate) would be the best solution,
```