PYTHON-3064 Add typings to test package #844

blink1073 · 2022-02-03T14:29:17Z

No description provided.

ShaneHarvey

A few points of feedback.

ShaneHarvey · 2022-02-03T22:23:17Z

test/test_son.py

+        self.assertTrue("2" in test_son, "in failed")
+        self.assertFalse("22" in test_son, "in succeeded when it shouldn't")
+        self.assertTrue(test_son.has_key("2"), "has_key failed")
+        self.assertFalse(test_son.has_key("22"), "has_key succeeded when it shouldn't")


Instead of changing these tests we need to change SON itself because it supports non-str keys.

ShaneHarvey · 2022-02-03T22:30:56Z

pymongo/socket_checker.py

@@ -43,7 +42,7 @@ def __init__(self) -> None:
        else:
            self._poller = None

-    def select(self, sock: Any, read: bool = False, write: bool = False, timeout: int = 0) -> bool:
+    def select(self, sock: Any, read: bool = False, write: bool = False, timeout: Optional[Union[float, int]] = 0) -> bool:


timeout should be float. The float type supports int automatically. Also is Optional needed? Do we ever pass None?

We call it with None explicitly in test_pooling.py, and there is internal handling for None. I'll change it to Optional[float]

ShaneHarvey · 2022-02-03T22:37:15Z

test/test_bson.py

@@ -186,7 +186,7 @@ def test_encode_then_decode_any_mapping_legacy(self):
            decoder=lambda *args: BSON(args[0]).decode(*args[1:]))

    def test_encoding_defaultdict(self):
-        dct = collections.defaultdict(dict, [('foo', 'bar')])
+        dct = collections.defaultdict(dict, [('foo', 'bar')])  # type: ignore[arg-type]


Can you report this as a mypy bug?

Actually, thinking about it more, I don't think this is a bug. We're declaring that new values in the dictionary are of dictionary type, but the dictionary itself is initialized with a value of string type. If I use foo = defaultdict(dict, {'a': dict(a=1)}) in the mypy playground it works fine.

This is a mypy bug. The docs for defaultdict says:

The first argument provides the initial value for the default_factory attribute; it defaults to None. All remaining arguments are treated the same as if they were passed to the dict constructor, including keyword arguments.

eg:

>>> dict([('foo', 'bar')]) {'foo': 'bar'} >>> dct = collections.defaultdict(dict, [('foo', 'bar')]) >>> dct defaultdict(<class 'dict'>, {'foo': 'bar'}) >>> dct['g'] {} >>> dct defaultdict(<class 'dict'>, {'foo': 'bar', 'g': {}})

That said, we can change this line to:

dct = collections.defaultdict(dict, {'foo': 'bar'})

I guess my point is, they'd have to change the signature of defaultdict in typeshed to not use generics, or perhaps add more additional overrides to provide non-generic versions. I'll update the line instead. 😄

Oh, I see what you meant. Do you still think this is worth bringing up to typeshed?

Yes we should report this bug to typeshed.

python/typeshed#7159

Oh sorry, I just realized I'm wrong here. The problem is that we're passing a 'str' value to a dictionary of type Dict['str', dict]. This is not a typeshed bug:

>>> dct = collections.defaultdict(dict, [('foo', 'bar')]) # This is a type error >>> dct = collections.defaultdict(dict, [('foo', {})]) # This works.

Haha, that is what I was poorly trying to convey. I still think the raised issue stands, but they are likely to ignore/close it.

test/test_bulk.py

test/test_cursor.py

test/utils.py

ShaneHarvey · 2022-02-03T23:09:34Z

.github/workflows/test-python.yml

@@ -46,4 +46,4 @@ jobs:
          pip install -e ".[zstd, srv]"
      - name: Run mypy
        run: |
-          mypy --install-types --non-interactive bson gridfs tools
+          mypy --install-types --non-interactive bson gridfs tools test


Does it make sense to separate the test/ type checking so that we can globally ignore certain error codes in the test/ package? Eg:

mypy --install-types --non-interactive bson gridfs tools mypy --install-types --non-interactive --disable-error-code var-annotated --disable-error-code attr-defined --disable-error-code union-attr --disable-error-code assignment test

Disabling some checks could make the tests easier to maintain.

ShaneHarvey · 2022-02-03T23:11:12Z

test/mockupdb/test_reset_and_request_check.py

@@ -45,7 +45,7 @@ def responder(request):
        kwargs = {'socketTimeoutMS': 100}
        # Disable retryable reads when pymongo supports it.
        kwargs['retryReads'] = False
-        self.client = MongoClient(self.server.uri, **kwargs)
+        self.client = MongoClient(self.server.uri, **kwargs)  # type: ignore


What's up with the type ignore here? It seems perfectly reasonable to pass **kwargs to MongoClient because it accepts **kwargs. Is this something we should report to mypy?

Yeah it is a type limitation of mypy, I'll look for/file an issue

Found this comment: I'll try TypedDict

Actually, it really is a bug, already reported: python/mypy#8862

Created https://jira.mongodb.org/browse/PYTHON-3105 to track this

ShaneHarvey · 2022-02-03T23:16:26Z

test/test_change_stream.py

@@ -121,6 +122,7 @@ def kill_change_stream_cursor(self, change_stream):


 class APITestsMixin(object):


I think this file is improperly using inheritance. Could you open a tech debt ticket so we can fix this at some point?

I addressed the code review comments, I'll handle the mypy/JIRA tickets tomorrow

https://jira.mongodb.org/browse/PYTHON-3106

ShaneHarvey · 2022-02-04T23:24:45Z

pymongo/socket_checker.py

 import sys
-from typing import Any, Optional
+from typing import Any, Optional, Union


Union is now unused.

ShaneHarvey · 2022-02-04T23:28:14Z

test/test_bson.py

@@ -302,7 +302,7 @@ def test_basic_decode(self):

    def test_decode_all_buffer_protocol(self):
        docs = [{'foo': 'bar'}, {}]
-        bs = b"".join(map(encode, docs))
+        bs = b"".join(map(encode, docs))  # type: ignore[arg-type]


This looks like another bug we can report to mypy.

I'm not sure about this one. This is another example of a generic in the second argument affecting the value in the first, but I think this time it is warranted. If we change it to docs: list[Mapping[str, Any]] then it matches and passes type check.

ShaneHarvey · 2022-02-04T23:59:07Z

test/test_examples.py

@@ -890,6 +890,7 @@ def update_employee_info(session):
            update_employee_info(session)

        employee = employees.find_one({"employee": 3})
+        assert employee is not None
        self.assertIsNotNone(employee)


This is disappointing. I added a comment to the mypy issue tracker here: python/mypy#4063 (comment)

test/test_examples.py

ShaneHarvey · 2022-02-05T00:06:09Z

test/test_examples.py

@@ -1091,6 +1095,8 @@ def test_causal_consistency(self):

        # Start Causal Consistency Example 2
        with client.start_session(causal_consistency=True) as s2:
+            assert s1.cluster_time is not None
+            assert s1.operation_time is not None


For the same reason as above, let's move this asserts to outside the example (above "Start Causal Consistency Example 2"). Also I'm very confused how this assert helps at all since we're accessing s1.cluster_time again below. From earlier conversations I would have expected mypy to not narrow the type here since each property access can theoretically return a different instance.

Fixed. Yeah, that does seem like a limitation.

test/test_grid_file.py

test/__init__.py

ShaneHarvey · 2022-02-05T00:14:22Z

test/test_bson.py

-        as_utc = (aware - aware.utcoffset()).replace(tzinfo=utc)
+        offset = aware.utcoffset()
+        assert offset is not None
+        as_utc = (aware - offset).replace(tzinfo=utc)


This is disappointing. IIUC a datetime with a tzinfo should always return a non-None utcoffset(). I think it could be related to python/mypy#10067 but I think it's worth reporting as a separate issue.

I think the only way to do it is to do what I suggested in python/mypy#10067 (comment)

ShaneHarvey

Just one minor change

ShaneHarvey · 2022-02-07T22:53:05Z

test/test_examples.py

        # Start Causal Consistency Example 2
        with client.start_session(causal_consistency=True) as s2:
+


Unneeded blank line.

ShaneHarvey

LGTM!

blink1073 · 2022-02-08T01:33:36Z

Merging since the failure is fixed by #848

blink1073 added 11 commits February 3, 2022 08:29

PYTHON-3064 Add typings to test package

8627451

cleanup typings

7ed5731

fix dict type

a385dff

fix son tests

d435dad

fix python 3.6 syntax

6009bf6

fixups

699e696

fixups

3f651f0

fixups

6247b9a

fixups

44607b7

fixups

d1b77b7

fixups

3c6ada0

blink1073 marked this pull request as ready for review February 3, 2022 16:42

blink1073 requested review from juliusgeo and ShaneHarvey as code owners February 3, 2022 16:42

blink1073 added 6 commits February 3, 2022 16:13

test the package in CI

887d804

fixups

d63bcc3

make pipeline a sequence

2b3796e

fixup

937a366

fixup

7ca474f

remove use of deprecated functions

1f3a2aa

ShaneHarvey requested changes Feb 3, 2022

View reviewed changes

address review

363c947

blink1073 requested a review from ShaneHarvey February 4, 2022 19:21

ShaneHarvey reviewed Feb 5, 2022

View reviewed changes

address review

d12570e

ShaneHarvey requested changes Feb 7, 2022

View reviewed changes

address review

1920c47

blink1073 requested a review from ShaneHarvey February 7, 2022 23:30

ShaneHarvey approved these changes Feb 8, 2022

View reviewed changes

blink1073 merged commit f4cef37 into mongodb:master Feb 8, 2022

blink1073 deleted the PYTHON-3064 branch February 8, 2022 01:33

juliusgeo pushed a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 5, 2022

PYTHON-3064 Add typings to test package (mongodb#844)

ee564c6

juliusgeo pushed a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 7, 2022

PYTHON-3064 Add typings to test package (mongodb#844)

984acf4

		@@ -121,6 +122,7 @@ def kill_change_stream_cursor(self, change_stream):


		class APITestsMixin(object):

		# Start Causal Consistency Example 2
		with client.start_session(causal_consistency=True) as s2:

PYTHON-3064 Add typings to test package #844

PYTHON-3064 Add typings to test package #844

Conversation

blink1073 commented Feb 3, 2022

ShaneHarvey 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

ShaneHarvey Feb 4, 2022 • edited Loading

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

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

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

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

blink1073 commented Feb 8, 2022

ShaneHarvey Feb 4, 2022 •

edited

Loading