Skip to content
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

PYTHON-1714 Add c extension use to client metadata #1874

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aclark4life
Copy link

@aclark4life aclark4life commented Sep 23, 2024

  • 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

@aclark4life aclark4life changed the title Add c extension use to client metadata PYTHON-1714: Add c extension use to client metadata Sep 23, 2024
@aclark4life aclark4life changed the title PYTHON-1714: Add c extension use to client metadata PYTHON-1714 Add c extension use to client metadata Sep 23, 2024
test/asynchronous/test_client.py Outdated Show resolved Hide resolved
@@ -341,71 +341,68 @@ async def test_read_preference(self):
)
self.assertEqual(c.read_preference, ReadPreference.NEAREST)

async def test_metadata(self):

def _test_metadata(self, add_meta, *args, **kwargs):
Copy link
Member

@ShaneHarvey ShaneHarvey Sep 24, 2024

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

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.

Copy link
Author

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|async string to synchro.py too.

Shouldn't we see a failure since it's not added there yet?

Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants