Upgrade test dependencies pytest and pytest-asyncio #1829
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1829 +/- ##
==========================================
+ Coverage 79.69% 86.19% +6.50%
==========================================
Files 31 33 +2
Lines 4324 4391 +67
Branches 780 465 -315
==========================================
+ Hits 3446 3785 +339
+ Misses 721 428 -293
- Partials 157 178 +21 ☔ View full report in Codecov by Sentry. |
|
The macOS and Linux test failures appear to show |
There was a problem hiding this comment.
I think we want to use the storage backend loop still ... If I get some time I'll try revert that and see what error we get since I think your key check of
asyncio_default_fixture_loop_scope = function
was the big fix we needed as we need lots of event loops with how our tests are written ...
src/bandersnatch/delete.py
Outdated
| if dry_run: | ||
| logger.info(f" rm {blob_path}") | ||
| return 0 | ||
| blob_exists = await storage_backend.loop.run_in_executor( | ||
| blob_exists = await loop.run_in_executor( |
There was a problem hiding this comment.
I bet the backend loop is being patch / mocked or something to make the tests work. Especially for swift (I do need to delete swift support - no one is helping support it).
So I bet we don't want to do this change and workout why the backend's loop is failing here ...
|
I can only repro locally on my Mac running latest minio with py3.11 ... on py3.12 it works ... which is interesting .... |
This upgrades the two packages together:
- pytest => 8.x
- pytest-asyncio => 0.24.x
Commands used:
pip install -U pytest pytest-asyncio
pip freeze -r requirements_test.txt
0.24.x of pytest-asyncio adds a deprecation warning for the default event loop fixture scope changing from fixture-cache to per-function. This pre-emptively sets this option to align with the upcoming default.
Fixes order-dependent test failures in test_main.py caused by tests that tried to initialize the singleton BandersnatchConfig instance when it had already been created. This left the config out of sync with what the tests expected. Some tests in this module were already doing this with a call to a 'setup' function that erased the Singleton metaclass's stored instances. This moves that into a pytest fixture and applies it to the whole module.
Tests that are not coroutines themselves may require an event loop to exist for code that uses get_event_loop/get_running_loop.
Adds an autouse fixture at the module scope that removes all loaded storage plugins. This prevents plugin instances with closed event loops from being re-used across tests. Also adds a non-autouse fixture for resetting the storage plugins at more narrow scopes when required.
flyinghyrax
force-pushed
the
update-pytest-asyncio
branch
from
63fc8e4 to
a4dbbad
Compare
|
I'm interested in adding |
There was a problem hiding this comment.
Awesome work and many many thanks @flyinghyrax ... This is a huge cleanup with out test suite and will help keeping it cleaner in the future.
Also, love the comment explaining why stuff was done. Can't click merge fast enough!
I will try get a release out over the holiday break finally for 3.12 and start work on making bandersnatch work for 3.13,.
|
You're welcome! I'm very happy that I was able to get things working with relatively small changes. 🙂 |
Dependency Upgrades
This performs the same version changes as #1656 and #1797 in a single merge request, since the associated issues and fixes appear to be related:
pytest=> 8.3.4pytest-asyncio=> 0.24.0This is a major version increment for pytest and a 0ver minor version increment for pytest-asyncio.
Unit Test Changes
Locally, I used
pytest-randomlyto shuffle the test execution order. Varying the test order uncovered a number of places where shared state was causing order-dependent failures.Tests that indirectly invoke a call to
get_running_loop/get_event_looprequire an event loop to be available even if the test itself and the code under test is not asynchronous. Depending on execution order an event loop was not always active when needed. I addedasynciomarks to test modules wherever tests were failing because of this (or for 3.12, displaying aDeprecationWarningwhen there was no current event loop).Storage plugin instances store the current event loop as an attribute, and we cache initialized plugins globally. This led to some situations where
I created fixtures to clear cached storage plugins between test modules. Most problems seemed solved by clearing between modules and I made that an
autousefixture. For one function intest_deletethe scope needed to be narrower and there's a comment inline explaining that case.