PYTHON-5090 Convert test.test_monitor to async#2106
PYTHON-5090 Convert test.test_monitor to async#2106sleepyStick merged 16 commits intomongodb:masterfrom
Conversation
| class TestMonitor(AsyncIntegrationTest): | ||
| async def create_client(self): | ||
| listener = ServerAndTopologyEventListener() | ||
| client = await self.unmanaged_async_single_client(event_listeners=[listener]) |
There was a problem hiding this comment.
| client = await self.unmanaged_async_single_client(event_listeners=[listener]) | |
| client = await self.async_single_client(event_listeners=[listener]) |
Avoid using the unmanaged helper methods wherever possible.
There was a problem hiding this comment.
looking at the tests, i thought it was intentional to use unmanaged because the tests want to explicitly close the client and test behavior after client close (or client del)?
There was a problem hiding this comment.
Does it cause failures to switch the call from unmanaged? Calling close multiple times shouldn't affect the client here.
There was a problem hiding this comment.
Yes we have to used unmanaged here, otherwise the client will never get garbage collected.
There was a problem hiding this comment.
Adding client.close to the test cleanup will keep a reference to the client and prevent it from being GCd.
There was a problem hiding this comment.
Ah, the reference inside the cleanups list created by the managed helper will cause the del client call to not GC the client?
There was a problem hiding this comment.
Oh wait: a GC'd AsyncMongoClient will trigger the unclosed client warning, as seen in the failing test.
There was a problem hiding this comment.
We still want to make sure that a GC'd async client's tasks eventually get cleaned up.
|
okay in summary, we need the unmanaged async client for the test, because we want to test that the executors get GCd on client del and client.close. if we used managed, the client will not get GCd. on pypy GC is non-deterministic hence the failure. with that said, i believe the only failure is because of that and we should either skip this test in pypy (if so how?) or do something else to mitigate the failing test? |
| for ref, name in executor_refs: | ||
| await async_wait_until( | ||
| partial(unregistered, ref), f"unregister executor: {name}", timeout=5 | ||
| ) |
There was a problem hiding this comment.
Can we add an extra async_wait_until here that waits for the __del__ warning to be raised? That would avoid the warning being accidentally raised later on in a subsequent test like we see in the most recent run: https://github.com/mongodb/mongo-python-driver/actions/runs/13466188001/job/37632412774?pr=2106
There was a problem hiding this comment.
Like:
with warnings.catch_warnings(record=True) as w:
...
wait until the "Call AsyncMongoClient.close() to safely shut down your client and free up resources" warning appears in w.
NoahStapp
left a comment
NoahStapp
left a comment
There was a problem hiding this comment.
Great work! LGTM, assuming passing tests.
|
The windows test failures are from |
3 participants
No description provided.