PYTHON-4533 Convert essential tests to async for Async PyMongo beta release #1724

NoahStapp · 2024-07-01T20:04:13Z

Needs #1718 to be merged first.

…ctory

blink1073 · 2024-07-10T15:29:24Z

pymongo/asynchronous/mongo_client.py

@@ -1517,6 +1521,9 @@ async def close(self) -> None:
            # TODO: PYTHON-1921 Encrypted MongoClients cannot be re-opened.
            await self._encrypter.close()

+    async def aclose(self) -> None:
+        await self.close()


Public API should have a docstring.

Why do we need aclose() when we already have close()?

contextlib expects that asynchronous context managers use aclose. We could simplify by just setting aclose = close instead.

In that case should we remove "close()"?

I think that's better, yeah.

pymongo/synchronous/client_session.py

blink1073 · 2024-07-10T15:35:44Z

test/pymongo_mocks.py


        client_options = client_context.default_client_options.copy()
        client_options.update(kwargs)

        super().__init__(*args, **client_options)

+    @classmethod
+    def get_async_mock_client(


Should this be get_sync_mock_client?

Good catch, using get_mock_client seems better.

blink1073 · 2024-07-10T15:37:55Z

test/test_client.py

@@ -2332,7 +2398,7 @@ class TestClientPool(MockClientTest):
    @client_context.require_connection
    def test_rs_client_does_not_maintain_pool_to_arbiters(self):
        listener = CMAPListener()
-        c = MockClient(
+        c = MockClient.get_async_mock_client(
            standalones=[],


Should this be get_sync_mock_client?

blink1073 · 2024-07-10T15:43:35Z

test/test_session.py

-                        f"{f.__name__} sent wrong $clusterTime with {event.command_name}",
-                    )
-
+# class TestCausalConsistency(unittest.TestCase):


Whoops yes good catch.

ShaneHarvey

Does anyone else feel there's a bit too much going on in this one PR to review effectively? Can you split it up into smaller and more manageable pieces?

ShaneHarvey · 2024-07-10T16:09:32Z

pymongo/asynchronous/mongo_client.py

@@ -861,6 +861,10 @@ def __init__(
            # This will be used later if we fork.
            AsyncMongoClient._clients[self._topology._topology_id] = self

+    async def connect(self) -> None:


"Connect" might not be the best name here since we're not actually connecting. I know we have the "connect=True" argument but I can't help but feel like "open()" would be more appropriate. If a user actually wanted to connect I would expect them to run something like "db.command('ping')". What do you think?

It feels a bit odd to me for the synchronous API to have a connect=True option but the equivalent async option is called open instead even though they do the same thing. Consistency of naming makes more sense to me here.

In that case I think the docstring just needs to explain the concept more clearly.

NoahStapp · 2024-07-10T16:44:58Z

Does anyone else feel there's a bit too much going on in this one PR to review effectively? Can you split it up into smaller and more manageable pieces?

Good point, let me try.

ShaneHarvey · 2024-07-10T17:16:46Z

pymongo/synchronous/mongo_client.py

@@ -860,6 +860,10 @@ def __init__(
            # This will be used later if we fork.
            MongoClient._clients[self._topology._topology_id] = self

+    def connect(self) -> None:
+        """Explicitly connect synchronously."""
+        self._get_topology()


We should avoid adding connect() and aclose() to the sync client.

What if we rename them to be private for the synchronous client? Like aconnect -> _aconnect and aclose -> _aclose? There isn't a clean way of having the synchro process just not convert specific methods.

How about something like this?:

_IS_SYNC = False class AsyncMongoClient: if not _IS_SYNC: async def aclose(self): pass

A better solution for aclose is to just rename it to close when it's synchronized. That way all async code will use aclose and all sync code will use close without any extra work. For aconnect what about this:

async def aconnect(self): if _IS_SYNC: pass else: ...

aclose -> close SGTM. For aconnect() I would prefer the approach I outlined above so that the method is never added to the sync client.

Actually. renaming aconnect -> _aconnect is fine too.

The synchro process will still convert the aconnect method within the if not _IS_SYNC block and add it to the sync client source. Is that acceptable?

I'm good with any approach as long as aconnect() or connect() isn't a public method on MongoClient.

ShaneHarvey · 2024-07-10T17:18:22Z

test/__init__.py

+    with warnings.catch_warnings():
+        # Ignore warning that ping is always routed to primary even
+        # if client's read preference isn't PRIMARY.
+        warnings.simplefilter("ignore", UserWarning)


I've never seen this warning. What is it referring to?

This already exists in our test suite:

mongo-python-driver/test/utils.py

Line 866 in 554ce7d

# Ignore warning that ping is always routed to primary even

I did some digging and that warning was removed in https://jira.mongodb.org/browse/PYTHON-814 in PyMongo 3.0. Could you remove the catch_warnings block?

ShaneHarvey · 2024-07-10T17:22:46Z

test/asynchronous/__init__.py

+        sys.setswitchinterval(interval)
+
+
+def lazy_client_trial(reset, target, test, get_client):


Does this even make sense to test in async? I would expect an async client to only be useable from a single thread since an I/O loop only runs on a single thread.

It isn't being tested at all in async, not sure why I moved it here. Putting back into test/utils.py.

ShaneHarvey · 2024-07-10T17:26:19Z

test/test_transactions.py

@@ -389,7 +392,7 @@ def test_transaction_direct_connection(self):
            with client.start_session() as s, s.start_transaction():
                res = f(*args, session=s)  # type:ignore[operator]
                if isinstance(res, (CommandCursor, Cursor)):
-                    list(res)
+                    res.to_list()


Can you add a docstring and changelog entry for to_list?

list(cursor) still works on synchronous cursors, but asynchronous cursors require the use of Cursor.to_list() instead. This change is purely so our tests can be synchronized smoothly.

Right, the only issue is that to_list() is undocumented.

ShaneHarvey · 2024-07-10T17:29:13Z

pymongo/asynchronous/client_session.py

@@ -598,7 +601,10 @@ def _inherit_option(self, name: str, val: _T) -> _T:

    async def with_transaction(
        self,
-        callback: Callable[[AsyncClientSession], _T],
+        callback: Union[
+            Callable[[AsyncClientSession], Coroutine[Any, Any, _T]],


Should we use Coroutine here or Awaitable? Which is a better fit?

We've used Coroutine consistently so far, Awaitable is a less-specific type hint that allows several other objects besides asynchronous functions.

ShaneHarvey · 2024-07-10T17:30:14Z

pymongo/asynchronous/client_session.py

-        callback: Callable[[AsyncClientSession], _T],
+        callback: Union[
+            Callable[[AsyncClientSession], Coroutine[Any, Any, _T]],
+            Callable[[AsyncClientSession], _T],


Why are there two callables here?

An artifact of the reasoning below, removed.

ShaneHarvey · 2024-07-10T17:31:25Z

pymongo/asynchronous/client_session.py

@@ -693,7 +699,10 @@ async def callback(session, custom_arg, custom_kwarg=None):
                read_concern, write_concern, read_preference, max_commit_time_ms
            )
            try:
-                ret = callback(self)
+                if not _IS_SYNC and iscoroutinefunction(callback):


Why check IS_SYNC or iscoroutinefunction here? Shouldn't it just be:

ret = await callback(self)

I was trying to be more lenient and allow both async and sync callbacks, but it's better to require all asynchronous callbacks for the Async API.

Jibola

Halfway through! Left some comments for now.

Jibola · 2024-07-10T18:08:36Z

pymongo/synchronous/client_session.py

+        callback: Union[
+            Callable[[ClientSession], _T],
+            Callable[[ClientSession], _T],
+        ],


It looks like the type definition is the same twice here. Is that needed?

No, this is an artifact of supporting both kinds of callbacks in the async API. Will be resolved in the separate PR for test_transaction

Jibola · 2024-07-10T18:09:34Z

pymongo/synchronous/mongo_client.py

@@ -1512,6 +1516,9 @@ def close(self) -> None:
            # TODO: PYTHON-1921 Encrypted MongoClients cannot be re-opened.
            self._encrypter.close()

+    def aclose(self) -> None:


Same as the comment from @ShaneHarvey before. We can remove close

Jibola · 2024-07-10T18:11:45Z

test/__init__.py

+    def require_sync(self, func):
+        """Run a test only if using the synchronous API."""
+        return self._require(
+            lambda: _IS_SYNC, "This test only works with the synchronous API", func=func


Since _IS_SYNC is defined as true, would the lambda ever fail?

This file is generated from test/asynchronous/__init__.py, it always passes for a synchronous test, as intended.

Jibola · 2024-07-10T18:13:12Z

test/__init__.py

+
+    @classmethod
+    def setUpClass(cls):
+        if _IS_SYNC:


Same comment. How does/may _IS_SYNC get redefined when tests are running?

_IS_SYNC is always true for a synchronous file and always false for an asynchronous file.

Jibola · 2024-07-10T18:25:46Z

test/__init__.py

+
+def drop_collections(db: Database):
+    # Drop all non-system collections in this database.
+    for coll in db.list_collection_names(filter={"name": {"$regex": r"^(?!system\.)"}}):


Do we need to filter the non system collection names? I had assumed those couldn't be dropped.

This is ancient code, probably it's to avoid attempting to drop a system collection and getting an OperationFailure because it's not allowed.

Jibola · 2024-07-10T18:30:17Z

test/__init__.py

+        t.start()
+
+    for t in threads:
+        t.join(60)


60 seconds is quite a bit of time. Have we been doing 60 historically?

Yeah, 60 is just a fail safe in case something goes wrong.

Jibola · 2024-07-10T18:33:26Z

test/__init__.py

+        for _i in range(NTRIALS):
+            reset(collection)
+            lazy_client = get_client()
+            lazy_collection = lazy_client.pymongo_test.test


NIT: do we need to redefine the lazy_client each time?

Yes, the point of repeating the test is to run with a new client each time.

mongodb-drivers-pr-bot bot requested a review from caseyclements July 1, 2024 20:30

NoahStapp changed the title ~~PYTHON-4533 Convert test_client to async~~ PYTHON-4533 Convert essential tests to async for Async PyMongo beta release Jul 1, 2024

NoahStapp marked this pull request as draft July 1, 2024 23:38

NoahStapp removed the request for review from caseyclements July 1, 2024 23:38

NoahStapp added 4 commits July 2, 2024 09:06

PYTHON-4530 - Synchronized test code should be in top-level test dire…

8c96646

…ctory

Convert test_client to async

17a6761

Fix typechecking

ad48086

Convert test/test_cursor.py to async

73d3b19

NoahStapp force-pushed the async-beta-tests branch from cf839b7 to 73d3b19 Compare July 2, 2024 16:10

NoahStapp added 6 commits July 2, 2024 09:12

Fix test/__init__.py docstring

46fbc97

Convert test/test_transactions.py

d588167

Convert test/test_session.py to async

427e4b5

Fix typechecking

9c1bf99

Merge branch 'master' into async-beta-tests

985c25c

Use compatibility anext/next

665f730

NoahStapp force-pushed the async-beta-tests branch from 240e00e to 665f730 Compare July 3, 2024 18:09

NoahStapp marked this pull request as ready for review July 3, 2024 18:11

mongodb-drivers-pr-bot bot requested a review from Jibola July 3, 2024 18:21

NoahStapp added 3 commits July 3, 2024 11:30

fix failing replicaset tests

0172ff3

Fix tearDownClass for async tests

a0652ce

Only test contextlib.aclosing on >= 3.10

c00c15b

NoahStapp force-pushed the async-beta-tests branch from 56b779e to c00c15b Compare July 3, 2024 20:04

Fix async test_contextlib

2850ed7

NoahStapp mentioned this pull request Jul 8, 2024

PYTHON-4538 await _check_user_provided #1726

Merged

Merge branch 'master' into async-beta-tests

6457f6c

NoahStapp requested a review from ShaneHarvey July 9, 2024 22:15

blink1073 requested review from blink1073 and removed request for Jibola July 10, 2024 15:14

blink1073 reviewed Jul 10, 2024

View reviewed changes

ShaneHarvey reviewed Jul 10, 2024

View reviewed changes

ShaneHarvey requested changes Jul 10, 2024

View reviewed changes

Jibola requested changes Jul 10, 2024

View reviewed changes

		sys.setswitchinterval(interval)


		def lazy_client_trial(reset, target, test, get_client):

PYTHON-4533 Convert essential tests to async for Async PyMongo beta release #1724

Are you sure you want to change the base?

PYTHON-4533 Convert essential tests to async for Async PyMongo beta release #1724

Conversation

NoahStapp commented Jul 1, 2024

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

Choose a reason for hiding this comment

NoahStapp commented Jul 10, 2024

Choose a reason for hiding this comment

NoahStapp Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Jul 10, 2024 • 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

Jibola 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

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 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

NoahStapp Jul 10, 2024 •

edited

Loading

ShaneHarvey Jul 10, 2024 •

edited

Loading

ShaneHarvey Jul 10, 2024 •

edited

Loading