Add types to test_io, test_run, various other fixes
#2773
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2773 +/- ##
==========================================
- Coverage 98.94% 98.94% -0.01%
==========================================
Files 113 113
Lines 16870 16852 -18
Branches 3041 3032 -9
==========================================
- Hits 16692 16674 -18
Misses 123 123
Partials 55 55
|
| "withKnownType": 682, | ||
| "withKnownType": 680, |
There was a problem hiding this comment.
if checking manually (I realize now I removed generating full_output.json feel free to add that back if it helps), what's the removed symbols?
There was a problem hiding this comment.
It'd be the nursery.start() protocol, and its __call__ method.
There was a problem hiding this comment.
👍
We might want to keep a version of these json files around in the end after all, though just as the list of symbols. Kinda similar to what test_exports is doing but not quite. I'm not sure if dropping private symbols is ever really a problem, but could be good to keep track of it.
There was a problem hiding this comment.
Not really, as long as everything works (= tests pass). If it's private, by definition it can change whenever we want.
There was a problem hiding this comment.
Well, main reason would be checking the public types - the private types mostly just incidental but I don't think it hurts (at least if the updating of the file can be automated in pre-commit or so)
| self, async_fn: _NurseryStartFunc[StatusT], *args: object, name: object = None | ||
| self, | ||
| async_fn: Callable[..., Awaitable[object]], | ||
| *args: object, | ||
| name: object = None, |
There was a problem hiding this comment.
So this drops the check for it accepting a task_status parameter. If that's planned to be added back with ParamSpec/TypeVarTuple it probably warrants a #TODO explicitly mentioning that. Or is there a reason for dropping it?
There was a problem hiding this comment.
Yeah it needs TypeVarTuple. That protocol would only match callables that actually themselves use *args, which is not ideal. I got myself a little mixed up when trying to do it.
| "withKnownType": 682, | ||
| "withKnownType": 680, |
There was a problem hiding this comment.
Well, main reason would be checking the public types - the private types mostly just incidental but I don't think it hurts (at least if the updating of the file can be automated in pre-commit or so)
| nonlocal t1, t2 | ||
| t1 = _core.current_task() | ||
| print("child1 start") | ||
| x = await sleep_forever() | ||
| print("child1 woke") | ||
| assert x == 0 | ||
| print("child1 rescheduling t2") | ||
| _core.reschedule(t2, outcome.Error(ValueError())) | ||
| _core.reschedule(not_none(t2), outcome.Error(ValueError())) |
There was a problem hiding this comment.
Why not put the assert above this call? (similar for the one below)
Just a nitpick though, really.
There was a problem hiding this comment.
I added not_none() to try and distinguish between asserts used to test Trio, and ones used to indicate what should be happening here. If t2 never gets set, there's either something wrong with the test or I guess Trio's core internals.
| async def test_root_task(): | ||
| root = _core.current_root_task() | ||
| async def test_root_task() -> None: | ||
| root = not_none(_core.current_root_task()) |
There was a problem hiding this comment.
Similar to above comment.
I'm not commenting on test_current_task I feel like getting a None there would be less an issue with the current_task() but idk, maybe that too?
Adds types for two tests, and implements some TODOs.