Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Changed importError to ModuleNotFoundError #12220

Merged
merged 14 commits into from
Apr 26, 2024
Merged

Changed importError to ModuleNotFoundError #12220

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3a26211
Changed importError to ModuleNotFoundError
shekhuverma Apr 16, 2024
3f80fd7
Merge branch 'pytest-dev:main' into main
shekhuverma Apr 19, 2024
fd288e1
added testing for importorskip
shekhuverma Apr 19, 2024
1fe14e0
Merge branch 'importorskip-ModuleNotFoundError'
shekhuverma Apr 19, 2024
543262c
added exc_types parameter in importorskip
shekhuverma Apr 19, 2024
1456b71
Merge branch 'importorskip-ModuleNotFoundError'
shekhuverma Apr 19, 2024
9a9643d
Added warning and Test Cases
shekhuverma Apr 20, 2024
3ca5036
Merge branch 'importorskip-ModuleNotFoundError'
shekhuverma Apr 20, 2024
7de869d
Improve tests and docs
nicoddemus Apr 20, 2024
3ca329b
Improve deprecation docs
nicoddemus Apr 20, 2024
f14d78d
Change exc_type to kw only
nicoddemus Apr 20, 2024
b59336a
Apply suggestions from code review
nicoddemus Apr 25, 2024
76fd0e5
Fix check
nicoddemus Apr 25, 2024
a55abeb
Merge branch 'pytest-dev:main' into main
shekhuverma Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/11523.improvement.rst
View file
Open in desktop
@@ -0,0 +1 @@
pytest.importorskip will now skip both ImportError and ModuleNotFoundError.
23 changes: 21 additions & 2 deletions src/_pytest/outcomes.py
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -192,7 +194,10 @@ 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.
Expand All @@ -205,6 +210,8 @@ def importorskip(
:param reason:
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.

:returns:
The imported module. This should be assigned to its canonical name.
Expand All @@ -223,12 +230,24 @@ def importorskip(
# 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 = ModuleNotFoundError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I was not clearer.

I meant that we should only issue the warning if:

  1. The user is not passing an explicit exc_type.
  2. We actually capture ImportError.

So something like:

    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:
            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.",
                    )
                )
            raise Skipped(reason, allow_module_level=True) from None

So we will not be changing the behavior right now, only in the future.

Do you agree @The-Compiler?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that sounds like a solid approach. We'll also need tests for the warning and the different argument behaviours.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

   def importorskip(
    modname: str,
    minversion: Optional[str] = None,
    reason: Optional[str] = None,
    exc_type: Optional[Type[ImportError,ModuleNotFoundError]] = None,
    ) -> Any:
    .
    .
    .
    if exc_type is None:
       #to keep the old behavior
       exc_type = ImportError
       warn_on_import_error = True
    else:
        warn_on_import_error = False
        
    if warn_on_import_error or type(exc) is ImportError:
        warnings.warn()

I think the function should be like this, showing a warning if the user doesn't pass any exc_type or when there is ImportError

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not want to issue the warning unless we actually caught ImportError.

Most of the time the exception being raised is ModuleNotFoundError, which is OK and what users expect.

The problem is when the library is found, but ImportError is raised because of a related problem (for example failure to load a required a shared library). In this case we want to issue a warning now, and in pytest 9.0, let the exception propagate upwards.

The exc_type parameter will let users be explicit about what they want to capture.

else:
exc_type = ImportError
warnings.warn(
PytestDeprecationWarning(
"The Default behaviour will change to ImportError in future",
)
)

try:
__import__(modname)
except ImportError as exc:
except exc_type as exc:
if reason is None:
reason = f"could not import {modname!r}: {exc}"
raise Skipped(reason, allow_module_level=True) from None

mod = sys.modules[modname]
if minversion is None:
return mod
Expand Down
15 changes: 15 additions & 0 deletions testing/test_runner.py
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@
from _pytest.monkeypatch import MonkeyPatch
from _pytest.outcomes import OutcomeException
from _pytest.pytester import Pytester
from _pytest.warning_types import PytestDeprecationWarning
import pytest


Expand Down Expand Up @@ -762,6 +763,20 @@ 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()

with pytest.raises(pytest.skip.Exception):
with pytest.warns(PytestDeprecationWarning):
pytest.importorskip(fn.stem, exc_type=ImportError)


def test_importorskip_dev_module(monkeypatch) -> None:
try:
mod = types.ModuleType("mockmodule")
Expand Down