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-4651: Migrate test_client_context.py to async #1819

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

sleepyStick
Copy link
Contributor

No description provided.

@sleepyStick
Copy link
Contributor Author

I think the failing tests are all related to PYTHON-4701

@sleepyStick sleepyStick marked this pull request as ready for review August 28, 2024 20:51
@sleepyStick sleepyStick changed the title PYTHON-4651 - Migrate test_client_context.py to async PYTHON-4651: Migrate test_client_context.py to async Aug 28, 2024
@ShaneHarvey
Copy link
Member

Strange the rhel8-pr-assign-reviewer task ran but didn't assign a reviewer.

@sleepyStick
Copy link
Contributor Author

Oh i assumed it was cuz the tests/checks weren't passing? Because it won't let me assign / add reviews either..?

class TestAsyncClientContext(AsyncUnitTest):
@classmethod
async def _setup_class(cls):
pass
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove _setup_class since it's just a pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AsyncUnitTest assumes that _setup_class would be implemented..without this the code would infinitely recurse and eventually error.

To my knowledge this is the first time that a class inherits from AsyncUnitTest that doesn't "need" a _setup_class. I don't know if we want to change AsyncUnitTest to accommodate this class's use case or not...

Copy link
Member

@ShaneHarvey ShaneHarvey Aug 28, 2024

Choose a reason for hiding this comment

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

The AsyncUnitTest methods should pass instead of trying to recursively call itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That work's too, don't know why i didn't think of that earlier! Just changed :)

@blink1073
Copy link
Member

Strange the rhel8-pr-assign-reviewer task ran but didn't assign a reviewer.

That's intentional in draft mode

@ShaneHarvey
Copy link
Member

But the PR was moved out of draft mode. Shouldn't the reviewer be auto-assigned in that case?

@blink1073
Copy link
Member

But the PR was moved out of draft mode. Shouldn't the reviewer be auto-assigned in that case?

Not unless the EVG task is re-run.

@ShaneHarvey
Copy link
Member

Oh TIL. I thought it worked the other way.

@blink1073
Copy link
Member

Oh TIL. I thought it worked the other way.

Not until we can have other GitHub events trigger EVG runs, or we make our own service listener with a webhook.

Copy link
Contributor

@NoahStapp NoahStapp left a comment

Choose a reason for hiding this comment

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

Great work! The async test_client_context.py should also be run here as part of the test_sdist PR hook.

ShaneHarvey
ShaneHarvey previously approved these changes Aug 29, 2024
@@ -209,4 +209,4 @@ jobs:
ls
which python
pip install -e ".[test]"
PYMONGO_MUST_CONNECT=1 pytest -v test/test_client_context.py
PYMONGO_MUST_CONNECT=1 pytest -k client_context
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PYMONGO_MUST_CONNECT=1 pytest -k client_context
PYMONGO_MUST_CONNECT=1 pytest -v -k client_context

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency across our test runs.

Copy link
Member

Choose a reason for hiding this comment

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

Is that something we can set in pyproject?

@sleepyStick sleepyStick merged commit ba8a139 into mongodb:master Sep 3, 2024
31 of 34 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.

4 participants