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 type tests for all exported objects #2623

Closed
wants to merge 1 commit into from

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Mar 29, 2023

As a prelude towards deprecating trio-typing (see https://github.com/python-trio/trio-typing), and more work on #543 I'm adding the ability to test types with https://github.com/typeddjango/pytest-mypy-plugins (as suggested in python-trio/trio-typing#10).

Also adding tests for all currently exported objects that has some type annotations (note that those are not visible to users currently due to trio not being marked with py.typed though), and a living list of all remaining objects that are not typed which can be thinned out as more exports get typed - and if/when that gets emptied or thin enough we can retire trio-typing and mark trio as fully typed without having to have fully typed all the internals.

One could also copy this test over to trio-typing to see that the types are in accordance with the stubs (the stubtest added in python-trio/trio-typing#73 will mostly achieve that, but that's a separate CI), and/or set up a test environment here that installs the stubs from trio-typing and reruns tests.

I'm a bit unsure whether to remove the temporary py.typed file in check.sh - on one hand you want it there if you want to run pytest locally, but on the other hand it might be accidentally pushed or if you're running a separate application against the dev environment the existence of py.typed might mess with type checking. I'll probably change to removing it if it didn't exist before check.sh ran, and add a dedicated test that checks for the existence of it and gives a more helpful error message than what pytest-mypy-plugins will give when testing the yaml file. Or maybe it's possible to add some fancy extension hook or something https://github.com/typeddjango/pytest-mypy-plugins#options

@jakkdl jakkdl requested a review from Zac-HD March 29, 2023 13:34
@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #2623 (53addd0) into master (9ba894d) will increase coverage by 0.15%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2623      +/-   ##
==========================================
+ Coverage   92.27%   92.43%   +0.15%     
==========================================
  Files         118      118              
  Lines       16352    16352              
  Branches     3156     3156              
==========================================
+ Hits        15089    15115      +26     
+ Misses       1146     1114      -32     
- Partials      117      123       +6     

see 12 files with indirect coverage changes

@jakkdl jakkdl force-pushed the pytyped branch 3 times, most recently from 950722b to 4edfd3d Compare March 29, 2023 14:01
@jakkdl
Copy link
Member Author

jakkdl commented Mar 29, 2023

oh goodness generating requirements.txt is a nightmare. I may have finally succeeded after the nth try, but github seems to be struggling atm. It may be possible to freeze some version somewhere that supports 3.7 to not need to remove --strict-config and manage to install pytest-mypy-plugins on 3.7, but that's for another day.

@Fuyukai
Copy link
Member

Fuyukai commented Mar 29, 2023

Oh my god. When did Python get to the point of testing type annotations?????????

I'm no authority but if you are ever at the point of having to run tests for types you have done something very very wrong. Absolutely, vehemently against this.

@A5rocks
Copy link
Contributor

A5rocks commented Mar 29, 2023

I don't quite understand the point of this PR FWIW. I can see two potential points, though:

  1. We want to ensure the types match runtime.

This is the explicit goal of things like stubtest (which ships with mypy): https://mypy.readthedocs.io/en/stable/stubtest.html

The reason why it's explicitly called stubtest (and not, say, typetest) is because the goal of types is such that if you are fully typed, the types should roughly match behavior at runtime and be guaranteed to do so without tests!

  1. We want to make sure trio is fully typed.

I think pyright's "type completeness" score is a better metric of this (it also checks some more stuff like "are we relying on mypy's inference behavior" (which isn't specified to work for all type checkers)), though I'm not sure how to make sure we only increase it.

See https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#type-completeness (and https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#verifying-type-completeness for checking it).


Basically, would you mind elaborating on the point of this PR? I might be missing something!

@TeamSpen210
Copy link
Contributor

Indeed writing tests for all the annotations isn't really useful. Typeshed does have tests, but only for a select few things. It's used either for complicated functions like round() that need to be checked to confirm the annotations do the right thing, or for cases where bugs have been found previously.

For doing type tests, there's actually now a standard way to do tests that will work with both mypy and pyright. Simply use assert_type() in a script to specify the type that is expected. To verify that a line of code produces an error, # type: ignore it, then use --warn-unused-ignores to have mypy produce an error if it in fact succeeded.

@jakkdl
Copy link
Member Author

jakkdl commented Mar 30, 2023

  1. Ah, I did not know about pyright --verifytypes! That indeed does what most of this PR attempts to do.

  2. stubtest cannot be used (afaik) if you don't have stubs - but I added it in Add stubtest, fix incorrect stubs trio-typing#73 to check the stubs there. But at the point of considering removing trio-typing and adding py.typed to trio one could check with stubtest against it and see where/how it differs.

  3. assert_type and type: ignore is clever! And can indeed be used for testing Add generic typing support for Memory[Send/Receive]Channel #2549

Thank you for the constructive comments! I'll close this and rethink the approach :)

@jakkdl jakkdl closed this Mar 30, 2023
@jakkdl
Copy link
Member Author

jakkdl commented Mar 30, 2023

Turns out --verifytypes is not compatible with the way trio exports stuff, opened microsoft/pyright#4869. Unless that's fixed this'd either require reintroducing __all__ in __init__.pys #594 (comment) or entirely upending the current structure.

It would likely also warrant moving trio/tests/ to trio/_tests #274

reopened in case of further discussion

@jakkdl jakkdl reopened this Mar 30, 2023
@Fuyukai
Copy link
Member

Fuyukai commented Mar 30, 2023

or entirely upending the current structure.

I support this. Kill _core forever. But I'm still against testing types.

@jakkdl
Copy link
Member Author

jakkdl commented Apr 11, 2023

alternative ways of solving this are superior and I'm making decent progress on them.

@jakkdl jakkdl closed this Apr 11, 2023
@jakkdl jakkdl deleted the pytyped branch April 11, 2023 08: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.

4 participants