PYTHON-5073 Convert test.test_connection_monitoring to async #2087

sleepyStick · 2025-01-28T23:18:07Z

Convert test.test_connection_monitoring to async

NoahStapp · 2025-01-29T13:20:17Z

test/asynchronous/test_connection_monitoring.py

+    #
+    async def test_1_client_connection_pool_options(self):
+        client = await self.async_rs_or_single_client(**self.POOL_OPTIONS)
+        self.addAsyncCleanup(client.close)


Our helper methods like self.async_rs_or_single_client call self.addAsyncCleanup(client.close) for us, so we don't need to do it ourselves.

NoahStapp · 2025-01-29T13:27:50Z

test/asynchronous/test_connection_monitoring.py

+    def start(self, op):
+        """Run the 'start' thread operation."""
+        target = op["target"]
+        thread = SpecRunnerThread(target)


There's a separate ticket to create an asynchronous version of SpecRunnerThread using asyncio tasks: https://jira.mongodb.org/browse/PYTHON-4864. Until we implement that ticket, we don't want to use a thread-based SpecRunnerThread for asynchronous tests due to the poor performance and inconsistent behavior of mixing threads and asyncio.

Makes sense, until that ticket is complete, what should I do for the tests? (or is this going to need to wait for that ticket to be completed?)

wait ignore me, my brain is finally waking up and realizing that's what the last comment was for HAHA

NoahStapp · 2025-01-29T13:29:28Z

test/asynchronous/test_connection_monitoring.py

+
+class AsyncTestCMAP(AsyncIntegrationTest):
+    # Location of JSON test specifications.
+    TEST_PATH = os.path.join(os.path.dirname(os.path.realpath(__file__)), "connection_monitoring")


We need to update this path to reflect the asynchronous subfolder inside test. The pattern is the same across all asynchronous tests that use a TEST_PATH:

if _IS_SYNC: _TEST_PATH = os.path.join(pathlib.Path(__file__).resolve().parent, <module_name>) else: _TEST_PATH = os.path.join(pathlib.Path(__file__).resolve().parent.parent, <module_name>)

NoahStapp · 2025-01-29T13:49:28Z

test/asynchronous/test_connection_monitoring.py

+        return [scenario_def]
+
+
+test_creator = CMAPSpecTestCreator(create_test, AsyncTestCMAP, AsyncTestCMAP.TEST_PATH)


To only run the SpecRunnerThread tests on the sync API:

if _IS_SYNC: test_creator = CMAPSpecTestCreator(create_test, AsyncTestCMAP, AsyncTestCMAP.TEST_PATH) test_creator.create_tests()

NoahStapp

Great work, just one minor cleanup!

NoahStapp · 2025-01-29T18:43:18Z

test/asynchronous/test_connection_monitoring.py

@@ -83,7 +84,12 @@

 class AsyncTestCMAP(AsyncIntegrationTest):
    # Location of JSON test specifications.
-    TEST_PATH = os.path.join(os.path.dirname(os.path.realpath(__file__)), "connection_monitoring")
+    if _IS_SYNC:
+        TEST_PATH = os.path.join(pathlib.Path(__file__).resolve().parent, "connection_monitoring")


We can do Path instead of pathlib.Path and change the import to from pathlib import Path for clarity here.

NoahStapp

Looks great, let's wait to merge this until #2094 is merged so we can add in the tests that rely on SpecRunnerThread too.

NoahStapp · 2025-02-05T19:38:13Z

test/asynchronous/utils_spec_runner.py

@@ -222,14 +222,14 @@ async def _create_tests(self):
                test_type = os.path.splitext(filename)[0]

                # Construct test from scenario.
-                for test_def in self.tests(scenario_def):
+                for test_def in await self.tests(scenario_def):


self.tests is not asynchronous, so no await needed here.

okay i thought it was really weird too, but without the await i got the following error:

Traceback (most recent call last): File "/opt/homebrew/Cellar/[email protected]/3.12.8/Frameworks/Python.framework/Versions/3.12/lib/python3.12/warnings.py", line 555, in _warn_unawaited_coroutine warn(msg, category=RuntimeWarning, stacklevel=2, source=coro) RuntimeWarning: coroutine 'CMAPSpecTestCreator.tests' was never awaited test/asynchronous/test_connection_monitoring.py:None (test/asynchronous/test_connection_monitoring.py) test_connection_monitoring.py:475: in <module> test_creator.create_tests() utils_spec_runner.py:243: in create_tests asyncio.run(self._create_tests()) /opt/homebrew/Cellar/[email protected]/3.12.8/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py:194: in run return runner.run(main) /opt/homebrew/Cellar/[email protected]/3.12.8/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py:118: in run return self._loop.run_until_complete(task) /opt/homebrew/Cellar/[email protected]/3.12.8/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/base_events.py:686: in run_until_complete return future.result() utils_spec_runner.py:225: in _create_tests for test_def in self.tests(scenario_def): E TypeError: 'coroutine' object is not iterable```

Ah, found the issue:

class CMAPSpecTestCreator(AsyncSpecTestCreator): async def tests(self, scenario_def): ...

should be

class CMAPSpecTestCreator(AsyncSpecTestCreator): def tests(self, scenario_def): ...

And then await self.tests() can become self.tests()

ahh! good find! thanks!

NoahStapp · 2025-02-06T18:53:41Z

Encryption test failure is due to test_encryption's create_test being async when it should be sync: https://evergreen.mongodb.com/test_log/mongo_python_driver_encryption_rhel8_python3.10_test_4.0_sharded_cluster_auth_ssl_sync_async_patch_3dd44e6e5f89ffc6171c8ffc7ed052b620e35065_67a4fc4e7a1a330007e6f608_25_02_06_18_15_43/0?test_name=4b8a3fb7e7ebd8ee1a0182780cc2ba8a#L0

sleepyStick added 2 commits January 28, 2025 15:17

Convert test.test_connection_monitoring to async

19896a8

fix

fadef89

sleepyStick requested a review from NoahStapp January 29, 2025 03:30

NoahStapp requested changes Jan 29, 2025

View reviewed changes

sleepyStick changed the title ~~PYTHON-5072 Convert test.test_connection_monitoring to async~~ PYTHON-5073 Convert test.test_connection_monitoring to async Jan 29, 2025

sleepyStick added 3 commits January 29, 2025 09:34

address review comments

f0bb7c8

Merge branch 'master' into PYTHON-5073

58bae37

don't mind me being silly

6a6577a

sleepyStick marked this pull request as ready for review January 29, 2025 17:48

NoahStapp requested changes Jan 29, 2025

View reviewed changes

sleepyStick and others added 2 commits January 29, 2025 11:24

change import

1dbe1f9

Merge branch 'master' into PYTHON-5073

eb74f1b

sleepyStick requested a review from NoahStapp January 29, 2025 20:17

NoahStapp reviewed Jan 29, 2025

View reviewed changes

sleepyStick and others added 3 commits January 29, 2025 13:18

Merge branch 'master' into PYTHON-5073

ad106b2

Merge branch 'master' into PYTHON-5073

7bde85c

run all tests with SpecRunnerTask

0e5df26

sleepyStick requested a review from NoahStapp February 5, 2025 19:30

NoahStapp requested changes Feb 5, 2025

View reviewed changes

sleepyStick and others added 2 commits February 6, 2025 10:13

Merge branch 'master' into PYTHON-5073

b2f8d98

fix tests

8cd9f1a

sleepyStick requested a review from NoahStapp February 6, 2025 18:16

sleepyStick added 2 commits February 6, 2025 11:53

Merge branch 'master' into PYTHON-5073

ebe590f

Merge branch 'master' into PYTHON-5073

6f05489

ShaneHarvey mentioned this pull request Feb 7, 2025

PYTHON-5120 Reduce configureFailPoint duplication in tests #2131

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PYTHON-5073 Convert test.test_connection_monitoring to async #2087

PYTHON-5073 Convert test.test_connection_monitoring to async #2087

sleepyStick commented Jan 28, 2025

NoahStapp Jan 29, 2025

NoahStapp Jan 29, 2025

sleepyStick Jan 29, 2025

sleepyStick Jan 29, 2025

NoahStapp Jan 29, 2025

NoahStapp Jan 29, 2025

NoahStapp left a comment

NoahStapp Jan 29, 2025

NoahStapp left a comment

NoahStapp Feb 5, 2025

sleepyStick Feb 5, 2025

NoahStapp Feb 6, 2025 •

edited

Loading

sleepyStick Feb 6, 2025

NoahStapp commented Feb 6, 2025

		return [scenario_def]


		test_creator = CMAPSpecTestCreator(create_test, AsyncTestCMAP, AsyncTestCMAP.TEST_PATH)

PYTHON-5073 Convert test.test_connection_monitoring to async #2087

Are you sure you want to change the base?

PYTHON-5073 Convert test.test_connection_monitoring to async #2087

Conversation

sleepyStick commented Jan 28, 2025

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

NoahStapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp commented Feb 6, 2025

NoahStapp Feb 6, 2025 •

edited

Loading