Repository navigation
Add enum iteration test cases - #4400
Conversation
|
|
|
@elazarg I added an iteration test that directly iterates over an enum type without checking |
|
@ngaya-ll see previous comment - the enum stub is incomplete, it does not have an PR 1755 is about the removal of explicit baseclasses, which will fix the other tests; you can try mimicking it in the stub. |
|
@elazarg If I understand correctly, you're saying the unit tests here are independent of the typeshed definitions. I observed a problem with mypy in the wild, so I thought it would make sense to add test cases here to demonstrate the problem and verify when it was fixed. But it sounds like these tests use dummy type definitions that will not reflect the real-world behavior of mypy. Is there a better way to test what I'm trying to test (i.e. that mypy + typeshed correctly handles enums in real life)? Or is this not helpful? |
|
This is what python-eval tests are for. |
1f3e08e to
7b8b2d0
Compare
Test cases for python#2305
|
@elazarg Thanks! Moved the enum tests to pythoneval and rebased the branch onto master and the tests are now passing. Can this PR be merged? |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Here are some suggestions.
| from typing import Iterable | ||
| class E(Enum): | ||
| a = 'a' | ||
| def f(ie:Iterable[E]): |
There was a problem hiding this comment.
Please follow PEP 8 (even in tests).
| _testNoCrashOnGenericUnionUnpacking.py:15: error: Revealed type is 'Union[builtins.int*, builtins.str*]' | ||
| _testNoCrashOnGenericUnionUnpacking.py:16: error: Revealed type is 'Union[builtins.int*, builtins.str*]' | ||
|
|
||
| [case testEnumIteration] |
There was a problem hiding this comment.
I would use longer/more detailed names for test cases (there are lots of tests in pythoneval.test).
There was a problem hiding this comment.
Hm, not sure what more detail would be helpful here. Did you have something in mind?
There was a problem hiding this comment.
Something like EnumIterationPreservesPreciseType
There was a problem hiding this comment.
That's only part of what this is aiming to test. I'll add a comment in the test with more details.
| [case testEnumIteration] | ||
| from enum import Enum | ||
| class E(Enum): | ||
| A = 'a' |
There was a problem hiding this comment.
Why do you use uppercase A here and lowercase a in other places?
There was a problem hiding this comment.
I'll change to uppercase for all enum constants to be consistent.
| from enum import Enum | ||
| class E(Enum): | ||
| A = 'a' | ||
| l = [e for e in E] |
There was a problem hiding this comment.
Also reveal type of list(E)?
There was a problem hiding this comment.
The test is more concerned with the element type of the comprehension, not the resulting list. I'll edit the test to make that more explicit.
There was a problem hiding this comment.
The test is more concerned with the element type of the comprehension, not the resulting list.
It is (almost) never bad to test more.
There was a problem hiding this comment.
I was looking at it more in terms of trying to check the enum behavior with minimal reliance on other type-checking features. But I can add the additional check if you feel strongly that it would be valuable.
Thank you for PR! Please see some suggestions I left above. After you consider them, we can add these tests. |
| _testNoCrashOnGenericUnionUnpacking.py:15: error: Revealed type is 'Union[builtins.int*, builtins.str*]' | ||
| _testNoCrashOnGenericUnionUnpacking.py:16: error: Revealed type is 'Union[builtins.int*, builtins.str*]' | ||
|
|
||
| [case testEnumIteration] |
There was a problem hiding this comment.
That's only part of what this is aiming to test.
Then choose a better name. Also the comment below is too long, just leave # Regression test for #2305
I am not arguing for fun here, there is pytest -k command that will run all test cases with a given pattern in names. This is why it is important to add some specific keyword like Precise or Preserve (or your choice). In addition, this avoids accidental test name collisions (we currently have thousands of test cases).
3 participants
Test cases for #2305.