-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-4804 Migrate test_comment.py to async #1887
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
Not sure why the link check is failing...I don't think its anything I did? |
No, the docs team is working on a fix |
6ceed75
to
f6d2fcd
Compare
f6d2fcd
to
4120a15
Compare
test/asynchronous/test_comment.py
Outdated
listener.reset() | ||
kwargs = {"comment": cc} | ||
if h == coll.rename: | ||
_ = await db.get_collection("temp_temp_temp").drop() |
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.
Is this underscore needed?
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.
probably not! just removed it :)
test/asynchronous/test_comment.py
Outdated
if iscoroutinefunction(coll.create_index): | ||
await coll.create_index("a") | ||
else: | ||
coll.create_index("a") |
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.
if iscoroutinefunction(coll.create_index): | |
await coll.create_index("a") | |
else: | |
coll.create_index("a") | |
if not _IS_SYNC and isinstance(coll, Empty): | |
coll.create_index("a") | |
else: | |
await coll.create_index("a") |
For clarity so unfamiliar readers can understand why coll.create_index
could be synchronous.
test/asynchronous/test_comment.py
Outdated
if iscoroutinefunction(h): | ||
maybe_cursor = await h(*args, **kwargs) | ||
else: | ||
maybe_cursor = h(*args, **kwargs) |
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.
if iscoroutinefunction(h): | |
maybe_cursor = await h(*args, **kwargs) | |
else: | |
maybe_cursor = h(*args, **kwargs) | |
if not _IS_SYNC and iscoroutinefunction(h): | |
maybe_cursor = await h(*args, **kwargs) | |
else: | |
maybe_cursor = h(*args, **kwargs) |
Same here.
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.
LGTM!
Oops, didn't realize that merging would dismiss the review...sorry! |
test/test_comment.py
Outdated
from test import IntegrationTest, client_context, unittest | ||
from test.utils import EventListener | ||
|
||
from bson.dbref import DBRef | ||
from pymongo.operations import IndexModel | ||
from pymongo.synchronous.command_cursor import CommandCursor | ||
|
||
_IS_SYNC = True | ||
|
||
|
||
class Empty: |
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 Empty class looks very suspicious. What's the point of it?
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.
Uhhh honestly not sure. It was added in PYTHON-2682.
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.
cleaned up _test_ops
and now Empty
isn't needed anymore :)
test/test_comment.py
Outdated
coll.create_index("a") | ||
maybe_cursor = h(*args, **kwargs) | ||
if not _IS_SYNC and isinstance(coll, Empty): | ||
coll.create_index("a") |
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.
How can we call create_index when coll is Empty
?
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.
Thats a great question...HAHA code is removed now.
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.
LGTM assuming the tests pass.
No description provided.