Django

Code

Ticket #8138 (new)

Opened 4 months ago

Last modified 1 month ago

Switch django tests to use transactions

Reported by: mremolt Assigned to: nobody
Milestone: post-1.0 Component: Testing framework
Version: SVN Keywords:
Cc: trevor, dan.fairs@gmail.com, remco@maykinmedia.nl, copelco Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

Currently the Django testsuite flushes the database after every test. That is a major performance drawback. The fixture loading/truncating takes much longer than the actual test. So I propose to switch the Django testtools to use transactions:

  • Before every test a transaction is started
  • The fixtures (if existing) are loaded
  • No commit is performed
  • The tests run
  • A rollback is performed

There is a matching discussion on django-developers:

http://groups.google.com/group/django-developers/browse_thread/thread/49aa551ad41fb919

My first tests with the concept show some promise. The first try at a patch breaks some tests (1 on SQLite, 25 on PostgreSQL), but the performance improvement is significant. With the django testsuite the results on my Laptop (slow disk - IO is the bottleneck with the tests here) so far are:

  • SQLite in-memory-db:
    • before: 415 sec
    • after: 79 sec
  • PostgreSQL:
    • before: 4285 sec
    • after: 359 sec

As suggested by Russell Keith-Magee, I renamed the original test.TestCase? to test.TransactionTestCase? and implemented the new functionality in test.TestCase?. If someone explicitly wants to use transactions inside a test, he can use TransactionTestCase? and will only loose the performance gain. With the current patch all doctests now use transactional behaviour. We'll see, if I can implement a choice there, too - if it is necessary.

Attachments

django_transaction_tests.diff (6.9 kB) - added by mremolt on 08/07/08 10:17:42.
Now doesn't touch _doctests.py
django_transaction_tests_2.diff (7.5 kB) - added by lukeplant on 08/07/08 17:13:25.
test_in_transactions.diff (14.2 kB) - added by lukeplant on 08/07/08 19:31:57.
patch of test framework and fixes for tests
django_transaction_tests_3.diff (14.8 kB) - added by mremolt on 08/12/08 02:17:57.
django_transaction_tests_3.2.diff (14.9 kB) - added by mremolt on 08/12/08 02:22:11.

Change History

08/06/08 13:46:38 changed by jacob

  • needs_better_patch changed.
  • needs_docs changed.
  • stage changed from Unreviewed to Accepted.
  • needs_tests changed.
  • milestone changed from post-1.0 to 1.0.

Although the naming is non-obvious (at least to me), this is a pretty incredible improvement. I'm marking this as targeted for 1.0: we should sort out the failing tests, fix the naming, and get this in. It'll really help make testing less painful, and that's fantastic.

(follow-up: ↓ 3 ) 08/06/08 14:33:38 changed by ElliottM

IMO it's kind of counterintuitive to call the tests that don't use transactions TransactionTestCase?.

(in reply to: ↑ 2 ) 08/06/08 18:44:30 changed by russellm

Replying to ElliottM:

IMO it's kind of counterintuitive to call the tests that don't use transactions TransactionTestCase?.

The reason behind the naming was actually the other way around - you use TransactionTestCase? if you need to test transactional behaviour. Normal 'just put it in the database' behaviour is covered by a standard test case.

However, I'm not married to either name - suggestions are welcome.

08/07/08 10:17:42 changed by mremolt

  • attachment django_transaction_tests.diff added.

Now doesn't touch _doctests.py

08/07/08 17:01:40 changed by lukeplant

I updated mremolt's patch:

  • Removed the public '--no-commit' option to the loaddata command (it is still available as a kwarg 'no_commit', just not documented publically)
  • Refactored _pre_setup() and _post_teardown() to avoid the duplication between TransactionTestCase? and its subclass, TestCase?

08/07/08 17:13:25 changed by lukeplant

  • attachment django_transaction_tests_2.diff added.

08/07/08 19:31:57 changed by lukeplant

  • attachment test_in_transactions.diff added.

patch of test framework and fixes for tests

08/11/08 09:56:54 changed by russellm

One minor addition - SecureViewTest? (added in [8271]) also needs to be TransactionTestCase?.

Regarding the "#Test uses transactions" modifier in the doctests: there is an interesting crossover here with #5624. #5624 calls for a way to associate fixtures with doctests. It occurs to me that if we're going to start adding directives to doctests (and there are some other candidates beyond these two), we should be consistent with how we specify options like this one.

At the very least, we need to pick a consistent (and extensible) syntax; better still would be to use an existing syntax, like doctest option syntax. Using doctest.register_optionflag() isn't an option because of the need to support Python 2.3.

08/11/08 10:16:37 changed by russellm

Clarifying for Luke's benefit - I don't know if you were planning to, but don't check this in yet :-)

08/11/08 18:43:46 changed by russellm

  • milestone changed from 1.0 to post-1.0.

Ok; Speaking with Malcolm and Jacob, we've decided to defer this to post-1.0. Feature freeze is in 2 days; we won't have time to resolve the minor design issue of doctest flags, Jacob doesn't like the TransactionTestCase? name, and there is the risk (however small) that we could introduce an unexpected error into the test system just days before we go into a heavy duty test phase.

The idea is definitely going into v1.1, though.

08/12/08 02:17:57 changed by mremolt

  • attachment django_transaction_tests_3.diff added.

08/12/08 02:22:11 changed by mremolt

  • attachment django_transaction_tests_3.2.diff added.

08/12/08 02:32:33 changed by mremolt

I've uploaded my current (rough) progress on the ticket (django_transaction_tests_3.2.diff).

At the momrent I only need one TransactionTestCase?, as the other problems are resolved by the testsuite automatically and that test really does some nasty things.

  • SQLite: 1 failure (something new to do with the filestore refactor)
  • Postgres: 1 failure (same as sqlite) and 2 errors (seems something inside session handling)
  • MySQL: has problems loading some fixtures, the dreaded "foreign key constraint fails"

Before I continue, could I have some feedback on my way of solving the problems. As you've put it to post 1.0 (good idea), there is no hurry. At the moment it looks like we could get this thing fully backwards compatible, so getting rid of TransactionTestCase? completely.

By the way, please ignore django_transaction_tests_3.diff. Wrong patch base directory.

08/12/08 07:28:17 changed by lukeplant

  • owner changed from lukeplant to nobody.

Russell - I wasn't planning on committing it, and I'm glad you've decided to defer it, it's too big a change to make at this point until we are really clear on the implications and have thrashed them out in some different code bases for a while.

Also, my method of flagging the transactional tests in the doctest string was a hack really, I meant to mark it as such and ask about better ways of doing it.

I'm un-assigning from myself as I'm not intending to do anything on this in the near future.

08/13/08 09:36:41 changed by russellm

(In [8336]) Refs #8138 -- Added a stealth option to the loaddata command so that the use of transactions in loaddata can be disabled. This behavior isn't enabled as a commad line option because it doesn't make much sense, but it can be used when invoking loaddata from another script.

08/15/08 17:54:24 changed by trevor

  • cc set to trevor.

09/02/08 15:13:48 changed by danfairs

  • cc changed from trevor to trevor, dan.fairs@gmail.com.

09/10/08 00:22:45 changed by adrian

  • component changed from Tools to Testing framework.

10/06/08 04:36:31 changed by shanx

  • cc changed from trevor, dan.fairs@gmail.com to trevor, dan.fairs@gmail.com, remco@maykinmedia.nl.

10/10/08 08:56:23 changed by anonymous

  • cc changed from trevor, dan.fairs@gmail.com, remco@maykinmedia.nl to trevor, dan.fairs@gmail.com, remco@maykinmedia.nl, copelco.

10/22/08 02:54:14 changed by Almad

From looking at patch: I think it can also be clever and flush database between cases only if they differ in used fixtures - if they are same, there is no reason to.

Not sure how to implement it using standard django test framework (fairly easy with nose or functools, I guess).


Add/Change #8138 (Switch django tests to use transactions)




Change Properties
Action