Refactor integration tests to have no side-effects

[dhruvkb]

Description

This PR refactors the integration tests.

• Moves integration tests from the root directory into a separate integration/ directory within api/tests/.
• Combines image and audio tests into parametrised versions in a single test suite.
• Deletes inactive and outdated tests.
• Uses api_client instead of requests to speed up tests and also enable mocking/patching.
• Deduplicates fixtures by moving them to conftest.py

Testing Instructions

A passing CI suite is proof that the changes work, because this PR only affects tests.

Checklist

• My pull request has a descriptive title (not a vague title likeUpdate index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.
• I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[dhruvkb]

@sarayourfriend I need your help with one thing here.

This PR changes request.gets to client.gets. This means that when you run the following command

$ just api/test test/integration/

...the following errors come up.

Screenshot

I believe this is because once the tests have completed the test runner just exits without cleaning up the tasks threads. The tests still pass, which is great, but I would appreciate if you could take a look into how we can fix these errors.

They don't appear when running all the tests with the following command...

$ just api/test

...because this gives enough time for the tasks created during the ingestion tests to complete while pytest is running the unit tests.

[sarayourfriend]

@dhruvkb I'm not totally sure, but one thing I'd try first is to use api_client instead of client. Even if that doesn't fix it, I think it's still the right option because api_client ensures all the DRF stuff is initialised.

The specific error there about ClientSession is meant to be handled by ensure_asgi_lifecycle, so it seems like that fixture isn't working for the integration test context, for some reason. Have you confirmed that the fixtures in test/integration/conftest that are set to autouse are definitely being auto-used? I'm wondering if somehow importing them from a different context isn't working correctly (seems far fetched, but it's where I would start). The fixtures that are shared between the integration and unit tests could be added to test/conftest, FWIW, removing the need to add shared fixtures in two places. If there's something strange happening with the imported context that might be the way to fix it? I'm wondering if pytest sees the same function object but then for some reason only associates it with the first one it read? 🤷 Total shot in the dark though, I really have no idea why it wouldn't work with the current code, it definitely looks like it should work the way we expect it to.

I'll look more closely at this later today though, see if I can find out anything beyond these guesses.

[sarayourfriend]

Trying this out locally, and noticing something interesting! I always use the -k flag to filter tests locally. If I run these as just api/test test/integration, like you've done, I get the errors. If I run them as just api/test -k integration I have zero issues with the async loop not closing.

Here it is using -k

➜  openverse git:(int_test_using_client---potential_fixes) just api/test -k test/integration
env COMPOSE_PROFILES="api" just ../up 
env COMPOSE_PROFILES="api" docker-compose -f docker-compose.yml up -d
[+] Running 9/9
 ✔ Container openverse-cache-1             Running                                                                                                                                              0.0s 
 ✔ Container openverse-upstream_db-1       Running                                                                                                                                              0.0s 
 ✔ Container openverse-db-1                Running                                                                                                                                              0.0s 
 ✔ Container openverse-es-1                Running                                                                                                                                              0.0s 
 ✔ Container openverse-indexer_worker-1    Running                                                                                                                                              0.0s 
 ✔ Container openverse-web-1               Running                                                                                                                                              0.0s 
 ✔ Container openverse-ingestion_server-1  Running                                                                                                                                              0.0s 
 ✔ Container openverse-nginx-1             Running                                                                                                                                              0.0s 
 ✔ Container openverse-proxy-1             Started                                                                                                                                              0.0s 

================================================================================
                                 Service Ports                                  
================================================================================
nginx (nginx):
-  http://0.0.0.0:50270 (→ 8080)
web (api):
-  http://0.0.0.0:50280 (→ 8000)
ingestion_server (ingestion_server):
-  http://0.0.0.0:50281 (→ 8001)
upstream_db (upstream_db):
-  http://0.0.0.0:50255 (→ 5432)
db (postgres):
-  http://0.0.0.0:50254 (→ 5432)
es (docker.elastic.co/elasticsearch/elasticsearch):
-  http://0.0.0.0:50292 (→ 9200)
cache (redis):
-  http://0.0.0.0:50263 (→ 6379)
================================================================================
just ../ingestion_server/wait # API profile includes ingestion server
just wait # API waits for ES in entrypoint
env DC_USER="opener" just ../exec web pytest -k test/integration
just dc exec -u opener  web pytest -k test/integration
env COMPOSE_PROFILES="api,ingestion_server,frontend,catalog" docker-compose -f docker-compose.yml exec -u opener web pytest -k test/integration
Test session starts (platform: linux, Python 3.11.6, pytest 7.4.3, pytest-sugar 0.9.7)
django: version: 4.2.7, settings: conf.settings (from env)
rootdir: /api
configfile: pytest.ini
plugins: Faker-20.1.0, anyio-4.0.0, django-4.7.0, raises-0.11, sugar-0.9.7
collected 700 items / 597 deselected / 103 selected                                                                                                                                                 

 test/integration/test_audio_integration.py ✓✓                                                                                                                                          2% ▎         
 test/integration/test_auth.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                                                                                  94% █████████▌
 test/integration/test_dead_link_filter.py ✓✓✓✓✓✓                                                                                                                                      28% ██▊       
 test/integration/test_image_integration.py ✓✓✓✓✓                                                                                                                                      33% ███▍      
 test/integration/test_media_integration.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                             93% █████████▍
 test/integration/test_deprecations.py ✓✓✓✓✓✓                                                                                                                                         100% ██████████

Results (34.38s):
     103 passed
     597 deselected

Here it is without -k

➜  openverse git:(int_test_using_client---potential_fixes) just api/test test/integration 
env COMPOSE_PROFILES="api" just ../up 
env COMPOSE_PROFILES="api" docker-compose -f docker-compose.yml up -d
[+] Running 9/9
 ✔ Container openverse-cache-1             Running                                                                                                                                              0.0s 
 ✔ Container openverse-db-1                Running                                                                                                                                              0.0s 
 ✔ Container openverse-upstream_db-1       Running                                                                                                                                              0.0s 
 ✔ Container openverse-es-1                Running                                                                                                                                              0.0s 
 ✔ Container openverse-web-1               Running                                                                                                                                              0.0s 
 ✔ Container openverse-nginx-1             Running                                                                                                                                              0.0s 
 ✔ Container openverse-proxy-1             Started                                                                                                                                              0.0s 
 ✔ Container openverse-indexer_worker-1    Running                                                                                                                                              0.0s 
 ✔ Container openverse-ingestion_server-1  Running                                                                                                                                              0.0s 

================================================================================
                                 Service Ports                                  
================================================================================
nginx (nginx):
-  http://0.0.0.0:50270 (→ 8080)
web (api):
-  http://0.0.0.0:50280 (→ 8000)
ingestion_server (ingestion_server):
-  http://0.0.0.0:50281 (→ 8001)
upstream_db (upstream_db):
-  http://0.0.0.0:50255 (→ 5432)
db (postgres):
-  http://0.0.0.0:50254 (→ 5432)
es (docker.elastic.co/elasticsearch/elasticsearch):
-  http://0.0.0.0:50292 (→ 9200)
cache (redis):
-  http://0.0.0.0:50263 (→ 6379)
================================================================================
just ../ingestion_server/wait # API profile includes ingestion server
just wait # API waits for ES in entrypoint
env DC_USER="opener" just ../exec web pytest test/integration
just dc exec -u opener  web pytest test/integration
env COMPOSE_PROFILES="api,ingestion_server,frontend,catalog" docker-compose -f docker-compose.yml exec -u opener web pytest test/integration
Test session starts (platform: linux, Python 3.11.6, pytest 7.4.3, pytest-sugar 0.9.7)
django: version: 4.2.7, settings: conf.settings (from env)
rootdir: /api
configfile: pytest.ini
plugins: Faker-20.1.0, anyio-4.0.0, django-4.7.0, raises-0.11, sugar-0.9.7
collected 103 items                                                                                                                                                                                 

 test/integration/test_audio_integration.py ✓✓                                                                                                                                          2% ▎         
 test/integration/test_auth.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                                                                                  94% █████████▌
 test/integration/test_dead_link_filter.py ✓✓✓✓✓✓                                                                                                                                      28% ██▊       
 test/integration/test_image_integration.py ✓✓✓✓✓                                                                                                                                      33% ███▍      
 test/integration/test_media_integration.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                             93% █████████▍
 test/integration/test_deprecations.py ✓✓✓✓✓✓                                                                                                                                         100% ██████████

Results (33.50s):
     103 passed
Exception ignored in: 
Traceback (most recent call last):
  File "/venv/lib/python3.11/site-packages/aiohttp/client.py", line 363, in __del__
  File "/usr/local/lib/python3.11/asyncio/base_events.py", line 1804, in call_exception_handler
  File "/usr/local/lib/python3.11/logging/__init__.py", line 1518, in error
  File "/usr/local/lib/python3.11/logging/__init__.py", line 1634, in _log
  File "/usr/local/lib/python3.11/logging/__init__.py", line 1644, in handle
  File "/usr/local/lib/python3.11/logging/__init__.py", line 1706, in callHandlers
  File "/usr/local/lib/python3.11/logging/__init__.py", line 974, in handle
  File "/usr/local/lib/python3.11/logging/__init__.py", line 830, in filter
  File "/venv/lib/python3.11/site-packages/log_request_id/filters.py", line 12, in filter
  File "/venv/lib/python3.11/site-packages/asgiref/local.py", line 101, in __getattr__
  File "/venv/lib/python3.11/site-packages/asgiref/local.py", line 81, in _get_storage
  File "/venv/lib/python3.11/site-packages/asgiref/local.py", line 49, in _get_context_id
ImportError: sys.meta_path is None, Python is likely shutting down

.... ETC cut of for brevity, but there's heaps of these tracebacks

I have no idea why this would cause a difference in the way Pytest configures the environment for the tests and the documentation doesn't say anything obvious at first glance... but I've only done a cursory search of the docs and would need to spend more time. What is weird is the setup/environment stuff pytest logs before running the tests looks identical to me, but again, I'd need to take a much closer look to see if there aren't small differences that could explain this.

I made a bunch of changes in #3560 to see if they would change this, but they didn't. So, use whatever you'd like from there, but they don't fix the issue that appears when targeting using a path rather than a pattern.

[dhruvkb]

Fascinating, I didn't think the behaviour with -k or with the directory pattern would be different and I hadn't thought to try. Thanks for investigating this!

Also, in any case, your PR is really good so we should definitely merge it.

[AetherUnbound]

@dhruvkb Should we close this or revisit it?

[dhruvkb]

It is a net positive, doesn't break CI and I feel the refactoring is quite clean. I would like to revive this. Also @sarayourfriend contributed to this PR so maybe let's factor in what Sara thinks about reviving and possibly merging this?

[sarayourfriend]

I think it's a great change. I don't know how many people rely on passing a filepath directly instead of using -k. I didn't even know pytest worked as expected if you didn't use -k (and in fact, I believe it does not work as expected), but apparently others have relied on this pattern. Just a matter of education if it is a widespread pattern people use, otherwise, there's no reason not to make this change.

[sarayourfriend]

@dhruvkb Just a heads up that the PR description is still "under construction 🚧" 😅

[dhruvkb]

Oops, sorry I missed that in the excitement of undrafting the PR. I've updated the description now.

[obulat]

Awesome refactor!

[AetherUnbound]

This is fantastic!! The way you leveraged a fixture for both media types is really excellent.