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

@bluetech bluetech commented Nov 20, 2020

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.

bluetech added 2 commits Nov 20, 2020
If the user passed stdin=PIPE for some reason, they have no way to close
it themselves since it is not exposed.
@bluetech bluetech force-pushed the bluetech:unraisable branch from bef21d7 to 61c9123 Nov 20, 2020
Copy link
Member

@nicoddemus nicoddemus left a comment

Awesome work @bluetech!

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

This comment has been minimized.

@nicoddemus

nicoddemus Nov 21, 2020
Member

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

This comment has been minimized.

@bluetech

bluetech Nov 21, 2020
Author Member

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

This comment has been minimized.

@nicoddemus

nicoddemus Nov 21, 2020
Member

Great docs!

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

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Nov 21, 2020
Member

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

This comment has been minimized.

@bluetech

bluetech Nov 21, 2020
Author Member

I prefer to keep the context manager itself standalone from pytest stuff just in case it's useful on its own.

@bluetech bluetech force-pushed the bluetech:unraisable branch from 61c9123 to d50df85 Nov 21, 2020
@bluetech bluetech requested a review from graingert Nov 21, 2020
Copy link
Member

@The-Compiler The-Compiler left a comment

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

@bluetech bluetech commented Nov 25, 2020

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.

@bluetech bluetech merged commit e398c93 into pytest-dev:master Dec 5, 2020
20 checks passed
20 checks passed
build (windows-py36)
Details
build (windows-py37)
Details
build (windows-py37-pluggy)
Details
build (windows-py38)
Details
build (ubuntu-py36)
Details
build (ubuntu-py37)
Details
build (ubuntu-py37-pluggy)
Details
build (ubuntu-py37-freeze)
Details
build (ubuntu-py38)
Details
build (ubuntu-py39)
Details
build (ubuntu-pypy3)
Details
build (macos-py37)
Details
build (macos-py38)
Details
build (docs)
Details
build (doctesting)
Details
build (plugins)
Details
linting
Details
deploy
Details
codecov/patch 96.87% of diff hit (target 96.70%)
Details
docs/readthedocs.org:pytest Read the Docs build succeeded!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.