Skip to content

PYTHON-5382 - Add a test with min dependencies #2410

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

Merged
merged 14 commits into from
Jun 27, 2025

Conversation

NoahStapp
Copy link
Contributor

No description provided.

@NoahStapp NoahStapp marked this pull request as ready for review June 26, 2025 15:41
@NoahStapp NoahStapp requested a review from blink1073 June 26, 2025 16:21
@NoahStapp
Copy link
Contributor Author

Using uv sync --resolution=lowest-direct would try to install libmongocrypt and mockupdb. The approach taken here ensures we only install dnspython + pytest for the connect test suite.

Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

Where does this install the lowest version of dnspython we support? And don't we need to actually run a test suite that uses dnspython to confirm compatibility?

@NoahStapp
Copy link
Contributor Author

Where does this install the lowest version of dnspython we support? And don't we need to actually run a test suite that uses dnspython to confirm compatibility?

Good catch, the --resolution=lowest-direct tag didn't get included in the commit. The original ticket Steve opened suggested the connect tests or srv, agreed that srv is a better suite to run here.

@NoahStapp
Copy link
Contributor Author

dnspython didn't add resolver.resolve until 2.0.0: https://dnspython.readthedocs.io/en/latest/whatsnew.html#id12, but we explicitly test for it in test_import_dns_resolver. Added an or clause to the test to check for query instead for dnspython<2.0.0.

@NoahStapp NoahStapp requested a review from ShaneHarvey June 27, 2025 14:10
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

Nice!

uv venv
source .venv/bin/activate
uv pip install -e ".[test]" --resolution=lowest-direct
pytest -v test/test_srv_polling.py
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I just noticed this was test_srv_polling but test_dns would probably be a better choice. Or both? And should we do both sync and async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost all of the test_dns tests require a sharded or replica cluster. We can add it here anyway, but it'll only add a handful of tests. And good catch, we should run sync and async tests here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay then, could you add a comment explaining why we use test_srv_polling only and not test_dns?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like test_dns's mocking is not compatible with dnspython 1.0. Perhaps it's not worth it to add in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And unfortunately there's no resolver.query method for the async resolver. I'll add a comment that the async test_dns is not compatible with dnspython 1.x, which is consistent with our docs for the async API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a separate test for dnspython 2.0 for async support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up having to use dnspython 2.1.0, as 2.0.0 does not include the lifetime kwarg we pass to our resolve calls, but only for the async API.

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean we have to update our docs and error message to say >=2.1 is required for async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll file a follow-up ticket for those changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NoahStapp NoahStapp requested a review from ShaneHarvey June 27, 2025 15:47
@NoahStapp NoahStapp requested a review from a team as a code owner June 27, 2025 17:54
@NoahStapp NoahStapp merged commit 6a672d4 into mongodb:master Jun 27, 2025
77 of 78 checks passed
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.

2 participants