Fixed #20483 -- Made TransactionTestCase faster. #1240

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
Owner

aaugustin commented Jun 4, 2013

No description provided.

Member

alex commented Jun 4, 2013

How much faster?

Member

alex commented Jun 4, 2013

I have to say this whole thing scares me quite a bit. Why isn't @overide_settings(INSTALLED_APPS=['app1', 'app2']) sufficient? So much global state :(

Owner

aaugustin commented Jun 4, 2013

The entire test suite passes in about 150 seconds with this patch. See the ticket for more information.

https://code.djangoproject.com/ticket/20483

Member

charettes commented Jun 4, 2013

@alex the @overide_settings(INSTALLED_APPS=['app1', 'app2']) approach should be used once the app cache refactor is merged.

Member

akaariai commented Jun 4, 2013

I don't believe using @override_settings would actually change anything. You still have to change global state (the used app-cache, or what the cache contains), so there is zero gain in that respect. And as mentioned in the ticket, you still have the problem that imports happen only once.

Using override_settings so that we don't have two things that do almost the same thing could be a good idea.

Member

akaariai commented Jun 4, 2013

I think you can get rid of the nastiest parts of this changeset by requiring that all "cascades" are included in installed_apps. That is, all models which will need to be truncated or cascaded into by delete are included, and thus you will not need to do any changes there.

It is true that this is trading code complexity for testing speed. On my machine PostgreSQL tests are 4500s vs 300s between master and the original isolated_apps branch. IMO important enough to have some complexity.

+ try:
+ get_model('auth', 'Permission')
+ except UnavailableApp:
+ return
@mjtamlyn

mjtamlyn Jun 4, 2013

Member

Presumably we don't actually want this to fail silently? If for some reason some end user code was managing to make this throw UnavailableApp I don't think it should hide it.

@akaariai

akaariai Jun 4, 2013

Member

The reason this is done is that flush sends post_syncdb, and permission & contenttype creation are listening for the signal.

This seems to be one point more for override_settings() based approach. If done that way, permissions & contenttypes could register a listener for settings_changed and add/remove the signal listener as needed. No need to know about "app mask" at all.

Member

mjtamlyn commented Jun 4, 2013

Is it possible we may be able to speed things up even more by setting available_apps = [] on tests which don't actually hit the DB therefore don't need to reset anything? This might be dangerous...

The speed improvements are very impressive though!

django/db/models/loading.py
@@ -15,6 +15,8 @@
__all__ = ('get_apps', 'get_app', 'get_models', 'get_model', 'register_models',
'load_app', 'app_cache_ready')
+class UnavailableApp(ValueError):
@ptone

ptone Jun 5, 2013

Member

minor - but any reason to subclass ValueError instead of Exception?

django/db/models/loading.py
@@ -287,3 +321,4 @@ def register_models(self, app_label, *models):
register_models = cache.register_models
load_app = cache.load_app
app_cache_ready = cache.app_cache_ready
+set_app_mask = cache.set_app_mask
@ptone

ptone Jun 5, 2013

Member

these module level functions are only here for legacy reasons - as this is a new feature, it doesn't really need to be included in this list like this

django/db/models/manager.py
@@ -124,6 +125,9 @@ def get_queryset(self):
"""Returns a new QuerySet object. Subclasses can override this method
to easily customize the behavior of the Manager.
"""
+ # Check that the model is available in the app cache. This is required
+ # when TransactionTestCase.available_apps is set.
+ get_model(self.model._meta.app_label, self.model._meta.model_name)
@ptone

ptone Jun 5, 2013

Member

can this be checked more directly by looking at cache.available_apps for the app_label, and raise UnavailableApp directly instead of just using get_model as a throw away here? or adding a a cache.model_available('applabel.modelname') type of method? But either way I'm assuming you are wanting to throw the exception here?

@@ -7,6 +7,10 @@
import sys
import tempfile
import time
+try:
@ptone

ptone Jun 5, 2013

Member

are changes to this test related to this issue?

- apps = [(v, k) for k, v in self.app_store.items()]
- apps.sort()
- return [elt[1] for elt in apps]
+ apps = sorted(apps, key=lambda elt: elt[1])
@akaariai

akaariai Jun 9, 2013

Member

This is likely irrelevant but the sort order isn't exactly the same as before - lambda elt: elt[1], elt[0] is exactly same...

(But now that I look deeper I don't get the whole code. app_store is SortedDict, elt[1] seems to be insertion order. So, .keys() should be in the wanted order directly. But maybe better to just ignore this comment and wait for app-loading refactor to deal with this...)

@aaugustin

aaugustin Jun 9, 2013

Owner

There are no duplicates in elt[1]. This should be all right.

aaugustin added some commits Jun 4, 2013

Added a stealth option to flush to allow cascades.
This allows using flush on a subset of the tables without having to
manually cascade to all tables with foreign keys to the tables being
truncated, when they're known to be empty.

On databases where truncate is implemented with DELETE FROM, this
doesn't make a difference. The cascade is allowed, not mandatory.
Added TransactionTestCase.available_apps.
This makes Django's test suite significantly faster by reducing the
number of models for which content types and permissions must be created
and tables must be flushed in each non-transactional test.

Most of the credit goes to Anssi. He got the idea and did the research.

It's documented for Django contributors and committers but it's branded
as a private API to preserve our freedom to change it in the future.

Refs #20483.
Owner

aaugustin commented Jun 10, 2013

Merged

@aaugustin aaugustin closed this Jun 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment