* A transaction is opened during `setUpClass` (one atomic per database)
* That transaction is closed during `tearDownClass`
* The database connection is no longer closed after each test function
This allows to create objects and complex state in `setUpClass`, and reuse
it through all test functions,
without inter-test conflicts.
This could lead to a single load of fixtures during `setUpClass` (instead
of the current `setUp`), thus improving test speed.
--
Ticket URL: <https://code.djangoproject.com/ticket/20392>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Yes, this is a good idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:1>
Comment (by aaugustin):
I'm not sure about reset_connections_per_test. Closing the connections
during the execution of the tests breaks with in-memory SQLite. What is
this parameter designed to achieve? Is it intended as a public API?
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:2>
Comment (by xelnor):
The `reset_connections_per_test` attribute is here to avoid duplicating
the logic in `TransactionTestCase._post_teardown`.
The current behavior (see
https://github.com/django/django/blob/master/django/test/testcases.py#L516)
is to close all database connections after `self.tearDown`.
I wanted to be able to distinguish between `TransactionTestCase`, where
this connection closure is required for database reset, and `TestCase`,
where it should be avoided to gain the features discussed above.
Put another way, the current behaviour is:
- `TransactionTestCase` recreates the database and loads fixtures before
each `test_foo`; closes the connection after each `test_foo`.
- `TestCase` creates the database once, loads `TestCase`-specific fixtures
before each `test_foo`; rolls back the transaction and closes the
connection after each `test_foo`.
The goal is:
- No change to `TransactionTestCase`
- `TestCase` creates the database once, begins a transaction at
`setUpClass` where it loads `TestCase`-specific fixtures, makes a
savepoint before each `test_foo`, rolls back to savepoint after each
`test_foo`, rolls back the transaction then closes connections in
`tearDownClass`.
I'll try to upgrade the patch with more comments, and move the "load
fixtures" code to `setUpClass`.
This discussion should probably move to django-developers@, what do you
think?
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:3>
* cc: charettes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:4>
* cc: loic84 (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:5>
Comment (by aaugustin):
Related discussion on django-developers: https://groups.google.com/d/msg
/django-developers/_nQufnu7uFI/Ue5ILCas-ZMJ
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:6>
Comment (by aaugustin):
Xelnor, when you say "create the database", you mean "create the database
connection", don't you?
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:7>
Comment (by aaugustin):
Judging by this discussion on django-developers:
https://groups.google.com/d/msg/django-developers/N0HEAD1ht8k/GQJ77RLUydsJ
and test cases included with the patch, it seems that the patch changes
(for the better) the behavior of setUpClass: data changes made in
setUpClass now get reset between test cases. This should be mentioned in
the release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:8>
* cc: mmanfre@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:9>
* owner: nobody => smeatonj
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:10>
Comment (by xelnor):
Replying to [comment:7 aaugustin]:
> Xelnor, when you say "create the database", you mean "create the
database connection", don't you?
Hmm, yes, I think that's what I thought. Anyway, that's what the patch
does ;)
Thanks for reviving this ticket!
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:11>
Comment (by smeatonj):
xelnor, I'm looking into bringing this ticket up-to-date and testing the
run time of the test suite with loaddata in the setUpClass. Is there any
other information that you could offer up, or if you're already
maintaining an up-to-date patch, providing the details of that?
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:12>
* cc: mathieu.agopian@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:13>
Comment (by xelnor):
Replying to [comment:12 smeatonj]:
> xelnor, I'm looking into bringing this ticket up-to-date and testing the
run time of the test suite with loaddata in the setUpClass. Is there any
other information that you could offer up, or if you're already
maintaining an up-to-date patch, providing the details of that?
I haven't maintained this patch — I could try to retrieve the original
branch if you need; I think the patch was designed against 1.5 (to get
atomic).
The biggest issue I had was that this patch '''requires''' `setUpClass` to
be called: if a `TestCase` subclass defines a custom `setUpClass` but
doesn't call `super()`, we'd have some issues — mostly that this
`setUpClass` would occur outside a transaction.
This could be alleviated with custom hooks, similar to the `_pre_setup`
and `_post_teardown` used in the Django TestRunner.
Unfortunately, `unittest` doesn't provide hooks around `setUpClass` /
`tearDownClass` in the test runner: they are all called on the `TestSuite`
object, and Django reuses the default one.
For me, we have a few options here:
* Document that, obviously, `super()` should be called when overriding
`setUpClass`
* Add safeguards in `setUpClass` and check them, for instance, in
`_pre_setup` : this could maybe raise if `super()` wasn't called?
* Look harder and find a way to replicate the `_pre_setup` part for
`setUpClass`, perhaps with a custom `TestSuite` wrapper?
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:14>
Comment (by smeatonj):
Thanks for the reply. I've already brought the patch up to date, so no
need to chase down the branch. I also implemented the loaddata (fixtures)
from inside the setUpClass, which has halved the run time for tests that
use fixtures. I've run into a few problems with tests that manipulate the
connections and transactions, but I haven't tracked down the exact causes
yet.
The issues you raise are good ones. I think documenting the contract
should suffice. If someone wanted to override setUpClass, they should have
the option of not inheriting the behaviour if they wish - but they'd be
responsible for all transactions and tear downs also.
The other way I thought about going was implementing a
SharedFixtureTestCase, and moving the behaviour of this patch into that
class. It would mean that the existing TestCase would be left as-is, and
opt-in is required for the double transaction behaviour. I think this
would be the safest route, but wouldn't reach as many people. Luckily, we
can update the internal django tests that use fixtures to use this new
class, and get quite a substantial speedup for those tests.
I profiled the entire test suite with this patch and loaddata is
responsible for about 10% of the total time. That time might be skewed by
the test failures I mentioned above, but it should be fairly indicative.
I'll keep working on it until there are no more test failures, and then
make a decision (hopefully with input from others) on whether to extract a
new TestCase subclass.
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:15>
Comment (by aaugustin):
Don't make a new subclass. Either document the pitfall, or test in
_pre_setup that the appropriate setUpClass has run and blow up loudly
otherwise.
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:16>
Comment (by xelnor):
I'd side with aaugustin on that one: the contract of `TestCase` is
"whatever happens in the database during the `test_` is temporary; if you
use fixtures, they will be cleaned afterwards; be careful when creating
instances in `setUpClass`."
I don't think there is any documentation stating that "if you want to
reuse data created in `setUpClass` in other Cases, it will work", nor that
this would actually be a good idea.
Currently, users who create objects in `setUpClass` carefully remove them
during `tearDownClass`, and so the only tests that would be broken by this
change would be tests where "one `TestCase` relies on the `setUpClass` of
another one to have run before", which sounds pretty broken to me.
If you feel that users should be allowed to mess with transactions, this
could be done with a simple class attribute, similar to `fixtures = `.
Besides, any improvement to Django's test suite is likely to benefit big
projects using Django as well; please don't make them "hidden" for the
sake of backwards-compatibility on undocumented behavior ;)
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:17>
Comment (by smeatonj):
This is the reason that I thought about implementing a new class:
{{{
# django/tests/file_uploads/tests.py
class FileUploadTests(TestCase):
@classmethod
def setUpClass(cls):
if not os.path.isdir(MEDIA_ROOT):
os.makedirs(MEDIA_ROOT)
}}}
Already we run into issues where the setUpClass doesn't call the parent -
so any fixtures that would have been loaded no longer are. I'm unsure
whether the behaviour here should be "blow up and make the users fix a
backward-incompatible change", or to fall back to the previous behaviour,
and load the fixtures in setUp rather than setUpClass. We could provide
very clear guidance in the release notes (always call
super().setUpClass()), but that no longer allows callers to completely
customise their setUpClass (opt-out of class-based transactions). Which
behaviour is preferred?
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:18>
Comment (by aaugustin):
Yes, you must blow up and make users add a super() call in that case.
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:19>
Comment (by smeatonj):
I had a go at implementing the ideas in this ticket:
https://github.com/jarshwah/django/tree/testcase-optimization
Test cases that use fixtures improved by a factor of about 3.
Unfortunately, this only represents about 10% of the entire test suite, so
doesn't make a huge impact. I also ran into some weird threading issues
with sqlite that was causing a segmentation fault.
I'm going to bow out at this point, but I'm more than happy to help if
someone else would like to continue on the patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:20>
* status: assigned => new
* cc: josh.smeaton@… (added)
* owner: smeatonj =>
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:21>
Comment (by tchaumeny):
I spent some time working on that and eventually came up with something.
Besides the fact that setUpClass/tearDownClass should call super, I
encountered the following problems :
* When `@override_settings` is decorating a `TestCase` it should also
affect `setUpClass` / `tearDownClass`
=> There already is a ticket for that (#21281), but I believe it is
required if we are going to tell people they can interact with the
database within `setUpClass` as the result of that interaction might
depend on whether or not the settings were overridden when it happened
(`USE_TZ` for instance).
* Connections should no longer be closed after each test within a
`TestCase`. This was already part of the initial proposal and might be
considered as a backward incompatible change as some tests purposely
generate database failures or even close the connection explicitly. Those
tests should use `TransactionTestCase` instead — in Django test suite,
some threading tests in `django/tests/backends` are concerned.
* If the database does not support transaction, `TestCase` will keep
behaving as `TransactionTestCase`. This means that initializing data
within `setUpClass` will result in random behavior. This is the current
behavior hence should not be considered as a backward incompatible change,
but it means that tests refactored to take advantage of `setUpClass`
(which might be very interesting, see below) should not run under MySQL
with MyISAM (no transaction support).
I did those modifications in my branch
https://github.com/tchaumeny/django/compare/use_setupclass, and the django
test suite is passing with both PostgreSQL and SQLite — I still need to
test with MySQL (InnoDB mode) but run into some unrelated issues.
In order to demonstrate the advantage of using `setUpClass`, I modified
`select_related` and `queries` tests so that data are generated once in
`setUpClass`:
Current implementation:
{{{
$ ./tests/runtests.py select_related queries --settings=test_postgres
...
Ran 253 tests in 10.637s
OK (skipped=1, expected failures=2)
}}}
Using `setUpClass`:
{{{
$ ./tests/runtests.py select_related queries --settings=test_postgres
...
Ran 253 tests in 2.291s
OK (skipped=1, expected failures=2)
}}}
So, do you think it would be worthwhile to pursue in that direction ?
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:22>
* cc: t.chaumeny@… (added)
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:23>
* stage: Accepted => Ready for checkin
Comment:
At least a second set of eyes would be nice!
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:24>
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
I reviewed the patch and had several comments or questions. Sorry for the
delay.
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:25>
* needs_better_patch: 1 => 0
Comment:
Thanks for the review. I updated the PR to address your comments.
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:26>
* owner: => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"da9fe5c717c179a9e881a40efd3bfe36a21bf4a6"]:
{{{
#!CommitTicketReference repository=""
revision="da9fe5c717c179a9e881a40efd3bfe36a21bf4a6"
Fixed #20392 -- Added TestCase.setUpTestData()
Each TestCase is also now wrapped in a class-wide transaction.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:27>
Comment (by Tim Graham <timograham@…>):
In [changeset:"26dd518b5c0c6f4a7678bfb7bd3ed0bf8dc8978e"]:
{{{
#!CommitTicketReference repository=""
revision="26dd518b5c0c6f4a7678bfb7bd3ed0bf8dc8978e"
Refactored some tests to take advantage of refs #20392.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:28>
Comment (by Tim Graham <timograham@…>):
In [changeset:"119154ca7f65d423bb742aa0e32ffcfcb21d4b87"]:
{{{
#!CommitTicketReference repository=""
revision="119154ca7f65d423bb742aa0e32ffcfcb21d4b87"
Refs #20392 -- Load fixtures once within TestCase
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20392#comment:29>