[code coverage confusion] `__class_getitem__` in classes below 3.9 is not covered

Question

[code coverage confusion] `__class_getitem__` in classes below 3.9 is not covered

webknjaz opened this issue 9 months ago · 29 comments

Looking at the coverage report posted on Codecov (and what's visible locally), it says that https://app.codecov.io/github/aio-libs/frozenlist/commit/d92751c796094f6d4afecad91b65ec98a3bb9201/blob/frozenlist/__init__.py#L26 and https://app.codecov.io/github/aio-libs/frozenlist/commit/d92751c796094f6d4afecad91b65ec98a3bb9201/blob/frozenlist/_frozenlist.pyx#L13 aren't covered.
These lines are effectively the only ones in the main frozenlist package that are marked as not covered.

Any ideas why that is? cc @mjpieters @Dreamsorcerer @bdraco

I was hoping that maybe MyPy would cover it, but it doesn't either. I'm quite puzzled. FWIW, Codecov now displays MyPy coverage, which reveals some partial typing that needs to be looked at too.

Answer 1 · 2023-12-15T14:52:28.000Z

Any ideas why that is?

I don't see anything in the tests that covers that, so it must be from mypy. Mypy is only run on the latest version of Python, so it won't type check the other case (there are open issues requesting it to check all platforms/versions in a single run).

Codecov now displays MyPy coverage

I'm not sure this is a good idea, as it will surely just make it harder to see what actually has runtime testing.
With the settings we have for mypy, I think it should be guaranteed that mypy will always have 100% coverage (for the platform and python version it has been run on).

Answer 2 · 2023-12-15T14:57:25.000Z

I don't see anything in the tests that covers that

Note that a runtime test would just be adding an annotation somewhere in the tests (or trying to instantiate it): foo = Frozenlist[str]()
As long as it doesn't error at runtime, the behaviour is probably correct.

Answer 3 · 2023-12-15T18:09:32.000Z

Mypy is only run on the latest version of Python, so it won't type check the other case

Not anymore, I added runs against a number of versions @ https://github.com/aio-libs/frozenlist/blob/b5ffcd7/.pre-commit-config.yaml#L161-L223. That's why I found it surprising.
My working theory is that MyPy somehow doesn't even look into .py when there's a separate *.pyi. And it obviously can't lurk into the .pyx module...

Answer 4 · 2023-12-15T18:13:21.000Z

Any ideas why that is?

I don't see anything in the tests that covers that, so it must be from mypy. Mypy is only run on the latest version of Python, so it won't type check the other case (there are open issues requesting it to check all platforms/versions in a single run).

Codecov now displays MyPy coverage

I'm not sure this is a good idea, as it will surely just make it harder to see what actually has runtime testing. With the settings we have for mypy, I think it should be guaranteed that mypy will always have 100% coverage (for the platform and python version it has been run on).

Apparently, that's not the case 🤷‍♂️
Also, I think we should be able to set up flag-based distinction in Codecov (I currently made it folder-based). And specifically, for runtime, we could make use of https://hynek.me/articles/ditch-codecov-python/ that features merging the runtime coverage and verifying it against the target metric.

So I think that uploading all coverage is useful, but might need some tweaks.

Answer 5 · 2023-12-15T18:14:51.000Z

So I think that uploading all coverage is useful, but might need some tweaks.

As long as the end result is that I can see the runtime coverage at a glance in the codecov comments, then I'm good.

Answer 6 · 2023-12-15T18:15:12.000Z

Note that a runtime test would just be adding an annotation somewhere in the tests (or trying to instantiate it): foo = Frozenlist[str]()
As long as it doesn't error at runtime, the behaviour is probably correct.

I think this likely won't work under Python 3.8, unless we call that dunder method directly. Also, initializing an object is probably unneeded and Frozenlist[str] would do the trick, when it works, right?

Answer 7 · 2023-12-15T18:16:44.000Z

So I think that uploading all coverage is useful, but might need some tweaks.

As long as the end result is that I can see the runtime coverage at a glance in the codecov comments, then I'm good.

That needs experimentation. Although, I'll postpone this for now as there's Multidict that I haven't updated yet — it's the last urgent bit people really want a new release for.

Answer 8 · 2023-12-15T18:18:59.000Z

@Dreamsorcerer are you familiar with the context flags on Codecov? Look at https://app.codecov.io/gh/aio-libs/frozenlist/blob/master/tests%2Ftest_frozenlist.py — there's "All flags" in the top right corner. It allows distinguishing what contexts different lines were tested in. It's match more granular compared to run-time vs. typechecking-time.

Answer 9 · 2023-12-15T18:38:43.000Z

@Dreamsorcerer so this is how it breaks under Python 3.8.18:

========================================================================== FAILURES ==========================================================================
___________________________________________________________ TestFrozenList.test___class_getitem__ ____________________________________________________________

self = <test_frozenlist.TestFrozenList object at 0x7f1782bafca0>

    def test___class_getitem__(self) -> None:
>       assert self.FrozenList[str] is not None
E       TypeError: __class_getitem__() takes exactly 0 positional arguments (1 given)

self       = <test_frozenlist.TestFrozenList object at 0x7f1782bafca0>

tests/test_frozenlist.py:17: TypeError
__________________________________________________________ TestFrozenListPy.test___class_getitem__ ___________________________________________________________

self = <test_frozenlist.TestFrozenListPy object at 0x7f1782b9d430>

    def test___class_getitem__(self) -> None:
>       assert self.FrozenList[str] is not None
E       TypeError: __class_getitem__() takes 1 positional argument but 2 were given

self       = <test_frozenlist.TestFrozenListPy object at 0x7f1782b9d430>

tests/test_frozenlist.py:17: TypeError

Answer 10 · 2023-12-15T18:43:00.000Z

Oh, there's an actual bug in the method signature — it should accept (cls, item) but ours only accepts (cls) 🤦‍♂️

Answer 11 · 2023-12-15T18:52:09.000Z

Also, initializing an object is probably unneeded and Frozenlist[str] would do the trick, when it works, right?

Well, initialising Frozenlist[str] should still work at runtime. i.e. Someone can do foo = Frozenlist[str]() to have mypy infer the type correctly without needing to duplicate it (i.e. foo: Frozelist[str] = Frozenlist()).

Answer 12 · 2023-12-15T18:54:28.000Z

Yeah, it does work. Although, MyPy will never hit it for as long as the .pyi file exists. Adding it to tests fixes the coverage gap.

Answer 13 · 2023-12-15T19:01:05.000Z

Hopefully, #571 will address this.

Answer 14 · 2023-12-15T19:04:23.000Z

are you familiar with the context flags on Codecov?

Not familiar, but I've seen it in passing. I guess the question is whether one flag can be set as the default and have all missing/partials reported in the codecov comment.

Yeah, it does work. Although, MyPy will never hit it for as long as the .pyi file exists. Adding it to tests fixes the coverage gap.

~~I don't think you ran mypy on the frozenlist directory:~~
~~https://github.com/aio-libs/frozenlist/blob/master/.pre-commit-config.yaml#L220-L222~~

~~I'm pretty sure it should check all .py files, the .pyi files should be used when checking other projects.~~

I'd also note this doesn't really match our configs elsewhere. We normally have all the files and config options already defined in the config, so the CI doesn't need any arguments to run:
https://github.com/aio-libs/aiohttp/blob/master/.mypy.ini#L2

Answer 15 · 2023-12-15T19:06:42.000Z

Nevermind. Not sure what's happening there. Could try explicitly adding the init file to the file list and see if that gets it to type check.

Answer 16 · 2023-12-15T19:10:00.000Z

I'd also note this doesn't really match our configs elsewhere. We normally have all the files and config options already defined in the config

You're right that it's a good idea to put the CLI flags into the mypy.ini config. But it's not correct that all other projects do this. I think, it's just aiohttp at this point — I've been making synchronous changes to both yarl and frozenlist, and they don't. I think that as I unify some of the configurations, it'll be easier to make these changes. And I agree it's a good idea in general.
The reason it's not there currently, is that I was copying some configs from external places that are set up like this.

Answer 17 · 2023-12-15T19:14:09.000Z

Nevermind. Not sure what's happening there. Could try explicitly adding the init file to the file list and see if that gets it to type check.

That's an interesting idea. Could work! The good news is that #571 does give us coverage we need.
Though, I still don't understand why most of the *.pyi lines report partial coverage: https://app.codecov.io/gh/aio-libs/frozenlist/pull/571/blob/frozenlist/__init__.pyi.

Answer 18 · 2023-12-15T19:16:48.000Z

But it's not correct that all other projects do this.

I probably updated most of them, so I think nearly all aiohttp-* libraries are doing it, plus a few like aiocache. multidict is also doing it, but appears to only be checking 2 test files currently.

Answer 19 · 2023-12-15T19:20:14.000Z

But it's not correct that all other projects do this.

I probably updated most of them, so I think nearly all aiohttp-* libraries are doing it, plus a few like aiocache. multidict is also doing it, but appears to only be checking 2 test files currently.

Yeah, I've been noticing incomplete commands all over the place. And we also never collected Cython coverage which I now implemented in these both projects.

Answer 20 · 2023-12-15T19:24:48.000Z

Though, I still don't understand why most of the *.pyi lines report partial coverage

My assumption is that because it is a stubs file, mypy doesn't check the body of the functions (if it did, it would produce an error due to not returning anything). You could probably use covdefaults or whatever to ignore ... lines, or else disable partials in .pyi files.

Answer 21 · 2023-12-15T19:25:59.000Z

Okay, I'll look into that later, maybe.

Answer 22 · 2023-12-15T19:27:23.000Z

I suspect with covdefaults you may need to reformat them into 2 lines, so it simply ignores the second line:

def function():
    ...

Answer 23 · 2023-12-15T19:29:05.000Z

Ah, it didn't occur to me that the uncovered part is actually the method body!

Answer 24 · 2023-12-15T20:09:59.000Z

So adding frozenlist/__init__.py to the CLI args breaks it:

$ pre-commit run mypy-py38 --all-files -v
MyPy, for Python 3.8.....................................................Failed
- hook id: mypy
- duration: 0.21s
- exit code: 2

Generated Cobertura report: ~/src/github/aio-libs/frozenlist/.tox/.tmp/.mypy/python-3.8/cobertura.xml
Generated HTML report (via XSLT): ~/src/github/aio-libs/frozenlist/.tox/.tmp/.mypy/python-3.8/index.html
Generated TXT report (via XSLT): ~/src/github/aio-libs/frozenlist/.tox/.tmp/.mypy/python-3.8/index.txt
frozenlist/__init__.py: error: Duplicate module named "frozenlist" (also at
"frozenlist/__init__.pyi")
frozenlist/__init__.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
frozenlist/__init__.py: note: Common resolutions include: a) using `--exclude` to avoid checking one of them, b) adding `__init__.py` somewhere, c) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)

But adding a separate MyPy invocation with just that, does perform type checking. Although, it emits a lot of errors because that file doesn't have annotations.

$ pre-commit run mypy-py38-init --all-files -v 
MyPy, for Python 3.8.....................................................Failed
- hook id: mypy
- duration: 0.99s
- exit code: 1

Generated Cobertura report: ~/src/github/aio-libs/frozenlist/.tox/.tmp/.mypy/python-3.8/cobertura.xml
Generated HTML report (via XSLT): ~/src/github/aio-libs/frozenlist/.tox/.tmp/.mypy/python-3.8/index.html
Generated TXT report (via XSLT): ~/src/github/aio-libs/frozenlist/.tox/.tmp/.mypy/python-3.8/index.txt
frozenlist/__init__.py:10:1: error: Name "Tuple" is not defined  [name-defined]
    __all__ = ("FrozenList", "PyFrozenList")  # type: Tuple[str, ...]
    ^
frozenlist/__init__.py:10:1: note: Did you forget to import it from "typing"? (Suggestion: "from typing import Tuple")
frozenlist/__init__.py:17:18: error: Missing type parameters for generic type
"MutableSequence"  [type-arg]
    class FrozenList(MutableSequence):
                     ^
frozenlist/__init__.py:31:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __init__(self, items=None):
        ^
frozenlist/__init__.py:40:5: error: Function is missing a return type
annotation  [no-untyped-def]
        def frozen(self):
        ^
frozenlist/__init__.py:43:5: error: Function is missing a return type
annotation  [no-untyped-def]
        def freeze(self):
        ^
frozenlist/__init__.py:43:5: note: Use "-> None" if function does not return a value
frozenlist/__init__.py:46:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __getitem__(self, index):
        ^
frozenlist/__init__.py:49:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __setitem__(self, index, value):
        ^
frozenlist/__init__.py:54:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __delitem__(self, index):
        ^
frozenlist/__init__.py:59:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __len__(self):
        ^
frozenlist/__init__.py:62:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __iter__(self):
        ^
frozenlist/__init__.py:65:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __reversed__(self):
        ^
frozenlist/__init__.py:68:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __eq__(self, other):
        ^
frozenlist/__init__.py:71:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __le__(self, other):
        ^
frozenlist/__init__.py:74:5: error: Function is missing a type annotation 
[no-untyped-def]
        def insert(self, pos, item):
        ^
frozenlist/__init__.py:79:5: error: Function is missing a type annotation 
[no-untyped-def]
        def __repr__(self):
        ^
frozenlist/__init__.py:82:5: error: Function is missing a return type
annotation  [no-untyped-def]
        def __hash__(self):
        ^
frozenlist/__init__.py:94: error: "type: ignore" comment without error code
(consider "type: ignore[import-not-found]" instead)  [ignore-without-code]
            from ._frozenlist import FrozenList as CFrozenList  # type: ig...
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
frozenlist/__init__.py:98: error: "type: ignore" comment without error code
(consider "type: ignore[misc]" instead)  [ignore-without-code]
            FrozenList = CFrozenList  # type: ignore
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Found 18 errors in 1 file (checked 1 source file)

I remembered what I did for sphinxcontrib.towncrier that uses setuptools-scm and so it imports a module that is not always there — I made a .pyi file just for that version module.

I think this would work here too — we need to make a _frozenlist.pyi stub, move all the annotations from __init__.pyi to __init__.py and delete that stub. This should give us the desired outcome of linting what we care about most.

Answer 25 · 2023-12-15T20:44:30.000Z

You could probably use covdefaults or whatever to ignore ... lines, or else disable partials in .pyi files.

So I realized that covdefaults won't work simply because it's a plugin for coverage.py and MyPy doesn't use coverage.py. And using .coveragerc to ignore these won't work for exactly the same reason.

Answer 26 · 2023-12-15T22:05:25.000Z

You could probably use covdefaults or whatever to ignore ... lines, or else disable partials in .pyi files.

So I realized that covdefaults won't work simply because it's a plugin for coverage.py and MyPy doesn't use coverage.py. And using .coveragerc to ignore these won't work for exactly the same reason.

@Dreamsorcerer so it's even more confusing — I moved ... to a separate line locally and it shows up green, while the signature line is still yellow.

I even tried putting the args (like self) on separate lines and those were marked as green, while the def <meth_name>( bit remained yellow.

Answer 27 · 2023-12-15T22:44:50.000Z

Open a discussion in the mypy repository? It's mypy that decide what is a partial, right?

Answer 28 · 2023-12-16T01:01:26.000Z

Open a discussion in the mypy repository?

I was considering doing this.

It's mypy that decide what is a partial, right?

Yes.

Answer 29 · 2023-12-16T01:17:31.000Z

Meanwhile, the smallest repro seems to be

$ cat repro.pyi                      
from typing import Generic, TypeVar

_T = TypeVar("_T")


class Repro(Generic[_T]):
    def meth(self) -> None:
        ...

And using _T is crucial here. Without it, if the class inherits something simpler, the method definition magically turns green.