Skip to content

Refactor TestCases to mixins #5961

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

florean
Copy link
Contributor

@florean florean commented Jul 14, 2025

There are a lot of TestCase that basically just setup some useful Factory objects and don't really need to be TestCases. This can cause problems when you want to use a TransactionTestCase and one of these TestCases and makes the class hierarchy more complicated than it needs to be. Converting them into mixin classes simplifies things quite a bit.

florean added 19 commits July 12, 2025 00:58
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.
Add some missing super() calls for setup and teardown test methods.
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.
All that and OpinionSearchFunctionalTest doesn't use AudioTestCase.  I looked at
other classes using AudioTestCase and decided to switch it back to a TestCase.
@florean
Copy link
Contributor Author

florean commented Jul 14, 2025

@mlissner, as I mentioned in #5954, I'd like to convert some TestCases into mixins. Does this approach look good to you? I branched from my existing test PR, so check out commits 574d24c and c22da50.

(sorry about the force pushes, my alternate test patch fix kept getting in there)

@florean florean force-pushed the refactor-test-mixins branch from 4113c16 to 972fac7 Compare July 14, 2025 00:39
florean and others added 5 commits July 13, 2025 17:46
The reason the parent Django class, SerialMixin, doesn't define the `__lockfile__` is because everything that inherits from it is then using the same lock.  So your Selenium tests would be locked when you're doing ElasticSearch tests, etc.  Better to just use the Django one and deal with `__lockfile__` collisions as they occur.
@florean florean force-pushed the refactor-test-mixins branch from 972fac7 to a7a6851 Compare July 14, 2025 00:47
florean added 2 commits July 13, 2025 18:36
I got my ordering wrong.  Because Django TestCase doesn't call super
().setUpTestData(), it ends the chain.  Any mixins have to go ahead of it or
their methods won't be called.
florean added 10 commits July 13, 2025 19:24
RestartSentEmailQuotaMixin is only used by a couple tests in users, so I moved it next to where it is used.  If it gets used elsewhere, then we call pull it out into the common mixins.
I've added an ESIndexTransactionTestCase and made ESIndexTestCase a subclass of that and TestCase.  I then converted all the subclasses to the appropriate class.
Also converted CountESTasksTestCase to a mixin.
@florean florean force-pushed the refactor-test-mixins branch from 615321b to b3e3e7f Compare July 14, 2025 10:06
@florean florean force-pushed the refactor-test-mixins branch from cc6748f to 02214ca Compare July 14, 2025 10:11
@mlissner
Copy link
Member

I think the conversion to mixins feels about right. Two thoughts I had:

  1. It seems like a lot of these mixins relate directly to models. In that case, it feels like these mixins should be in the app, rather than centralized in a huge file. Maybe we should have something like cl.audio.test_utils.AudioDataMixin instead of cl.test.mixins.AudioMixin.

    I'm not sure that's right, but something along those lines feels right to me. Maybe they live in cl.audio.test_mixins instead, but that feels overly specific. Or perhaps there's cl.audio.tests, inside which has tests.py, mixins.py, and (in some cases) the various other test files.

  2. I'm no git pro, but ideally we'll be able to merge this separately from your PR fixing SimpleTestCase and whatnot. That'd be a lot easier to review, rather than putting it all in one PR (as this appears to be).

@florean
Copy link
Contributor Author

florean commented Jul 14, 2025

  1. It seems like a lot of these mixins relate directly to models.

Yeah, now that I've got most things converted to mixins, I'm going to start pulling out ones that are more model specific. If they're only used by one app's tests, I'm just moving them into the tests file like this. Although I should move that to the top of the file. The PrayAndPayMixin is next and there's a few others. I don't think it's worth creating a bunch of single class mixin files. RECAPSearchMixin might be an exception, though, because it is used by multiple files and search already has a test directory.

  1. I'm no git pro, but ideally we'll be able to merge this separately from your PR fixing SimpleTestCase and whatnot.

I forked this from #5954 for two reasons: I didn't want to duplicate work from it and this isn't worth looking at until that is merged, anyway: that one actually fixes broken tests, this one is just a refactor to simplify some things. I just put it up as a draft to be able to point to some specific commits to make sure this was going in the right direction. I don't want to ambush you with a massive refactor that you don't want. And I don't expect this to be the final change. If you decide it's better to spread them across apps, we can do that. But this is a large step on the path to enabling that.
Honestly, most of the commits are a mess, so I'll probably squash it before review anyway (my personal philosophy is any commit on main should be deployable).

@mlissner
Copy link
Member

This all sounds good. Hold off on going too deep here, I think, until we've caught up with you. That point we'll have a better sense of where things stand? I'm +1 on mixins, but I think the dev team wants a chance to catch up before having opinions.

@florean
Copy link
Contributor Author

florean commented Jul 15, 2025

I think it is mostly done and I've moved on to some other things. I'll probably squash this down to a single commit and force push it, just to make it easier to read while #5954 is still hanging out.

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.

2 participants