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

reenable pytest -p unraisableexception and -p threadexception #1898

Merged

Conversation

graingert
Copy link
Member

@graingert graingert commented Feb 11, 2021

by restoring the default unraisablehook, and disabling the threading.excepthook in tests that need it

@graingert graingert force-pushed the reenable-unraisable-thread-except-hooks branch from 04b2f56 to 624736d Compare February 11, 2021 11:24

@contextmanager
def disable_threading_excepthook():
threading.excepthook, prev = _noop, threading.excepthook
Copy link
Member Author

@graingert graingert Feb 11, 2021

Choose a reason for hiding this comment

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

sadly threading.__excepthook__ is only in 3.10, so I disable the hook rather than restore the old one

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 know... if we were already testing 3.10 then maybe it would be good practice to put an if in here to use threading.__excepthook__ when possible and then when we dropped 3.9 there would be a coverage loss and we'd remove the work around. But we aren't testing against 3.10... so... a comment? Or perhaps this is too far forward looking to be appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

Probably shouldn't hold this up. Maybe I'll add something in #1921.

@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #1898 (1caaa22) into master (ff86c60) will decrease coverage by 5.79%.
The diff coverage is 95.31%.

@@            Coverage Diff             @@
##           master    #1898      +/-   ##
==========================================
- Coverage   99.59%   93.80%   -5.80%     
==========================================
  Files         114      114              
  Lines       14574    14576       +2     
  Branches     1110     1113       +3     
==========================================
- Hits        14515    13673     -842     
- Misses         42      822     +780     
- Partials       17       81      +64     
Impacted Files Coverage Δ
trio/_core/tests/tutil.py 91.52% <83.33%> (-8.48%) ⬇️
trio/_core/tests/test_asyncgen.py 99.06% <100.00%> (+0.01%) ⬆️
trio/_core/tests/test_guest_mode.py 100.00% <100.00%> (ø)
trio/_core/tests/test_run.py 100.00% <100.00%> (ø)
trio/_core/tests/test_thread_cache.py 100.00% <100.00%> (ø)
trio/_core/tests/test_windows.py 99.22% <100.00%> (-0.78%) ⬇️
trio/_core/_io_epoll.py 0.00% <0.00%> (-100.00%) ⬇️
trio/_subprocess_platform/kqueue.py 0.00% <0.00%> (-100.00%) ⬇️
...tests/test_multierror_scripts/apport_excepthook.py 0.00% <0.00%> (-100.00%) ⬇️
trio/_unix_pipes.py 6.89% <0.00%> (-93.11%) ⬇️
... and 44 more

@graingert graingert force-pushed the reenable-unraisable-thread-except-hooks branch from c03e3a9 to 7b65819 Compare February 11, 2021 11:32
trio/_core/tests/tutil.py Outdated Show resolved Hide resolved
@graingert graingert force-pushed the reenable-unraisable-thread-except-hooks branch from e565569 to 6273b1e Compare February 17, 2021 11:52
@graingert graingert closed this Feb 17, 2021
@graingert graingert reopened this Feb 17, 2021
@pquentin
Copy link
Member

faulthandler_timeout = 60
markers = ["redistributors_should_skip: tests that should be skipped by downstream redistributors"]
junit_family = "xunit2"
filterwarnings = ["error"]
Copy link
Member

Choose a reason for hiding this comment

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

The -W error in ci.sh wouldn't be needed anymore, right?

Copy link
Member Author

@graingert graingert Mar 5, 2021

Choose a reason for hiding this comment

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

I'm pretty sure it's redundant now, but does no harm there

Copy link
Member

Choose a reason for hiding this comment

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

It'll be confusing when someone wants it off and changes one spot and it's still active. Or when they are looking to Trio as an example.

@@ -1,9 +0,0 @@
[tool:pytest]
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 prefer that unrelated restructures like this be handled separately. Just commenting, not part of the requested changes.

pyproject.toml Show resolved Hide resolved
trio/_core/tests/tutil.py Outdated Show resolved Hide resolved

@contextmanager
def disable_threading_excepthook():
threading.excepthook, prev = _noop, threading.excepthook
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 know... if we were already testing 3.10 then maybe it would be good practice to put an if in here to use threading.__excepthook__ when possible and then when we dropped 3.9 there would be a coverage loss and we'd remove the work around. But we aren't testing against 3.10... so... a comment? Or perhaps this is too far forward looking to be appropriate.

trio/_core/tests/tutil.py Show resolved Hide resolved
@graingert graingert closed this Mar 3, 2021
@graingert graingert reopened this Mar 3, 2021
@graingert graingert closed this Mar 5, 2021
@graingert graingert reopened this Mar 5, 2021
@graingert graingert requested a review from altendky March 5, 2021 16:57
@graingert graingert closed this Mar 5, 2021
@graingert graingert reopened this Mar 5, 2021
@altendky altendky mentioned this pull request Mar 6, 2021
7 tasks
faulthandler_timeout = 60
markers = ["redistributors_should_skip: tests that should be skipped by downstream redistributors"]
junit_family = "xunit2"
filterwarnings = ["error"]
Copy link
Member

Choose a reason for hiding this comment

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

It'll be confusing when someone wants it off and changes one spot and it's still active. Or when they are looking to Trio as an example.


@contextmanager
def disable_threading_excepthook():
threading.excepthook, prev = _noop, threading.excepthook
Copy link
Member

Choose a reason for hiding this comment

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

Probably shouldn't hold this up. Maybe I'll add something in #1921.

@altendky altendky merged commit 3188548 into python-trio:master Mar 6, 2021
@graingert graingert deleted the reenable-unraisable-thread-except-hooks branch March 6, 2021 23:39
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.

3 participants