python-trio / trio

Trio – a friendly Python library for async concurrency and I/O
https://trio.readthedocs.io
Other
6.12k stars 335 forks source link

Add trio.testing.wait_all_threads_completed #2937

Closed VincentVanlaer closed 7 months ago

VincentVanlaer commented 8 months ago

Alternative to #2880

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (cd61e47) 99.64% compared to head (2c0eea9) 99.64%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2937 +/- ## ======================================= Coverage 99.64% 99.64% ======================================= Files 116 116 Lines 17521 17591 +70 Branches 3151 3157 +6 ======================================= + Hits 17459 17529 +70 Misses 43 43 Partials 19 19 ``` | [Files](https://app.codecov.io/gh/python-trio/trio/pull/2937?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio) | Coverage Δ | | |---|---|---| | [src/trio/\_tests/test\_threads.py](https://app.codecov.io/gh/python-trio/trio/pull/2937?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3Rlc3RzL3Rlc3RfdGhyZWFkcy5weQ==) | `100.00% <100.00%> (ø)` | | | [src/trio/\_threads.py](https://app.codecov.io/gh/python-trio/trio/pull/2937?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3RocmVhZHMucHk=) | `100.00% <100.00%> (ø)` | | | [src/trio/testing/\_\_init\_\_.py](https://app.codecov.io/gh/python-trio/trio/pull/2937?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vdGVzdGluZy9fX2luaXRfXy5weQ==) | `100.00% <100.00%> (ø)` | |
CoolCat467 commented 8 months ago

This looks pretty good, but one part I worry about is the incrementing and decrementing thread count system. As it stands I do think it would work, but I would feel a lot better if it were using a context manager instead. You could pretty easily make it a context manager with the following:

from contextlib import contextmanager
from collections.abc import Generator

@contextmanager
def handling_thread() -> Generator[None, None, None]:
    _increment_active_threads()
    yield
    _decrement_active_threads()

with handling_thread():
    # do all the things
    ...

Context managers are guaranteed to run their close methods, similar to a try finally block, but way nicer to work with.

Edit: For _ActiveThreadCount, I would also suggest using something like NamedTuple as well:

from typing import NamedTuple

class _ActiveThreadCount(NamedTuple):
    count: int = 0
    event: Event = Event()
VincentVanlaer commented 8 months ago

I believe I have addressed all comments. The codecov fail seems to be spurious (the diff is in lines with only comments in a complete unrelated file if I understand it correctly).

CoolCat467 commented 8 months ago

For future reference, force pushing makes it far more difficult to compare the previous version to new changes.

jakkdl commented 8 months ago

I believe I have addressed all comments. The codecov fail seems to be spurious (the diff is in lines with only comments in a complete unrelated file if I understand it correctly).

yeah the codecov can be weird sometimes, if there seems to be duplicates of codecov in the run list some may be stale and not getting updated / checking vs the wrong set of files / something like that.