Annotate functions in tests #4434

elazarg · 2018-01-07T19:19:55Z

Seems like some functions are accidentally unannotated. For example, testInplaceSetitem should have actually failed since the list fixture does not declare addition for int; but it doesn't, since mypy seems to silently infer Any for the attribute.

ilevkivskyi · 2018-01-07T20:38:18Z

I am wondering if some of those tests don't have annotations intentionally, for example to check that something works/fails even in non-annotated code. Maybe a safer way would be to make copies of tests that you changed, and keep both versions: with and without annotations?

elazarg · 2018-01-07T20:50:33Z

Tests for unannotated code would have (or should have) named appropriately. I have left out some tests that seem to address unannotated code intentionally.

As for copies, they will define the expected behaviour, of which I am not sure. PEP 484 does not define what should and should not be checked, inferred or reported in unannotated functions and methods.

gvanrossum · 2018-01-07T21:01:43Z

IIRC mypy analyzes the bodies of all unannotated functions just as if they were annotated with all Anys, and then suppresses errors -- but keeps inferences about things outside the function (in particular instance attributes). I would tread carefully here. Surely git blame can tell you who wrote these tests originally? (My guess would be Jukka. :-)

elazarg · 2018-01-07T21:44:16Z

Mypy does indeed, and these tests were found in an attempt to move the decision to a single place.

What exactly do you mean by "tread carefully"? IMHO the real question is the specification: what is the expected behaviour? The tests I've touched do no seem to intentionally try to define expected behaviour for the special case of unannotated functions, otherwise they neglect the more common and important case.

The inference seems like a waste of computation time, especially at early stages of "gradualization" of a large codebase, since the inferred type of will always be Any. The fact that a function is "declared" to be un-type-checkable means that non-obvious inference - i.e. inferring an attribute for an inferred type - is expected to be wrong. Simple inference of attributes can be done at the semantic analysis phase.

Blame:

check-classes, testReferToInvalidAttribute: @JukkaL, 5 years ago; mypy-style
check-inference.test, testInferDictInitializedToEmptyAndUpdatedFromMethod: Guido, Dec 2015
check-modules.test, testDeferredClassContext: Fix crash for deferred methods introduced by #2193 (self-type). #2375 (Guido)
check-statements.test, testInplaceSetitem: Respect in-place operation when operator-assigning through square brackets #2051 (@gnprice)

JukkaL

Thanks, paying attention to test case quality is worthwhile. However, I'd default to retaining the original test cases and adding new ones just in case we've lost some context about why they were written that way in the first place, unless the test case is obviously broken.

JukkaL · 2018-01-08T13:49:04Z

test-data/unit/check-classes.test

@@ -122,7 +122,7 @@ A().f = None # E: Cannot assign to a method
 [case testReferToInvalidAttribute]

 class A:
-    def __init__(self):
+    def __init__(self) -> None:


I think that omitting the return type was intentional here. It would be okay to have two variants of this method, one with an annotation for __init__ and another without.

JukkaL · 2018-01-08T13:50:54Z

test-data/unit/check-inference.test

@@ -1403,7 +1403,7 @@ b[{}] = 1

 [case testInferDictInitializedToEmptyAndUpdatedFromMethod]
 map = {}
-def add():
+def add() -> None:


It's unclear if the annotation was omitted intentionally. I'd suggest creating two variants of the test case.

JukkaL · 2018-01-08T13:52:45Z

test-data/unit/check-inference.test

@@ -1959,7 +1959,7 @@ class C:

 if bool():
    f()
-    1 + '' # E: Unsupported left operand type for + ("int")
+    1 + '' # E: Unsupported operand types for + ("int" and "str")


I'd prefer changing the test case so that we don't depend on int.__add__ instead (it's a little arbitrary to rely on it here). For example, use 1() instead of 1 + '' which doesn't require a fixture change.

JukkaL · 2018-01-08T13:53:03Z

test-data/unit/check-inference.test

@@ -1976,7 +1976,7 @@ class C:

 if bool():
    f()
-    1 + '' # E: Unsupported left operand type for + ("int")
+    1 + '' # E: Unsupported operand types for + ("int" and "str")


Similar to above, I'd prefer using 1() instead (and not changing the fixture).

JukkaL · 2018-01-08T13:53:14Z

test-data/unit/check-inference.test

@@ -1992,6 +1992,6 @@ class C:

 if bool():
    f([])
-    1 + '' # E: Unsupported left operand type for + ("int")
+    1 + '' # E: Unsupported operand types for + ("int" and "str")


Similar to above.

JukkaL · 2018-01-08T13:54:41Z

test-data/unit/check-modules.test

@@ -1312,7 +1312,7 @@ class A:
    def f(self) -> str: return 'foo'
 class B(A):
    def f(self) -> str: return self.x
-    def initialize(self): self.x = 'bar'
+    def initialize(self) -> None: self.x = 'bar'


Not having a return type may have well been intentional. I'd prefer either keeping the original test case or adding another variant with a return type annotation.

JukkaL · 2018-01-08T13:55:44Z

test-data/unit/check-statements.test

@@ -353,7 +353,7 @@ a @= 1  # E: Argument 1 to "__imatmul__" of "A" has incompatible type "int"; exp

 [case testInplaceSetitem]
 class A(object):
-    def __init__(self):
+    def __init__(self) -> None:


Good catch!

JukkaL · 2018-01-08T13:56:04Z

test-data/unit/fixtures/list.pyi

@@ -26,7 +26,8 @@ class list(Generic[T]):

 class tuple(Generic[T]): pass
 class function: pass
-class int: pass
+class int:


As discussed above, I wouldn't change the fixture.

elazarg · 2018-01-09T20:28:04Z

Done.

I still think this "just in case" is misplaced here. What's so wrong about changing tests, then (worst case) finding the context through some breakage at some edge case, and then test for it thoroughly? The resulting system will be more stable this way. And this is worst-case scenario.

This PR now defines an expected behaviour that was not explicitly defined previously, just in case. IMO with this definition, the distinction between annotated and unannotated functions is not obvious.

JukkaL

Thanks for the updates!

JukkaL · 2018-01-10T16:51:01Z

@elazarg The cost of having a few potentially redundant test cases (assuming that they aren't slow to run) is pretty minor. On the other hand, finding out whether a test case is potentially redundant (or invalid) can be hard, since it's 1) difficult to determine if the same feature or edge case is tested elsewhere and 2) a test case may test some edge case that isn't obvious from the name of the test case. So the default option is to leave a potentially poorly thought out test case instead of removing it -- but using a more descriptive name is still a good thing, or adding a comment such as "TODO: It's unclear if the missing annotation is intentional or not".

elazarg · 2018-01-10T17:16:45Z

I don't care about redundancy. I am only interested in the decision to use unannotated functions as a source for type information; I don't think such a decision should be the result of a (possibly accidental) omission in a test. But since it is now decided, what's left is to document this behaviour.

JukkaL · 2018-01-10T17:20:41Z

There have been bugs that were triggered only by unannotated functions. Sometimes these can be pretty surprising. Mypy still does a bunch of analysis for unannotated functions (even though it rarely generates type errors).

annotate functions in tests

36a1637

elazarg force-pushed the annotate-tests branch from c46760e to 36a1637 Compare January 7, 2018 19:31

JukkaL requested changes Jan 8, 2018

View reviewed changes

elazarg added 2 commits January 9, 2018 01:04

fix and duplicate tests as required

8c5be18

inplace addition for list; fix InvalidAttribute

815a3f5

JukkaL approved these changes Jan 10, 2018

View reviewed changes

JukkaL merged commit 2dc3d22 into python:master Jan 10, 2018

elazarg mentioned this pull request Jan 10, 2018

Inference from unannotated functions #4455

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Annotate functions in tests #4434

Annotate functions in tests #4434

elazarg commented Jan 7, 2018

ilevkivskyi commented Jan 7, 2018

elazarg commented Jan 7, 2018 •

edited

Loading

gvanrossum commented Jan 7, 2018

elazarg commented Jan 7, 2018

JukkaL left a comment

JukkaL Jan 8, 2018

JukkaL Jan 8, 2018

JukkaL Jan 8, 2018

JukkaL Jan 8, 2018

JukkaL Jan 8, 2018

JukkaL Jan 8, 2018

JukkaL Jan 8, 2018

JukkaL Jan 8, 2018

elazarg commented Jan 9, 2018

JukkaL left a comment

JukkaL commented Jan 10, 2018

elazarg commented Jan 10, 2018

JukkaL commented Jan 10, 2018

Annotate functions in tests #4434

Annotate functions in tests #4434

Conversation

elazarg commented Jan 7, 2018

ilevkivskyi commented Jan 7, 2018

elazarg commented Jan 7, 2018 • edited Loading

gvanrossum commented Jan 7, 2018

elazarg commented Jan 7, 2018

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elazarg commented Jan 9, 2018

JukkaL left a comment

Choose a reason for hiding this comment

JukkaL commented Jan 10, 2018

elazarg commented Jan 10, 2018

JukkaL commented Jan 10, 2018

elazarg commented Jan 7, 2018 •

edited

Loading