Type all remaining tests #2820
Type all remaining tests #2820
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2820 +/- ##
==========================================
- Coverage 99.18% 99.18% -0.01%
==========================================
Files 115 115
Lines 17466 17585 +119
Branches 3140 3140
==========================================
+ Hits 17324 17442 +118
- Misses 97 99 +2
+ Partials 45 44 -1
|
…py and missing type parameters for SSLStream
trio/_core/_tests/test_guest_mode.py
Outdated
| @@ -322,18 +345,18 @@ async def get_woken_by_host_deadline(watb_cscope): | |||
| # 'sit_in_wait_all_tasks_blocked', we want the test to | |||
| # actually end. So in after_io_wait we schedule a second host | |||
| # call to tear things down. | |||
| class InstrumentHelper: | |||
| def __init__(self): | |||
| class InstrumentHelper(trio._abc.Instrument): | |||
There was a problem hiding this comment.
Probably should import using a public name.
There was a problem hiding this comment.
Yeah I wanted to consistently do this, but I was inconsistent a bunch of times when everything else in a test did relative imports. Other than when it's required because a symbol isn't exported, is there any reason not to do all imports from the public name? If so is there a linter warning for it?
| # make_tree creates cycles itself, so a simple | ||
| raise MultiError([get_exc(raiser1), get_exc(raiser2)]) | ||
|
|
||
| def simple_filter(exc): | ||
| def simple_filter(exc: BaseException) -> Exception | RuntimeError: |
There was a problem hiding this comment.
Exception | RuntimeError is redundant.
There was a problem hiding this comment.
yeah, I did it intentionally to mirror the function body. I realize now the proper way to do it would maybe be (exc: T_Exception) -> T_Exception | RunTimeError, but that's kinda overkill.
trio/_tests/test_file_io.py
Outdated
| raw = io.BytesIO() | ||
| buffered = io.BufferedReader(raw) | ||
| buffered = io.BufferedReader(raw) # type: ignore[arg-type] # ???????????? |
There was a problem hiding this comment.
Okay, I think the reason is that BufferedReader wraps a unbuffered raw IO object, but BytesIO is already buffered - BufferedIOBase and RawIOBase are sibling classes, they don't share hierarchy. Maybe swap this to use tmp_path and open an unbuffered file?
There was a problem hiding this comment.
Or well, getting ASYNC101 for opening a file in an async function, so one might possibly want to do a more async way of opening it.
|
How do we fix the following errors
|
|
All of the |
|
yeah I intentionally don't bother in large part because I expect it to get autofixed, but also doesn't really matter. |
|
hrm, pre-commit.ci is running ruff on 0.1.1 whereas check.sh is running ruff with 0.1.0 - this feels destined to bring a conflict at some point. |
I quite agree, we should update that soon. |
|
In reference to commit 6170384, if you look at the pre-commit run in the PR checks area, at the very bottom of the page it displays, it says
|
|
I'll merge this soonish unless anybody has something to comment on, because this touches so much it very easily get merge conflicts. (e.g. other people adding test cases that aren't typechecked without this PR) |
| in_host_after_start: Callable[[], None] | None = None, | ||
| **start_guest_run_kwargs: Any, | ||
| ) -> T: | ||
| todo: queue.Queue[tuple[str, Outcome[T] | Callable[..., object]]] = queue.Queue() |
There was a problem hiding this comment.
Probably way over the top but this could have a literal of strings and use typing.assert_never. But that's like too much for a simple test (which we know will either pass or fail based on a local run whenever we make changes so not sure exhaustive checks is really necessary).
| @@ -450,7 +483,11 @@ async def trio_main(): | |||
| aio_task.cancel() | |||
| return "trio-main-done" | |||
|
|
|||
| async def aio_pingpong(from_trio, to_trio): | |||
| raise AssertionError("should never be reached") # pragma: no cover | |||
There was a problem hiding this comment.
I don't think this is necessary... unless mypy forces you to use explicit returns? async def trio_main() -> str | None.
There was a problem hiding this comment.
-> str|None would still give "missing return statement", if you have an explicit return somewhere in the function it wants explicit returns in all branches.
| class TaskRecorder: | ||
| record = attr.ib(factory=list) | ||
| class TaskRecorder(_abc.Instrument): | ||
| record: list[tuple[str, Task | None]] = attr.ib(factory=list) |
There was a problem hiding this comment.
I think you can keep the variable length tuples and just use tuple length to differentiate (I believe that narrows in mypy? If not, ignore this comment!)
Specifically list[tuple[str] | tuple[str, Task]]
There was a problem hiding this comment.
It does as of last week - python/mypy#16237. So not quite yet.
There was a problem hiding this comment.
ye not gonna bother waiting for that, so I'll leave the changes as is.
|
|
||
| # Changing the set of hooks implemented by an instrument after | ||
| # it's installed doesn't make them start being called right away | ||
| instrument.before_task_step = record.append | ||
| instrument.before_task_step = record.append # type: ignore[assignment] |
There was a problem hiding this comment.
I'm not sure why this is type erroring?
There was a problem hiding this comment.
trio/_core/_tests/test_instrumentation.py:222: error: Cannot assign to a method [method-assign]
trio/_core/_tests/test_instrumentation.py:222: error: Incompatible types in assignment (expression has type "Callable[[Task], None]", variable has type "Callable[[Arg(Task, 'task')], None]") [assignment]
If I edit _abc.Instrument.before_task_step to make task positional-only then I only get the first error. Seems like ignore[assignment] covers both, but made it extra explicit by ignoring both, and added comment that explains the errors.
| @_core.enable_ki_protection | ||
| @async_generator | ||
| async def agen_protected1(): | ||
| @async_generator # type: ignore[misc] # untyped generator |
There was a problem hiding this comment.
Just checking understanding, this is cause async_generator isn't typed?
There was a problem hiding this comment.
yeah, adding a comment.
| @@ -488,21 +489,22 @@ async def main(): | |||
|
|
|||
|
|
|||
| # Regression test for #461 | |||
| def test_ki_with_broken_threads(): | |||
| # don't know if _active not being visible is a problem | |||
There was a problem hiding this comment.
Probably more something to bring up with typeshed? But it's not documented AFAICT so probably intentional.
There was a problem hiding this comment.
Hmm, it's only _active and not __active, so I'm pretty sure a lot of code actually inspects it or modifies it in a subclass, but yeah makes sense that typeshed sticks to the documented parts.
| def check_sequence_matches( | ||
| seq: Sequence[object], template: Iterable[object | set[object]] | ||
| ) -> None: | ||
| def check_sequence_matches(seq: Sequence[T], template: Iterable[T | set[T]]) -> None: |
There was a problem hiding this comment.
Hm. I'm not sure I quite follow why the T here -- but I don't remember the variance of Iterable so maybe this makes sense... Though I guess maybe for set.... aaa whatever, might as well ask.
There was a problem hiding this comment.
I.. don't quite follow why the signature would be confusing? Though the comment above reversing them might be the cause of confusion.
You pass it a sequence of some thing, e.g. [1, 2, 3, 4, 5] or [1, 2, 3, 5, 4], and use this function to see whether that matches a template - which can include either objects of the same type, or if you expect the order to sometimes be jumbled you can specify them inside a set*: [1, 2, 3, {4, 5}]. So the template should be an Iterable which contains objects of the same type your sequence consists of, or sets of that object. Variance is not related in any way.
The Sequence vs Iterable is because the function does slicing on seq, but only iterates over template.
* I did not expect this to be how the function actually worked, and wrote an incorrect description at first where I assumed the sets were about unions. Not a great name imho.
Improved the comments accompanying the function.
There was a problem hiding this comment.
Well the T w/ Sequence (covariant) means that something like this would pass typechecking: check_sequence_matches([5.0, 4.0], [5, 4]) but I was in fact being blind to the fact that object has that problem too.
There was a problem hiding this comment.
Ah, huh. Yeah I don't think we really care about that level of precision, as it's only really used in a few tests and not a public function. I need to get better at variance, for sure.
trio/_highlevel_ssl_helpers.py
Outdated
| Args: | ||
| host (bytes or str): The host to connect to. We require the server | ||
| host (str): The host to connect to. We require the server |
There was a problem hiding this comment.
Does this passing bytes error at runtime?
trio/_subprocess_platform/waitid.py
Outdated
| assert (sys.platform != "win32" and sys.platform != "darwin") or not TYPE_CHECKING | ||
| if TYPE_CHECKING: | ||
|
|
||
| def sync_wait_reapable(pid: int) -> None: |
There was a problem hiding this comment.
Mind leaving a comment about why this function specifically is up here?
There was a problem hiding this comment.
I think it's to provide a definition if imported on other platforms.
There was a problem hiding this comment.
It's for trio/_tests/test_subprocess.py:test_waitid_eintr():
@slow
def test_waitid_eintr() -> None:
# This only matters on PyPy (where we're coding EINTR handling
# ourselves) but the test works on all waitid platforms.
from .._subprocess_platform import wait_child_exiting
if TYPE_CHECKING and sys.platform == "win32":
return
if not wait_child_exiting.__module__.endswith("waitid"):
pytest.skip("waitid only")
from .._subprocess_platform.waitid import sync_wait_reapable
...where despite having a
if TYPE_CHECKING and sys.platform == "win32": return it will still complain about the import on line 522. I guess this one could be reported upstream?
Though I realize now it's probably better to silence the error in the test than to make people think they can import sync_wait_reapable globally on all platforms only to get an ImportError at runtime.
trio/_subprocess_platform/waitid.py
Outdated
| assert (sys.platform != "win32" and sys.platform != "darwin") or not TYPE_CHECKING | ||
| if TYPE_CHECKING: | ||
|
|
||
| def sync_wait_reapable(pid: int) -> None: |
There was a problem hiding this comment.
I think it's to provide a definition if imported on other platforms.
|
pre-commit.ci autofix |
…pe: ignore to the correct line after incorrect autofix by black
Decided to do a speedrun through all the remaining test files, with the help of https://github.com/JelleZijlstra/autotyping
If you see anything incorrect, you're very welcome to edit it straight away - I didn't put a lot of effort on average into the type hints, so there's probably a decent amount of them that are bad in some way or another.