Using --keepdb causes test failures

[florean]

Using the --keepdb options for testing, which saves about a minute of applying migrations, causes around a hundred test failures. It appears that some of the data created by migrations used by tests, namely users and courts, is getting deleted from the test database. If you also use --parallel, Django runs all tests against clones of the test database (test_courtlistener_1, etc.). In that case, the test database isn't changed and deleting those clones fixes the problem, but it isn't automatic, so you have to manually do that between test runs. So some test is deleting that data outside of a transaction.

One solution might be to convert tests to only use data they explicitly create, rather than relying on implicitly created objects from migrations, but it would probably be worth tracking down and eliminating the root cause.

Example runs using the command docker exec -e SELENIUM_DEBUG=1 -e SELENIUM_TIMEOUT=30 cl-django ./manage.py test cl --verbosity=3 --exclude-tag selenium --parallel=1 --timing --durations 10 --keepdb

initial run:

----------------------------------------------------------------------
Ran 1510 tests in 261.842s

OK (skipped=53)
Preserving test database for alias 'default' ('test_courtlistener_1')...
Preserving test database for alias 'default' ('test_courtlistener_2')...
Preserving test database for alias 'default' ('test_courtlistener')...
Total database setup took 62.094s
  Creating 'default' took 61.754s
  Cloning 'default' took 0.181s
  Cloning 'default' took 0.159s
Total database teardown took 0.000s
Total run took 327.935s

second run:

----------------------------------------------------------------------
Ran 1421 tests in 142.359s

FAILED (failures=5, errors=186, skipped=53)
Preserving test database for alias 'default' ('test_courtlistener_1')...
Preserving test database for alias 'default' ('test_courtlistener_2')...
Preserving test database for alias 'default' ('test_courtlistener')...
Total database setup took 0.693s
  Creating 'default' took 0.672s
  Cloning 'default' took 0.010s
  Cloning 'default' took 0.011s
Total database teardown took 0.000s
Total run took 145.636s

Some snipped example errors look like:

======================================================================
ERROR: test_make_a_docket_alert (cl.alerts.tests.tests.DocketAlertAPITests.test_make_a_docket_alert)
Can we make a docket alert?
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/venv/lib/python3.13/site-packages/django/test/testcases.py", line 1400, in setUpClass
    cls.setUpTestData()
    ^^^^^^^^^^^
  File "/opt/courtlistener/cl/alerts/tests/tests.py", line 2600, in setUpTestData
    cls.court = Court.objects.get(id="scotus")
    ^^^^^^^^^^^^^^^
cl.search.models.Court.DoesNotExist: Court matching query does not exist.

======================================================================
ERROR: test_nda_get_document_number_fallback (cl.recap.tests.test_recap_email.GetDocumentNumberForAppellateDocuments.test_nda_get_document_number_fallback)
This test verifies if we can get the PACER document number for
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.13/unittest/case.py", line 603, in _callSetUp
    self.setUp()
    ~~~~~~~~~~^^
  File "/opt/courtlistener/cl/recap/tests/test_recap_email.py", line 2906, in setUp
    self.user = User.objects.get(username="recap-email")
    ^^^^^^^^^^^^^^^
django.contrib.auth.models.User.DoesNotExist: User matching query does not exist.

[mlissner]

Looks like a tough one. I don't have any theories what this might be.

[florean]

Okay, so the problem is we load at least one test fixture in a migration, which makes it an implicit dependency for every test. But there are some tests, such as cl.api.tests.DRFCourtApiFilterTests, that want to use their own data, so they delete the existing stuff to clear the board. Unfortunately, delete() for whatever reason isn't captured in TestCase's transactions. It looks like they should be, Django says it puts deletes() in a transaction, but maybe that is what causes it to not work in TestCases? I thought being wrapped in TestCase's atomic would mean deletes got rolled back, but maybe they are escaping that somehow because they get committed? (I tried manually managing this in a TransactionTestCase, but couldn't get that to work, either.) Unless I'm missing something obvious, it seems like this would be a problem people have run in to, but I can't find any documentation on deletes not working in a TestCase. I can only guess that most people test against empty databases and so no one is typically deleting things in setUpTestData.

IMO, the correct solution is to remove the implicit dependency and make it explicit for tests that use it. That is what I am working on. The other option would be to adjust the tests that are using delete() so that their data gets around the implicit data. I dislike that approach for two reasons: implicit dependencies are hard to reason about and it requires changing test logic.

This PR will also be based on #5954, because that's a requirement to avoiding database clobbering.

[mlissner]

I can only guess that most people test against empty databases and so no one is typically deleting things in setUpTestData.

Yeah, that's surprising and maybe worth filing a bug with Django?

IMO, the correct solution is to remove the implicit dependency

I agree, except if this is a django bug, in which case the right solution is to fix it there too. :)

Thanks for tracking this down. Annoying problems!

[florean]

I didn't find any info on deletes, but I did find a ton of info on --keepdb breaking if you have a single TransactionTestCase in your test suite, starting with this 10-year-old bug. It's arguably correct that tests should be able to rely on the state of the database after all migrations are finished. And maybe adding universal test data in a migration is fine; I prefer not to, but it can be a tricky thing and most of this probably predates the use of factories.

[mlissner]

Oh yeah, that bug definitely looks like it'd cause trouble. My assumption has always been that migrations happen, then tests are run in transactions, then the DB is left as it started. Apparently not! We do have a long-running effort to get rid of the fixtures we use and rely solely on migrations. Any appetite for making that happen? If I recall the fixture that is run during migrations, I think it mostly just sets up a court or two, so I think switching to factories might not be horrible?

[florean]

Hmm, I think I'm confused about getting rid of fixtures and relying on migrations. At least two migrations use fixtures: search uses a fixture file and users creates one programmatically. Are you saying you don't want to rely on fixtures or migrations for test data? I believe I have a branch that removes the fixture from the search migration, but it mostly makes it a dependency of the tests that need it.

I think the best approach would be to convert any small fixtures to data mixins and have tests add in the mixins for the data they need. I'm happy to do that, but that would build on my refactor of TestCases, my refactor of mixins, and my refactor of factories. In particular, TestCases need to call their super()s for any of it to work.

[mlissner]

Oops, sorry. This:

We do have a long-running effort to get rid of the fixtures we use and rely solely on migrations

...was supposed to say "rely solely on factories."

Sorry, that was confusing. Yes, I think mixins is the way to go here, and to remove the data migrations that happen and that apparently cause trouble. Each test(case) should be able to stand on its own, without fixtures or magic that happens as part of the migration.