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 enum iteration test cases #4400

Merged
merged 4 commits into from Feb 13, 2018

Conversation

Projects
None yet
3 participants
@ngaya-ll
Contributor

ngaya-ll commented Dec 21, 2017

Test cases for #2305.

@elazarg

This comment has been minimized.

Contributor

elazarg commented Dec 21, 2017

Enum should follow typeshed - __iter__ should be added to an EnumMeta metaclass.
(EDIT: the PR for typeshed: python/typeshed#1755)

@ngaya-ll

This comment has been minimized.

Contributor

ngaya-ll commented Dec 21, 2017

@elazarg I added an iteration test that directly iterates over an enum type without checking Iterable, but for some reason that also fails even though equivalent code passes with mypy 0.560. Do you know what's going on there? Is that a regression, a problem with the test harness, or a problem with the test?

@elazarg

This comment has been minimized.

Contributor

elazarg commented Dec 21, 2017

@ngaya-ll see previous comment - the enum stub is incomplete, it does not have an EnumMeta metaclass with an __iter__ method (which typeshed already has).

PR 1755 is about the removal of explicit baseclasses, which will fix the other tests; you can try mimicking it in the stub.

@ngaya-ll

This comment has been minimized.

Contributor

ngaya-ll commented Feb 1, 2018

@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?

@elazarg

This comment has been minimized.

Contributor

elazarg commented Feb 1, 2018

This is what python-eval tests are for.

@ngaya-ll

This comment has been minimized.

Contributor

ngaya-ll commented Feb 2, 2018

@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

Here are some suggestions.

from typing import Iterable
class E(Enum):
a = 'a'
def f(ie:Iterable[E]):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Feb 3, 2018

Collaborator

Please follow PEP 8 (even in tests).

@@ -1458,3 +1458,48 @@ _testNoCrashOnGenericUnionUnpacking.py:10: error: Revealed type is 'Union[builti
_testNoCrashOnGenericUnionUnpacking.py:11: error: Revealed type is 'Union[builtins.str, builtins.int]'
_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]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Feb 3, 2018

Collaborator

I would use longer/more detailed names for test cases (there are lots of tests in pythoneval.test).

This comment has been minimized.

@ngaya-ll

ngaya-ll Feb 5, 2018

Contributor

Hm, not sure what more detail would be helpful here. Did you have something in mind?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Feb 6, 2018

Collaborator

Something like EnumIterationPreservesPreciseType

This comment has been minimized.

@ngaya-ll

ngaya-ll Feb 6, 2018

Contributor

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'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Feb 3, 2018

Collaborator

Why do you use uppercase A here and lowercase a in other places?

This comment has been minimized.

@ngaya-ll

ngaya-ll Feb 5, 2018

Contributor

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]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Feb 3, 2018

Collaborator

Also reveal type of list(E)?

This comment has been minimized.

@ngaya-ll

ngaya-ll Feb 5, 2018

Contributor

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Feb 6, 2018

Collaborator

The test is more concerned with the element type of the comprehension, not the resulting list.

It is (almost) never bad to test more.

This comment has been minimized.

@ngaya-ll

ngaya-ll Feb 6, 2018

Contributor

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.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Feb 3, 2018

@ngaya-ll

Can this PR be merged?

Thank you for PR! Please see some suggestions I left above. After you consider them, we can add these tests.

ngaya-ll added some commits Feb 5, 2018

@@ -1458,3 +1458,51 @@ _testNoCrashOnGenericUnionUnpacking.py:10: error: Revealed type is 'Union[builti
_testNoCrashOnGenericUnionUnpacking.py:11: error: Revealed type is 'Union[builtins.str, builtins.int]'
_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]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Feb 7, 2018

Collaborator

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).

@ilevkivskyi ilevkivskyi merged commit bdd5de9 into python:master Feb 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment