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

Add unraisableexception and threadexception plugins #8055

Merged
merged 3 commits into from Dec 5, 2020

Conversation

bluetech
Copy link
Member

Fixes #5299.

Please see the added documentation for details. The first two commits just fix a couple of issues this found in our own testsuite.

While the two plugins are very similar, I chose to keep them separate because I think it's cleaner and so they may be disabled separately.

Performance-wise this adds about 3% to the pure pytest overhead, which seems fine.

If the user passed stdin=PIPE for some reason, they have no way to close
it themselves since it is not exposed.
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.

Awesome work @bluetech!

doc/en/usage.rst Outdated
because they don't cause the program itself to crash. Pytest detects these
conditions and issues a warning that is visible in the test run summary.

The modules are automatically enabled for pytest runs, unless the
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean?

Suggested change
The modules are automatically enabled for pytest runs, unless the
The warnings are automatically enabled for pytest runs, unless the

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 copy the phrasing from the faulthandler section just above this one, but I agree it's not great. I don't think we should use "warnings" here because enabling warnings is a separate thing. I'll change this to "plugins".

@@ -470,6 +470,38 @@ seconds to finish (not available on Windows).
the command-line using ``-o faulthandler_timeout=X``.


.. _unraisable:

Warning about unraisable exceptions and unhandled thread exceptions
Copy link
Member

Choose a reason for hiding this comment

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

Great docs!

def unraisable_exception_runtest_hook() -> Generator[None, None, None]:
with catch_unraisable_exception() as cm:
yield
if cm.unraisable:
Copy link
Member

Choose a reason for hiding this comment

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

would it be sensible to move this into the context manager, then the yield from generator hack wouldn't have to be used

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 prefer to keep the context manager itself standalone from pytest stuff just in case it's useful on its own.

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

There seems to be quite some code duplication between the two plugins - do they really need to be separate? I think I'd prefer having them in one plugin and sharing more code.

@bluetech
Copy link
Member Author

There seems to be quite some code duplication between the two plugins - do they really need to be separate?

The certainly look similar, but the similarity is just structural, not logical. IMO it would be wrong to couple them -- they couldn't be disabled separately, and I suspect will cause maintainability issues if new requirements ever come up, a process which is nicely described in this article and is pretty accurate from my experience.

@dstansby
Copy link

dstansby commented Dec 9, 2020

Thanks for the work on this, looking forward to making use of it! Does anyone know which pytest release this is likely to be released in?

@nicoddemus
Copy link
Member

It will go in 6.2, which we plan to release this Saturday! 🎉

#8101

@bluetech bluetech deleted the unraisable branch December 12, 2020 12:16
@xuhdev
Copy link
Contributor

xuhdev commented Dec 14, 2020

Could someone explain what this new feature means? I read the document of PytestUnraisableExceptionWarning but still have no idea what it means:

Unraisable exceptions are exceptions raised in __del__ implementations and similar situations when the exception cannot be raised as normal.

Thanks!

@bluetech
Copy link
Member Author

Here is an example from the tests:

        class BrokenDel:
            def __del__(self) -> None:
                raise ValueError("del is broken")

        def test_it() -> None:
            obj = BrokenDel()
            del obj

Previously the ValueError would go unnoticed, since the test does pass and the output is captured. Now pytest issues a PytestUnraisableExceptionWarning warning for this test, so you can better notice the ValueError problem.

@flyser
Copy link

flyser commented Dec 20, 2020

Is there a way to propagate this warning to an error to let the tests fail?

@graingert
Copy link
Member

graingert commented Dec 20, 2020

@flyser see https://docs.pytest.org/en/stable/warnings.html#warnings-capture

you can set:

# pyproject.toml
[tool.pytest.ini_options]
filterwarnings = ["error"]

I like to use the most strict config I can:

# pyproject.toml
[tool.pytest.ini_options]
addopts = ["--strict-config", "--strict-markers"]
filterwarnings = ["error"]
xfail_strict = true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set sys.unraisablehook (py38)
8 participants