Fix a bunch of database-clobbering tests #5954
Conversation
Using UserProfileWithParentsFactory means you are not a SimpleTestCase. This is the cause of a number of test failures.
😱 This is why SimpleTestCase isn't supposed to have `databases` set. Although most of the tests that inherited from these also inherited from TestCase, wrapping their database access in transactions, cl.search.tests.tests_es_opinion.OpinionSearchAPICommonTests, cl.search.tests.tests_es_oral_arguments.OASearchAPICommonTests, cl.search.tests.tests_es_person.PeopleSearchAPICommonTests, and cl.search.tests.tests_es_recap.RECAPSearchAPICommonTests did not. I recognize a few of those from random test failures I've run into.
RecapFetchApiSerializationTestCase has created database objects and lost its Simple privileges.
RECAPDocumentSignalTests and RECAPDocumentReceiverTests have been found guilty of saving to the database and have been stripped of their Simple rank.
AlertAPITests goes directly to TestCase jail, does not change the database, and does not save 200ms.
For the crime of ruining countless test runs over the last four years, SimpleTestCase is sentenced to summary erasure by backspace key.
APITests was trying to secretly access the database without anyone knowing about it, but its malfeasance has been exposed.
DocketAlertAPITests broke the database and has to pay extra test time for restitution.
APIVisualizationTestCase has been hiding its true nature for a while, but it can no longer deny its attraction to modifying databases and it is out and proud to be a TestCase.
No longer will WebhooksHTMXTests terrorize test results with its carelessly created users and profiles.
Added an import alias to avoid having two APITestCase symbols.
OneDatabaseMixin is replicating what Django test cases other than SimpleTestCase have done for six years and unfortunately extending database access to tests without any the protections of TransactionTestCase or TestCase. This is the root cause of a lot of random test failures.
FilteringCountTestCase isn't a test case, it's a mixin for test cases that provides a new assert method. Now its name matches its function.
It works! I guess I missed CitationTextTest, but it failed when it tried to modify the database as a SimpleTestCase. This upgrades it to a TestCase so it can finish its mission.
AudioTestCase already inherits from TestCase, so NoteTest shouldn't directly inherit from it.
|
Interesting one, thanks for working on this unglorious but important stuff. It was my fault that OneDatabaseMixin existed, but I got it from Adam Johnson's book, so I don't feel too bad about it. I definitely didn't realize that by giving access to Anyhow, Alberto is better at tests than me, so off to him for final check. Thank you! |
Add some missing super() calls for setup and teardown test methods.
|
The Selenium test failure is legit. Something is swallowing the |
|
Okay, the problem is |
You should never cross the streams of TransactionTestCase and TestCase. All of OpinionSearchFunctionalTest's data objects were being put in a transaction that Selenium couldn't see. This is fallout from AudioTestCase previously just adding stuff to the live database. I switched it from TestCase to TransactionTestCase and verified that nothing else importing it was also using TestCase.
This isn't good. I've seen a bunch of these missing and I'm guessing there's still more.
|
Alright, found and fixed the problem. The inheritance tree for |
All that and OpinionSearchFunctionalTest doesn't use AudioTestCase. I looked at other classes using AudioTestCase and decided to switch it back to a TestCase.
|
Okay, all tests are finally passing. I think that a lot of the *TestCase subclasses should really be mixins, because all most of them do is provide some data objects and convenience methods. I'd rather have |
|
I totally agree about mixins. All the inheritance is my fault. It was all the rage at the time... |
florean
force-pushed
the
remove-onedatabasemixin
branch
from
37a24af to
2831b62
Compare
There was a problem hiding this comment.
Thank you @florean this looks good. I only left a few suggestions.
florean
changed the title
As I've discussed, the tests for CourtListener are more unreliable than I'm used to. I've poked around a little, but tonight the same test that @amattalols mentioned in #5940 failed on me yet again and I decided to really dive in. And down the rabbit hole I fell...
The root cause is OneDatabaseMixin. It overrode the databases available to test cases to restrict them to only the default database, but that was already the default behavior in Django for a while. Although it didn't affect any test cases that already had database access, it did give database access to those that didn't. Notably, the custom version of SimpleTestCase, which doesn't normally have access because it has none of the protections of TransactionTestCase or TestCase.
So I read through every test that used SimpleTestCase, APITestCase, ESIndexTestCase, CountESTasksTestCase, or V4SearchAPIAssertions and fixed the ones that were using the database. Besides tests using those, any test that used CourtTestCase, PeopleTestCase, SearchTestCase, RECAPSearchTestCase, AudioTestCase, or AudioESTestCase was potentially affected (but a large number of them also inherited from one of the safe Django test cases). Now, if a test tries to access the database from a SimpleTestCase or its children, it'll throw an exception and fail.
In total, this affected about 562 tests or 37% of the total. The effect on performance is negligible from what I can tell. I don't think I've eliminated all of the flaky tests, but this should be a majority of them, especially when combined with serializing ElasticSearch tests as in #5883.
Sorry about the commit messages, but it was late and the work was extremely dull and repetitive.