-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-1714 Add c extension use to client metadata #1874
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
test/asynchronous/test_client.py
Outdated
|
|
||
| async def test_metadata(self): | ||
|
|
||
| def _test_metadata(self, add_meta, *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.
These tests don't actually verify the expected driver "name". They also no longer verify all the other expected metadata fields since we've removed this line:
self.assertEqual(options.pool_options.metadata, metadata)
I'd suggest we change this test back to how it was and deal with the |c issue like this:
async def test_metadata(self):
if has_c():
name = "PyMongo|c|async"
else:
name = "PyMongo|async"
metadata = copy.deepcopy(_METADATA)
metadata["driver"]["name"] = name
metadata["application"] = {"name": "foobar"}
client = self.simple_client("mongodb://foo:27017/?appname=foobar&connect=false")
options = client.options
self.assertEqual(options.pool_options.metadata, metadata)
...We'll need to add the PyMongo|c|async string to synchro.py too.
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.
These tests don't actually verify the expected driver "name".
You mean you don't like the assertIn or …
They also no longer verify all the other expected metadata fields since we've removed this line:
self.assertEqual(options.pool_options.metadata, metadata)
I updated _test_handshake to cover those.
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.
Right, the new test only checks for a substring while the old test checks the whole thing.
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.
Also the new _test_handshake is brittle since we need to update it every time we add a new field to the metadata.
So the old way is better imo.
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.
Done, thanks! Much simpler, I agree. That leaves:
FAILED test/asynchronous/test_client.py::AsyncClientUnitTest::test_metadata - AssertionError: {'dri[19 chars]ongo|c|async|FooDriver', 'version': '4.10.0.de[167 chars]ar'}} != {'dri[19 chars]ongo|async|FooDriver', 'version': '4.10.0....
FAILED test/test_client.py::ClientUnitTest::test_metadata - AssertionError: {'dri[19 chars]ongo|c|FooDriver', 'version': '4.10.0.dev0|1.2[161 chars]ar'}} != {'dri[19 chars]ongo|FooDriver', 'version': '4.10.0.dev0|1...
So I'll add the same conditional for the FooDriver test.
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'll need to add the
PyMongo|c|asyncstring to synchro.py too.
Shouldn't we see a failure since it's not added there yet?
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.
Hmm I suppose something is already translating PyMongo|c|async to PyMongo|c? Perhaps unasync does that already?
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.
I was expecting to add a line here: https://github.com/mongodb/mongo-python-driver/blob/d0772f2/tools/synchro.py#L103
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.
Yeah I don't understand why the tests pass with or without that string, maybe @blink1073 knows.
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.
Synchro hadn't been run, it failed the static check: https://github.com/mongodb/mongo-python-driver/actions/runs/11023004771/job/30613463481?pr=1874#step:5:67
- Move `has_c` to common - If `has_c` in pool_options update driver metadata - If `has_c` in tests update driver metadata - Add c replacement string to synchro
has_cto commonhas_cin pool_options update driver metadatahas_cin tests update driver metadata