-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-4789 Migrate test_retryable_reads.py to async #1877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
client = await self.async_rs_or_single_client( | ||
maxPoolSize=1, event_listeners=[cmap_listener, cmd_listener] | ||
) | ||
self.addAsyncCleanup(client.close) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to add explicit cleanup when using the helper methods to create clients, they do the cleanup for us.
for mongos in async_client_context.mongos_seeds().split(","): | ||
client = await self.async_rs_or_single_client(mongos) | ||
await async_set_fail_point(client, fail_command) | ||
self.addAsyncCleanup(client.close) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
_IS_SYNC = False | ||
|
||
# Location of JSON test specifications. | ||
_TEST_PATH = os.path.join(os.path.dirname(os.path.realpath(__file__)), "retryable_reads", "legacy") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path will be for test/asynchronous/retryable_reads...
which doesn't exist, it needs to look in the top-level test directory and not the test/asynchronous one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! I fixed it (I think), but uh why is _TEST_PATH
not used anywhere in the file? (like as far as I can tell, its just defined but never used??)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I actually think our retryable read tests are fully converted to unified, so we can remove _TEST_PATH
here entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it :)
afb7eaa
to
0b7f216
Compare
0b7f216
to
00d231e
Compare
00d231e
to
61591fd
Compare
Note: I made
test_pool_paused_error_is_retryable
require sync because it uses threads, we can consider changing it to tasks too though!