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.

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

unraisablehook enhancements #12958

Merged
merged 38 commits into from
Nov 30, 2024

Conversation

graingert
Copy link
Member

@graingert graingert commented Nov 13, 2024

A number of unraisablehook plugin enhancements:

  • set the unraisablehook as early as possible and unset it as late as possible, to collect the most possible unraisable exceptions (please let me know if there's an earlier or later hook I can use)
  • call the garbage collector just before unsetting the unraisablehook, to collect any straggling exceptions
  • collect multiple unraisable exceptions per test phase
  • report the tracemalloc allocation traceback, if available!
  • avoid using a generator based hook to allow handling StopIteration in test failures
  • report the unraisable exception as the cause of the PytestUnraisableExceptionWarning exception if raised.
  • compute the repr of the unraisable.object in the unraisablehook so you get the latest information if available, and should help with resurrection of the object

A lot of these enhancements could also apply to the threading.excepthook plugin and I'll try to do that in a followup

Could help with #10404 it doesn't fix the issue yet because the warnings filter is removed before the session finishes, so any warnings that occur during session/config teardown are not reported. This PR improves the situation because it's now possible to configure warnings as errors by setting the PYTHONWARNINGS environment variable and ensure all unraisable exceptions are collected

It may also be a good idea to add a flag that calls gc.collect() at the end of every test phase to help associate errors with tests - but probably you don't want to run this every time as it impacts performance

@graingert
Copy link
Member Author

graingert commented Nov 13, 2024

# test_demo.py
import asyncio
import pytest

async def my_task():
    pass

def test_scheduler_must_be_created_within_running_loop() -> None:
    with pytest.raises(RuntimeError) as _:
        asyncio.create_task(my_task())

This is an improvement it works now if you do: python -W error -m pytest test_demo.py because there's an issue where warnings filters are removed too early

@graingert
Copy link
Member Author

These fixes probably need to be applied to the thread hooks

try:
import tracemalloc
except ImportError:
return ""
Copy link
Member Author

Choose a reason for hiding this comment

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

this should be covered on pypy

Copy link
Member

Choose a reason for hiding this comment

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

Leave that comment in the code? More likely for future maintainers to see it that way 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah what I mean to say is that the coverage check is failing for this branch, but it shouldn't and I don't know why

@graingert graingert marked this pull request as ready for review November 20, 2024 10:59
@graingert graingert requested a review from Zac-HD November 20, 2024 11:01
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Looks great overall! Some comments below, merge when you think it's ready.

try:
import tracemalloc
except ImportError:
return ""
Copy link
Member

Choose a reason for hiding this comment

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

Leave that comment in the code? More likely for future maintainers to see it that way 🙂

Comment on lines +229 to +230
# TODO: should be a test failure or error
assert result.ret == pytest.ExitCode.INTERNAL_ERROR
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to change this now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I don't know how

Copy link
Member

Choose a reason for hiding this comment

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

I haven't looked at the code at all, but maybe set a flag from the unraisable hook, check it where we choose the exit code, and exit-with-error if it's set?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I know how to do this, but I need to report the exception group traceback from pytest_session_finish then raise sys.exit(pytest.ExitCode.ERROR), then detect any absolutely straggling unraisables from the config callback and raise that as INTERNALERROR

I'm not sure how to do that though

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have a way to issue warnings at this late point.

I'm think that perhaps configure/unconfigure is not the best hooks for this, as they're too early/late. The purpose of the plugin is to catch unraisables from user test code, not from pytest internals. So maybe sessionstart/sessionfinish are the more appropriate hooks for setting up & catching stragglers. At this point we still have session which I think we can use to issue the straggler warnings/messages.

Copy link
Member Author

@graingert graingert Nov 28, 2024

Choose a reason for hiding this comment

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

We still want to catch unraisable exceptions caused by pytest internals and plugins. An object created in user code could stay referenced by pytest or a plugin and then only issue the unraisablehook call during the last teardown+ GC run

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks, these are nice improvements.

I left some comments.

src/_pytest/unraisableexception.py Outdated Show resolved Hide resolved
src/_pytest/unraisableexception.py Outdated Show resolved Hide resolved
src/_pytest/unraisableexception.py Show resolved Hide resolved
src/_pytest/unraisableexception.py Outdated Show resolved Hide resolved
src/_pytest/unraisableexception.py Outdated Show resolved Hide resolved
)
)
except BaseException as e:
_unraisable_exceptions.append(e)
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this an UnraisableMeta as well, perhaps preserving the original unraisable just without the extra info?

Copy link
Member Author

Choose a reason for hiding this comment

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

this BaseException handling needs to be as simple as possible - the failures here are the sorts of thing relating to missing globals and things. I've refactored to make this branch only load locals

src/_pytest/unraisableexception.py Show resolved Hide resolved
testing/test_unraisableexception.py Outdated Show resolved Hide resolved
@graingert
Copy link
Member Author

I just noticed that raising an exception in a callback to Config.add_cleanup doesn't allow subsequent cleanups to run. I'll fix this in another pr

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Nov 21, 2024
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Overall great work, left some minor comments.

changelog/12958.improvement.rst Outdated Show resolved Hide resolved
src/_pytest/unraisableexception.py Show resolved Hide resolved
src/_pytest/unraisableexception.py Outdated Show resolved Hide resolved
src/_pytest/unraisableexception.py Outdated Show resolved Hide resolved
src/_pytest/unraisableexception.py Outdated Show resolved Hide resolved
src/_pytest/unraisableexception.py Outdated Show resolved Hide resolved
src/_pytest/unraisableexception.py Outdated Show resolved Hide resolved
@nicoddemus
Copy link
Member

Thanks @graingert!

src/_pytest/unraisableexception.py Show resolved Hide resolved
@@ -36,7 +38,8 @@ def test_2(): pass
" ",
" Traceback (most recent call last):",
" ValueError: del is broken",
" ",
Copy link
Member

Choose a reason for hiding this comment

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

I would keep this empty line, I think it's a good delineation.

src/_pytest/unraisableexception.py Show resolved Hide resolved
Comment on lines +229 to +230
# TODO: should be a test failure or error
assert result.ret == pytest.ExitCode.INTERNAL_ERROR
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have a way to issue warnings at this late point.

I'm think that perhaps configure/unconfigure is not the best hooks for this, as they're too early/late. The purpose of the plugin is to catch unraisables from user test code, not from pytest internals. So maybe sessionstart/sessionfinish are the more appropriate hooks for setting up & catching stragglers. At this point we still have session which I think we can use to issue the straggler warnings/messages.

@nicoddemus nicoddemus merged commit 4b32e7c into pytest-dev:main Nov 30, 2024
28 checks passed
@nicoddemus
Copy link
Member

Thanks a lot @graingert!

@graingert graingert deleted the set-unraisable-hook-per-session branch November 30, 2024 21:22
@graingert
Copy link
Member Author

Here's the version for threading's hook: #13016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants