Skip to content

Conversation

@NoahStapp
Copy link
Contributor

The failing test test/asynchronous/test_collection.py::AsyncTestCollection::test_find_one_and_write_concern will be fixed by #1678.

blink1073
blink1073 previously approved these changes Jun 24, 2024
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@NoahStapp NoahStapp requested a review from blink1073 June 25, 2024 00:05
@blink1073
Copy link
Member

TypeError: Listeners for event_listeners must be either a CommandListener, ServerHeartbeatListener, ServerListener, TopologyListener, or ConnectionPoolListener.

@NoahStapp
Copy link
Contributor Author

NoahStapp commented Jun 25, 2024

TypeError: Listeners for event_listeners must be either a CommandListener, ServerHeartbeatListener, ServerListener, TopologyListener, or ConnectionPoolListener.

That error will be fixed by #1678, which pulls all of the Listener types out of the asynchronous/synchronous directories.

I've added a temporary fix for the issue to be removed by #1678.

@NoahStapp NoahStapp requested a review from blink1073 June 25, 2024 16:39
blink1073
blink1073 previously approved these changes Jun 25, 2024
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!


type_replacements = {"_Condition": "threading.Condition"}

import_replacements = {"test.synchronous": "test"}
Copy link
Contributor Author

@NoahStapp NoahStapp Jun 25, 2024

Choose a reason for hiding this comment

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

This is to reduce the code duplication for the conversion process, due to the bulk of our tests still using the original test/__init__.py and associated helpers. Once we've fully migrated all tests into the test/asynchronous and test/synchronous directories, we will switch to using test.synchronous.<module> again.

blink1073
blink1073 previously approved these changes Jun 25, 2024
# https://docs.pytest.org/en/stable/how-to/capture-stdout-stderr.html
python -m pytest -v --capture=tee-sys --durations=5 --maxfail=10 $TEST_ARGS
python -m pytest -v --capture=tee-sys --durations=5 --maxfail=10 test/synchronous/ $TEST_ARGS
if [ -z "$TEST_ARGS" ]; then # TODO: remove this once test/__init__.py doesn't share with test/synchronous/__init__.py
Copy link
Member

Choose a reason for hiding this comment

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

Is there a ticket for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, created PYTHON-4528 and replaced the TODO with the ticket name.

@NoahStapp NoahStapp requested a review from blink1073 June 26, 2024 16:53
@blink1073
Copy link
Member

Merge conflict, ☹️

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@NoahStapp NoahStapp merged commit b035c9c into mongodb:master Jun 26, 2024
test
commands =
pytest -v --durations=5 --maxfail=10 {posargs}
pytest -v --durations=5 --maxfail=10 test/synchronous/ {posargs}
Copy link
Member

Choose a reason for hiding this comment

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

Does running pytest twice generate two XML reports? Which one gets uploaded?

There should be a way to run both of these in one pytest command, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'm not sure how the XML reports interact. I'll open a follow-up PR with a better solution.

Copy link
Member

Choose a reason for hiding this comment

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

Have you opened a ticket for this yet? The second XML file overwrites the first and there are actually other problems too. Like any test that relies on posargs gets run twice, for example the test_auth_oidc.py file gets run twice: https://evergreen.mongodb.com/task/mongo_python_driver_oidc_auth_test__platform~macos_oidc_auth_test_patch_636603f89371e626c41e48504c986e53f5122aaf_6696cbe31505d30007b20aae_24_07_16_19_39_11

Copy link
Contributor Author

@NoahStapp NoahStapp Jul 16, 2024

Choose a reason for hiding this comment

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

I haven't, sorry--this slipped past me. I don't know if there is a way to run this all in one command while keeping the existing behavior: we want to run all tests in test/ and test/asynchronous by default, but override both those directories with whatever tests are specified. We'd need a command that runs pytest ... test/ test/asynchronous/ when posargs is empty, but runs pytest ... posargs when posargs is present. The best approach I can think of is to add a check on posargs that runs one of two separate commands.

Copy link
Member

Choose a reason for hiding this comment

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

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