Changed importError to ModuleNotFoundError #12220

shekhuverma · 2024-04-16T14:34:35Z

Changed ImportError to ModuleNotFoundError in importorskip()``

nicoddemus

Can you please add a test that shows importorskip will only skip on an explicit ModuleNotFoundError, but produce a failure for an ImportError exception?

The-Compiler

I think at the very least, we need an easy escape hatch to get the former behavior, no? I'm somewhat ambivalent on adding a warning (after all, it's a loud failure - the only difference would be a warning vs. failing collection).

However, if someone really wants to catch ImportError here, they should be able to do pytest.importorskip("mymodule", exc=ImportError) or somesuch (with exc=, we might want to ensure that issubclass(exc, ImportError) maybe, at least on the type annotation level).

Right now, if someone wants to keep the existing behavior, they'd need to go from pytest.importorskip("mymodule") to something like (untested):

try:
    import mymodule
except ImportError:
    pytest.skip("some good reason here", allow_module_level=True)

or so, which seems quite burdensome.

The-Compiler · 2024-04-19T11:31:01Z

testing/test_runner.py

@@ -762,6 +765,27 @@ def test_importorskip_imports_last_module_part() -> None:
    assert os.path == ospath


+def test_importorskip_importError_Exception() -> None:
+    ## Mocking the import function to raise a importError
+    realimport = builtins.__import__


Please use monkeypatch instead of hand-rolling this.

nicoddemus

Besides the changes requested, please also add a new changelog entry, as improvement.

Actually. I think we probably should not introduce the new behavior right away, instead capture ImportError and show a warning stating that the default behavior will change in future versions to catch ModuleNotFoundError instead.

testing/test_runner.py

nicoddemus · 2024-04-19T11:31:29Z

src/_pytest/outcomes.py

@@ -225,7 +225,7 @@ def importorskip(
        warnings.simplefilter("ignore")
        try:
            __import__(modname)
-        except ImportError as exc:
+        except ModuleNotFoundError as exc:


Let's add a new parameter to this function, *, exc_type: Type[ImportError] = ModuleNotFoundError and use it here:

Suggested change

except ModuleNotFoundError as exc:

except exc_type as exc:

This way we provide a escape hatch to users which rely on the old behavior.

Also please update the docs, describing the parameters and explicitly showing an example for users that want the old behavior.

nicoddemus · 2024-04-19T11:40:19Z

@The-Compiler and I left similar reviews almost at the same time hehehe.

I think we should actually not change the default behavior right away, for better or worse this is a breaking change.

I think introducing a new parameter exc_type: Optional[Type[ImportError]] = None, that when None:

We capture ModuleNotFoundError first; in that case skip.
We capture ImportError next, in that case issue a warning and also skip.

In 9.0, we turn the warning into an error, and 9.1 remove (2) from the code and let the ImportError propagate unchanged.

The benefits of this is that users would be able to use exc_type today in code, will not break anything that passes today (only a warning on ImportError), and provides a escape hatch for users that don't care and want to skip on any ImportError.

shekhuverma · 2024-04-19T13:57:51Z

Thanks for the review.
I made the requested changes, let me know if there is something else to be done

nicoddemus · 2024-04-19T14:30:17Z

src/_pytest/outcomes.py

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


Sorry I was not clearer.

I meant that we should only issue the warning if:

The user is not passing an explicit exc_type.

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?

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

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

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.

nicoddemus · 2024-04-20T13:12:52Z

@shekhuverma thanks for following up!

Took the liberty of adding more tests, docs, and change the implementation a bit.

@The-Compiler would appreciate another review.

nicoddemus · 2024-04-20T13:13:29Z

After this gets merged I will create an issue to track changing the default in 9.1.

shekhuverma · 2024-04-20T14:27:13Z

Thanks for the help @The-Compiler and @nicoddemus.
This was my first open-source contribution.
Please mark more issues that I can solve

shekhuverma · 2024-04-24T08:54:40Z

Hello there, what are the next steps?
when will it get merged to the main?

nicoddemus · 2024-04-24T16:05:39Z

The ball is on our side now @shekhuverma, waiting on @The-Compiler's review.

nicoddemus · 2024-04-24T16:06:14Z

(Note to ourselves, we need to squash merge this)

The-Compiler · 2024-04-25T08:32:34Z

Sorry for the delay! I plan to look at this today, now that PyConDE is over.

The-Compiler

Thanks @shekhuverma and @nicoddemus. I have a couple of remaining questions and comments (most of them minor, but one semantic thing that I think we should aim to get correct).

doc/en/deprecations.rst

The-Compiler · 2024-04-25T19:27:35Z

src/_pytest/outcomes.py

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


Why not call warning.warn(...) here directly, given that warning = ... is only set from here?

Because at this point we are inside the with warnings.catch_warnings(): block, which would catch our own warning.

The-Compiler · 2024-04-25T19:28:43Z

src/_pytest/outcomes.py

+    if warning:
+        warnings.warn(warning, stacklevel=2)
+    if skipped:
+        raise skipped


Same here, couldn't this be simplified by moving the raise Skipped(...) to directly after the warning?

As per #12220 (comment), we need to raise this after the warning. 👍

src/_pytest/outcomes.py

The-Compiler · 2024-04-25T19:31:49Z

testing/test_runner.py

+            with pytest.raises(pytest.skip.Exception):
+                pytest.importorskip(fn.stem)
+
+        [warning] = captured


I love this! Never seen it before, I've always done:

assert len(captured) == 1 warning = captured[0]

for this kind of thing.

All credits go to https://www.cosmicpython.com, seen this pattern there and have been using it ever since. 😁

The-Compiler · 2024-04-25T19:32:58Z

src/_pytest/outcomes.py

+    # 2. Remove `warn_on_import` and the warning handling.
+    if exc_type is None:
+        exc_type = ImportError
+        warn_on_import_error = True


I'm a bit confused by the Codecov annotations here. It looks to me like this (and the others) should be covered by tests just fine? Are the added tests not running or something? Or is codecov broken? Or am I just missing something?

Not sure, but this is definitely being covered by tests (and normal tests, not through pytester either). I even tested this locally by adding a hard assert 0 in this line to ensure it is hit by the tests, and it is (as expected).

Not sure what's going on.

Co-authored-by: Florian Bruhin <me@the-compiler.org>

shekhuverma · 2024-04-26T07:40:45Z

Can you please mark more related issues/improvements that I can try?
I tried searching in the issues section but failed to choose one.
Thanks

The-Compiler · 2024-04-26T07:50:31Z

Thanks again! 👍 We have the good first issue label, though indeed I think we haven't really been doing a good job at applying it to issues recently...

shekhuverma · 2024-04-26T08:01:25Z

Thanks again! 👍 We have the good first issue label, though indeed I think we haven't really been doing a good job at applying it to issues recently...

Yes, there is only one issue with that label now

With pytest 8.2, pytest.importorskip(...) now only considers ModuleNotFoundError rather than all ImportErrors, and warns otherwise: pytest-dev/pytest#12220 While we could override this via pytest.importorskip(..., exc_type=machinery.Unavailable) this is a simpler solution, and it also makes more sense semantically: We only raise Unavailable when an import is being done that would otherwise result in a ModuleNotFoundError anyways (e.g. trying to import QtWebKit on Qt 6).

Changed importError to ModuleNotFoundError

3a26211

nicoddemus mentioned this pull request Apr 17, 2024

importorskip variant that uses ModuleNotFoundError #11523

Closed

nicoddemus requested changes Apr 17, 2024

View reviewed changes

shekhuverma added 3 commits April 19, 2024 13:48

Merge branch 'pytest-dev:main' into main

3f80fd7

added testing for importorskip

fd288e1

Merge branch 'importorskip-ModuleNotFoundError'

1fe14e0

shekhuverma requested a review from nicoddemus April 19, 2024 11:14

The-Compiler requested changes Apr 19, 2024

View reviewed changes

nicoddemus requested changes Apr 19, 2024

View reviewed changes

shekhuverma added 2 commits April 19, 2024 19:26

added exc_types parameter in importorskip

543262c

Merge branch 'importorskip-ModuleNotFoundError'

1456b71

nicoddemus reviewed Apr 19, 2024

View reviewed changes

shekhuverma and others added 4 commits April 20, 2024 12:43

Added warning and Test Cases

9a9643d

Merge branch 'importorskip-ModuleNotFoundError'

3ca5036

Improve tests and docs

7de869d

Improve deprecation docs

3ca329b

nicoddemus requested a review from The-Compiler April 20, 2024 13:12

nicoddemus approved these changes Apr 20, 2024

View reviewed changes

nicoddemus added this to the 8.2 milestone Apr 20, 2024

Change exc_type to kw only

f14d78d

The-Compiler requested changes Apr 25, 2024

View reviewed changes

Apply suggestions from code review

b59336a

Co-authored-by: Florian Bruhin <me@the-compiler.org>

nicoddemus requested a review from The-Compiler April 25, 2024 21:05

nicoddemus and others added 2 commits April 25, 2024 20:59

Fix check

76fd0e5

Merge branch 'pytest-dev:main' into main

a55abeb

The-Compiler approved these changes Apr 26, 2024

View reviewed changes

The-Compiler merged commit 4eb8b6d into pytest-dev:main Apr 26, 2024
24 checks passed

The-Compiler mentioned this pull request Apr 30, 2024

Update dependencies qutebrowser/qutebrowser#8179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed importError to ModuleNotFoundError #12220

Changed importError to ModuleNotFoundError #12220

shekhuverma commented Apr 16, 2024

nicoddemus left a comment

The-Compiler left a comment

The-Compiler Apr 19, 2024

nicoddemus left a comment •

edited

nicoddemus Apr 19, 2024

nicoddemus commented Apr 19, 2024 •

edited

shekhuverma commented Apr 19, 2024

nicoddemus Apr 19, 2024

The-Compiler Apr 19, 2024

shekhuverma Apr 19, 2024

nicoddemus Apr 19, 2024

nicoddemus commented Apr 20, 2024

nicoddemus commented Apr 20, 2024

shekhuverma commented Apr 20, 2024

shekhuverma commented Apr 24, 2024

nicoddemus commented Apr 24, 2024

nicoddemus commented Apr 24, 2024

The-Compiler commented Apr 25, 2024

The-Compiler left a comment

The-Compiler Apr 25, 2024

nicoddemus Apr 25, 2024 •

edited

The-Compiler Apr 25, 2024

nicoddemus Apr 25, 2024

The-Compiler Apr 25, 2024

nicoddemus Apr 25, 2024

The-Compiler Apr 25, 2024

nicoddemus Apr 25, 2024 •

edited

shekhuverma commented Apr 26, 2024

The-Compiler commented Apr 26, 2024

shekhuverma commented Apr 26, 2024

Changed importError to ModuleNotFoundError #12220

Changed importError to ModuleNotFoundError #12220

Conversation

shekhuverma commented Apr 16, 2024

nicoddemus left a comment

Choose a reason for hiding this comment

The-Compiler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Apr 19, 2024 • edited

shekhuverma commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Apr 20, 2024

nicoddemus commented Apr 20, 2024

shekhuverma commented Apr 20, 2024

shekhuverma commented Apr 24, 2024

nicoddemus commented Apr 24, 2024

nicoddemus commented Apr 24, 2024

The-Compiler commented Apr 25, 2024

The-Compiler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus Apr 25, 2024 • edited

Choose a reason for hiding this comment

shekhuverma commented Apr 26, 2024

The-Compiler commented Apr 26, 2024

shekhuverma commented Apr 26, 2024

nicoddemus left a comment •

edited

nicoddemus commented Apr 19, 2024 •

edited

nicoddemus Apr 25, 2024 •

edited

nicoddemus Apr 25, 2024 •

edited