PYTHON-5055 - Convert test_client.py to unittest #2074

NoahStapp · 2025-01-23T20:00:14Z

Main points of note:

All stateful unittest methods have been converted to pytest fixtures.
Every async test and fixture runs on the same loop that exists for the entire test session. This is separate from the pytest caching scope for fixtures, which is controlled by scope instead of loop_scope.
ClientContext has been kept as a metadata store for our tests served by a session-scoped fixture.
unit and integration pytest marks have been added. Going forward, all test classes should be marked with the appropriate one.

…meout on server selection

blink1073 · 2025-01-24T19:14:05Z

evergreen retry

sleepyStick

I think the Python changes look good to me. (I found things that I needed to change on my PR / test conversions as a result of reviewing this so that was helpful haha)

NoahStapp · 2025-01-27T19:16:04Z

The failing test.test_write_concern.TestWriteConcern.test_invalid test is due to async teardown of our existing unittest suite. I believe this is due to the teardown occurring at the very end of the entire test session rather than at the end of the asynchronous tests. I've opened pytest-dev/pytest-asyncio#1052 due to issues scoping our teardown fixture to the test/asynchronous subpackage.

ShaneHarvey · 2025-01-27T19:15:14Z

.evergreen/run-tests.sh

+    uv run ${UV_ARGS[*]} pytest "${ASYNC_PYTEST_ARGS[@]}" "--collect-only"
+    collected=$?
+    set -o errexit
+    # If we collected at least one async test, run all collected tests


What's the benefit using the "collect then run" approach? Wouldn't it be simpler to run pytest even if all tests are skipped?

Good point. My original intention was to avoid the error-handling that would require, but this approach doesn't avoid that either.

ShaneHarvey · 2025-01-27T22:53:15Z

test/utils.py

@@ -826,7 +827,7 @@ def lazy_client_trial(reset, target, test, get_client):
    with frequent_thread_switches():
        for _i in range(NTRIALS):
            reset(collection)
-            lazy_client = get_client()
+            lazy_client = client


I don't think this test is the same anymore. It's supposed to create a new client each iteration but this new code looks to reuse the same client.

Good catch, fixed.

ShaneHarvey · 2025-01-27T22:58:06Z

test/asynchronous/pymongo_mocks.py

@@ -166,7 +166,8 @@ async def get_async_mock_client(
            standalones, members, mongoses, hello_hosts, arbiters, down_hosts, *args, **kwargs
        )

-        await c.aconnect()
+        if "connect" not in kwargs or "connect" in kwargs and kwargs["connect"]:


Suggest if kwargs.get("connect", True):

ShaneHarvey · 2025-01-27T23:04:28Z

test/test_client.py

+
+
+@pytest.mark.usefixtures("require_integration")
+@pytest.mark.integration


Is there a simple way to avoid needing to mark the test as "integration" twice?

@pytest.mark.usefixtures("require_integration") isn't actually applying a mark, it's calling the require_integration as a requirement for the decorated test.

pytest -m require_integration won't select any tests, but pytest -m integration will.

I see this comment from the PR description is related:

unit and integration pytest marks have been added. Going forward, all test classes should be marked with the appropriate one.

If integration tests need to be tagged with require_integration, what's the benefit of using the unit vs integration marks?

Using the integration and unit marks allows us to run only that set of tests. If we want to add additional subsets of tests that cross suites in the future, having those marks will be a good first step.

ShaneHarvey · 2025-01-27T23:06:32Z

.evergreen/run-tests.sh

-      PYTEST_ARGS="-m $TEST_SUITES $PYTEST_ARGS"
+      # Workaround until unittest -> pytest conversion is complete
+      if [[ "$TEST_SUITES" == *"default_async"* ]]; then
+        ASYNC_PYTEST_ARGS=("-m asyncio" "--junitxml=xunit-results/TEST-asyncresults.xml" "${PYTEST_ARGS[@]}")


What about coverage? Does that pick up both test runs automatically?

The coverage runs pick up both test cases: https://spruce.mongodb.com/task/mongo_python_driver_test_rhel8_python3.9_cov_test_8.0_replica_set_noauth_ssl_sync_async_patch_dc182310dabff470dbbfc7da3c09eb6a4e08dfed_6797d0671d810000071bc171_25_01_27_18_28_57?execution=0&sortBy=STATUS&sortDir=ASC.

This shows it's not working as expected. The second coverage run will overwrite the .coverage output file by default: https://coverage.readthedocs.io/en/7.6.10/cmd.html#data-file

Good catch, does it make sense to use --append on the second run to keep the single file?

The alternative would be to use a separate file with COVERAGE_FILE and then coverage combine the two.

NoahStapp added 14 commits January 23, 2025 14:37

PYTHON-5044 - Successive AsyncMongoClients on a single loop always ti…

2b058a2

…meout on server selection

Only join executors on async

45e74da

Remove unneeded reset_async_client_context

0296c20

Convert test_client to pytest

266b0a3

TestAsyncClientUnitTest done

b6baf79

TestAsyncClientIntegrationTest converted

cca705d

Asynchronous test_client.py done

ae650e0

Fix fixture scopes

8402799

test_client.py conversion complete

048edf2

Lots of cleanup

84211e7

Fix pytest invocations

d3c053c

Workflow updates for asyncio tests

d2620be

Cleanup

7ee03c9

Typing fixes

b9d98c9

NoahStapp requested review from ShaneHarvey and sleepyStick January 23, 2025 20:22

NoahStapp added 9 commits January 23, 2025 15:31

Fix supports_secondary_read_pref

41fe61e

Fix async pytest invocation for EG

47b8c9d

Remove executor closes

9e115be

Remove executor cancel from close

8602dde

run-tests.sh fix for async

cc3f1ad

Merge branch 'master' into PYTHON-5036

46e7436

Fix uv.lock

d906c3b

Remove mock.patch decorator

8efa6ec

test_iteration regex

26b1178

NoahStapp closed this Jan 24, 2025

NoahStapp reopened this Jan 24, 2025

NoahStapp added 2 commits January 24, 2025 14:35

run-tests.sh hacking for compatibility

9694239

run-tests.sh should use arrays for arguments

9c3939b

TEST_ARGS should also use array

1a643c3

sleepyStick reviewed Jan 24, 2025

View reviewed changes

NoahStapp added 3 commits January 27, 2025 09:43

Undo unintended test/__init__.py changes

90b4693

More run-tests.sh fixes

0791a8f

Errexit hacking

3f1a86c

ShaneHarvey reviewed Jan 27, 2025

View reviewed changes

Address review

077a1cb

NoahStapp requested review from ShaneHarvey and sleepyStick January 28, 2025 15:19

sleepyStick approved these changes Jan 28, 2025

View reviewed changes

NoahStapp closed this Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PYTHON-5055 - Convert test_client.py to unittest #2074

PYTHON-5055 - Convert test_client.py to unittest #2074

NoahStapp commented Jan 23, 2025 •

edited

Loading

blink1073 commented Jan 24, 2025

sleepyStick left a comment

NoahStapp commented Jan 27, 2025 •

edited

Loading

ShaneHarvey Jan 27, 2025

NoahStapp Jan 28, 2025

ShaneHarvey Jan 27, 2025

NoahStapp Jan 28, 2025

ShaneHarvey Jan 27, 2025

ShaneHarvey Jan 27, 2025

NoahStapp Jan 28, 2025

ShaneHarvey Jan 28, 2025

NoahStapp Jan 28, 2025

ShaneHarvey Jan 27, 2025

NoahStapp Jan 28, 2025

ShaneHarvey Jan 28, 2025

NoahStapp Jan 28, 2025 •

edited

Loading

NoahStapp Jan 28, 2025



		@pytest.mark.usefixtures("require_integration")
		@pytest.mark.integration

PYTHON-5055 - Convert test_client.py to unittest #2074

PYTHON-5055 - Convert test_client.py to unittest #2074

Conversation

NoahStapp commented Jan 23, 2025 • edited Loading

blink1073 commented Jan 24, 2025

sleepyStick left a comment

Choose a reason for hiding this comment

NoahStapp commented Jan 27, 2025 • 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

NoahStapp Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp commented Jan 23, 2025 •

edited

Loading

NoahStapp commented Jan 27, 2025 •

edited

Loading

NoahStapp Jan 28, 2025 •

edited

Loading