Test reordering and TransactionTestCase cleanup

[Andreas Pelme]

I am trying to run my Django test suite with an alternative test runner (py.test), and found some issues with test isolation.

TransactionTestCase does currently not clean up after itself (i.e. flush the database), but instead assumes that the next test will flush the database. It is generally a good idea to restore the state after the test run, and let other tests start out with a known state.

Djangos default testrunner reorders the test suite to run all TestCase tests before TransactionTestCases, which avoids this problem. I cannot find this reordering documented anywhere, or even commented anywhere in the implementation (test/simple.py: reorder_suite and DjangoTestSuiteRunner.build_suite).

I propose to move the flush command out of _fixture_setup to _post_teardown in TransactionTestCase. This makes it possible for arbitrary execution order of the test suite. Performance could potentially be an issue, since it is "lazy" today and flushes the database just right before it is needed. That would however only affect the last test in the run (i.e. it will truncate the tables, and then the database itself will be teared down).

Are there any objections or anything that might break with such a change? Are there any specific reasons it works the way it does?

I will be happy to create a ticket and patch for this, I would get to input in case I miss something obvious.

Cheers
Andreas

[Anssi Kääriäinen]

I don't know if anything will break...

I think this is a good idea. In addition to the above issues this
would allow the test ran tell the flushing which tables are dirty,
which not. Currently the transaction_regress for example uses just a
single table. But after each test, all tables will be truncated, and
the contenttypes and permissions will be reloaded. I bet we could
speed up the testing _a lot_ if the cleaning up could be made more
intelligent. Making it robust is somewhat hard but should not be
impossible. Of course, the "StateTrackingTestCase" should not be done
in the same patch at all... just a possibility for future work.

- Anssi

[Karen Tracey]

On Thu, May 3, 2012 at 12:29 PM, Andreas Pelme <and...@pelme.se> wrote:
>
> Djangos default testrunner reorders the test suite to run all TestCase tests before TransactionTestCases, which avoids this problem. I cannot find this reordering documented anywhere,

It is documented:
https://docs.djangoproject.com/en/1.4/topics/testing/#django.test.TransactionTestCase

Mentioned in both the 3rd paragraph and the note.

Karen

[Ramiro Morales]

On Thu, May 3, 2012 at 1:29 PM, Andreas Pelme <and...@pelme.se> wrote:
> I am trying to run my Django test suite with an alternative test runner (py.test), and found some issues with test isolation.
>
> TransactionTestCase does currently not clean up after itself (i.e. flush the database), but instead assumes that the next test will flush the database.

> [...]

>
> Djangos default testrunner reorders the test suite to run all TestCase tests before TransactionTestCases, which avoids this problem.

Just a quick related note

There is a [1]proposal to extend this reordering to be:

* TestCases
* doctests
* TransactionTestCases

So doctests aren't affected either.

I intend to commit a fix for it soon.

--
Ramiro Morales

1. https://code.djangoproject.com/ticket/12408

[Andreas Pelme]

On Thursday 3 May 2012 at 21:17, Karen Tracey wrote:

> On Thu, May 3, 2012 at 12:29 PM, Andreas Pelme <and...@pelme.se (mailto:and...@pelme.se)> wrote:
> >
> > Djangos default testrunner reorders the test suite to run all TestCase tests before TransactionTestCases, which avoids this problem. I cannot find this reordering documented anywhere,
>
> It is documented:
> https://docs.djangoproject.com/en/1.4/topics/testing/#django.test.TransactionTestCase
>
> Mentioned in both the 3rd paragraph and the note.

Thanks -- I'm not sure how I missed that. I'll make sure my patch updates the docs too.

Andreas

[Andreas Pelme]

On Thursday 3 May 2012 at 22:14, Ramiro Morales wrote:

This won't be an issue if TransactionTestCase resets the database after itself.

Andreas

[Andreas Pelme]

On Thursday 3 May 2012 at 19:23, Anssi Kääriäinen wrote:

There are indeed some work that can be done to speed up the test running. :)

Here's the ticket:

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

… and an initial patch:

https://github.com/django/django/pull/45

Is there any sane way to test this except to run the test suite itself?

Andreas

[Anssi Kääriäinen]

On May 4, 10:09 am, Andreas Pelme <andr...@pelme.se> wrote:
> Here's the ticket:
>
> https://code.djangoproject.com/ticket/18271
>
> … and an initial patch:
>
> https://github.com/django/django/pull/45

I marked the ticket DDN, there are three reasons:
1. If a test case screws up cleanup it will cause problems for
itself currently, after the patch it will cause problems for the next
test case which makes debugging much harder.
2. Is this backwards incompatible? The behavior was documented.
3. Is there some reason why the flush must be done pre-test which
we are overlooking. Why was it implemented that way originally?
Perhaps because of the first item?

Each test case taking care of cleaning up after itself feels like the
right way to go. There are a couple of reasons:
- TestCase works already this way: it doesn't do cleanup before, it
just makes sure it cleans after itself (which is easy as it doesn't
leave any trash behind).
- The database is treated specially. The tests trust other tests to
clean up their state. Settings is an example.
- As said upthread, this allows for special test case subclasses
which do more intelligent cleanup. I don't think an intelligent
TransactionTestCase subclass which doesn't run flush pre-tests is
possible in the current implementation, as it would then see the trash
of other TransactionTestCases.

But, as said, I left the ticket DDN. None of the issues mentioned
above seem like a blocker. The backwards incompatibility is the most
likely blocker - I can't see that changing the behavior is going to
cause problems, but maybe there are users out there whose test cases
somehow require pre-test flush?

- Anssi

[Andreas Pelme]

Anssi, thanks a lot for the detailed feedback, it is much appreciated! My comments are inlined below:

On Friday 4 May 2012 at 10:03, Anssi Kääriäinen wrote:

> I marked the ticket DDN, there are three reasons:
> 1. If a test case screws up cleanup it will cause problems for
> itself currently, after the patch it will cause problems for the next
> test case which makes debugging much harder.

The same cleanup will still be done, it will just be moved from before the test run, to after.

The only way for a test case to screw up would be a bug in Django's _pre_setup/_post_teardown implementation, which would cause tricky bugs, no matter if the cleanup happens before or after the test.

> 2. Is this backwards incompatible? The behavior was documented.

See notes below.

> 3. Is there some reason why the flush must be done pre-test which
> we are overlooking. Why was it implemented that way originally?

That's a good question. Anyone who wrote to original TransactionTestCase/reordering implementation that wants to chime in? :-)

> Perhaps because of the first item?

The first item should get a newly created database, so it should not be a problem.

> Each test case taking care of cleaning up after itself feels like the
> right way to go. There are a couple of reasons:
> - TestCase works already this way: it doesn't do cleanup before, it
> just makes sure it cleans after itself (which is easy as it doesn't
> leave any trash behind).
> - The database is treated specially. The tests trust other tests to
> clean up their state. Settings is an example.
> - As said upthread, this allows for special test case subclasses
> which do more intelligent cleanup. I don't think an intelligent
> TransactionTestCase subclass which doesn't run flush pre-tests is
> possible in the current implementation, as it would then see the trash
> of other TransactionTestCases.

Agreed.

> But, as said, I left the ticket DDN. None of the issues mentioned
> above seem like a blocker. The backwards incompatibility is the most
> likely blocker - I can't see that changing the behavior is going to
> cause problems, but maybe there are users out there whose test cases
> somehow require pre-test flush?

I have run Djangos own test suite, and my current project's test suite with ~500 tests and some TransactionTestCases without problems.

This is indeed a change in documented behavior, although I don't think it will affect peoples test. In order for a test case to be backward incompatible, a user would need to override _pre_setup/_fixture_setup/_post_teardown/_fixture_teardown (which could be considered private?). Unless that is done, I cannot see how tests can be affected by this, but there can certainly be something I am missing here!

I guess we need someone who wrote the initial TransactionTestCase or knows why it is the way it is to give his/hers opinion on this not to oversee backward incompatible changes.


Andreas

[Karen Tracey]

On Fri, May 4, 2012 at 4:46 AM, Andreas Pelme <and...@pelme.se> wrote:
>
> That's a good question. Anyone who wrote to original TransactionTestCase/reordering implementation that wants to chime in? :-)

I worked on the test speedups that introduced TransactionTestCase and
I added the re-ordering, but the behavior of database flush being done
at the beginning of a test existed before that work. The conversation
that led to addition of the reordering is here:

http://groups.google.com/group/django-developers/browse_thread/thread/1e4f4c840b180895/

doctests (which we have no more?) play a prominent role in that
discussion. While we have gotten rid of doctests in Django's own
suite, we still support apps which may use doctests in their code, so
anything we do to change when the DB is cleared needs to take that
into account. On the table then was the idea of adding cleanup after
doctests, possibly that would need to be re-considered if you want to
move the database clearing to the end of everything rather than the
beginning of TransactionTestCase.

Karen

[Anssi Kääriäinen]

On May 4, 2:30 pm, Karen Tracey <kmtra...@gmail.com> wrote:

> On Fri, May 4, 2012 at 4:46 AM, Andreas Pelme <andr...@pelme.se> wrote:
>
> > That's a good question. Anyone who wrote to original TransactionTestCase/reordering implementation that wants to chime in? :-)
>
> I worked on the test speedups that introduced TransactionTestCase and
> I added the re-ordering, but the behavior of database flush being done
> at the beginning of a test existed before that work. The conversation
> that led to addition of the reordering is here:
>

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

>
> doctests (which we have no more?) play a prominent role in that
> discussion. While we have gotten rid of doctests in Django's own
> suite, we still support apps which may use doctests in their code, so
> anything we do to change when the DB is cleared needs to take that
> into account. On the table then was the idea of adding cleanup after
> doctests, possibly that would need to be re-considered if you want to
> move the database clearing to the end of everything rather than the
> beginning of TransactionTestCase.

Thanks for the link. While reading the previous threads I spotted at
one blocker issue: the first TransactionTestCase will not start with
zeroed database sequence values. To prevent this one would need to
flush the DB before the first TransactionTestCase. In addition, if the
goal is to get rid of test ordering, then one would need to flush
before every test case which isn't good. Getting rid of the
requirement to reset sequences between test cases would be good.
Oracle doesn't reset the sequences reliably currently just for
example. But I don't think we can change that for backwards
compatibility reasons. While tests that rely on sequence values are
IMHO broken, changing this would result in numerous broken tests for
users upgrading Django. Notably normal TestCases do not do sequence
resetting at all currently.

This is a blocker: why change the flushing from pre-test to post-test
if the end result is that you can't run the tests in arbitrary order
anyways? Or if you want to be able to run them in arbitrary order you
will need to run at least the sequence resets pre-test anyways.
Resetting sequences can be really expensive, so there goes any
potential savings in speed.

Ideas?

- Anssi

[Andreas Pelme]

On Friday 4 May 2012 at 13:30, Karen Tracey wrote:

#12408 is related here, which is caused by TransactionTestCase's not cleaning up after themselves. Rather than changing the test order to TestCase -> doctests -> TransactionTestCase I think the solution below is better.

Karens proposed a fix for this (#2) in the above thread [1], it basically says:

1) Make TransactionTestCase clean up after itself
2) Order the test suite to make sure doctests are always run *last*

That would run all unittest-style test cases first (the order of TestCase / TransactionTestCase will not be important), and last, all doctests.

A test suite that have doctests that *expects* state to be left from a TransactionTestCase before it would be broken by this change. In my opinion, a test suite with such expectations is already very broken and needs to be fixed any way, and should not be considered a blocker to this change (in that case, #12408 is a blocker too, since that effect will be the same).

I have updated my pull request to make unittest.TestCase subclasses to run first (i.e. doctests will run after all unit tests):
https://github.com/django/django/pull/45

Am I missing something? This seems like the most reliable way to solve this to me.

(I originally discovered this when I tried to run my tests with an alternative test runner, which does not order the tests in any particular way. I will still not be able to run doctests that does not clean up after themselves in arbitrary order, but as long as I document that's not supported with my test runner, I don't consider that to be a problem.)

Andreas

[1] https://groups.google.com/d/msg/django-developers/Hk9MhAsYCJU/fB4rj7F5SXEJ

[Andreas Pelme]

On Saturday 5 May 2012 at 00:39, Anssi Kääriäinen wrote:

> On May 4, 2:30 pm, Karen Tracey <kmtra...@gmail.com (http://gmail.com)> wrote:
> Thanks for the link. While reading the previous threads I spotted at
> one blocker issue: the first TransactionTestCase will not start with
> zeroed database sequence values. To prevent this one would need to
> flush the DB before the first TransactionTestCase. In addition, if the
> goal is to get rid of test ordering, then one would need to flush
> before every test case which isn't good. Getting rid of the
> requirement to reset sequences between test cases would be good.
> Oracle doesn't reset the sequences reliably currently just for
> example. But I don't think we can change that for backwards
> compatibility reasons. While tests that rely on sequence values are
> IMHO broken, changing this would result in numerous broken tests for
> users upgrading Django. Notably normal TestCases do not do sequence
> resetting at all currently.
>
> This is a blocker: why change the flushing from pre-test to post-test
> if the end result is that you can't run the tests in arbitrary order
> anyways? Or if you want to be able to run them in arbitrary order you
> will need to run at least the sequence resets pre-test anyways.
> Resetting sequences can be really expensive, so there goes any
> potential savings in speed.
>
> Ideas?

Agreed, it is indeed backward incompatible. However, I think we can make the upgrade for those affected very anyways:

* Document this change properly in the release notes and in the testing docs
* Document that tests should not depend on hard coded primary key values (this is already hinted in the testing docs where TestCase/TransactionTestCase is described)
* Add a reset_sequences attribute on TransactionTestCase, that can be applied to existing TransactionTestCase tests that already uses hard coded primary key value - which will reset sequences *before* the test run, just like before. (And those tests will also flush the database after they are run)

I have updated the pull request with the reset_sequences attribute and an initial draft for the release notes and documentation:
https://github.com/django/django/pull/45

1.1 introduced a far greater change to testing behavior (the TestCase with transaction/rollback), if that change was considered acceptable from a backward compatibility perspective, this change should be too. When that happened, the same problem appeared since TestCase does not reset sequences. (Yeah, just because it was done in the past does not make it OK now, but that should give some perspective).

I guess it boils down to where to draw the line for backward compatibility here. :-) I personally think it is acceptable since we can offer a very easy fix for the existing (already broken) tests out there.


Andreas

[Anssi Kääriäinen]

I would like to just get rid of the sequence resets. Oracle doesn't do
it currently, TestCase doesn't do it, and IMO assuming the IDs are
going to start from 1 is an assumption one should not make.

Objections to just getting rid of the sequence resets altogether (with
the opt-in flag)?

- Anssi

[Anssi Kääriäinen]

Just another idea: Maybe the right approach is to add flags to
TransactionTestCase (reset_sequences, track_state, track_fixtures,
more_ponies,...) and then recommend use of own subclasses in testing.
Something like:

class MyTestCase(TransactionTestCase):
reset_sequences = False
track_state = True # (flush only dirty tables).
track_fixtures = True # (Track if fixture data needs reloading
between tests).

class LoginLogouTests(MyTestCase):
fixtures = ['auth-user.json']
def test_login(self):
...

The defaults should be conservative (safe but slow). If there are
mysterious test case failures it will be easy to just comment out all
the flags to check if the test failures are explained by dirty state.
Similarly you could have "MySeleniumTestCase" subclass for selenium
tests.

This also allows deprecation of reset_sequences (or changing the
default from True to False) if we want to do so in the future.

It seems hard to provide subclasses in Django core, as that will lead
to combinatorial explosion down the road. Maybe mixins is the right
approach. I haven't used mixins based approaches, so I don't know how
easy it is to do the above using mixins.

- Anssi

[Karen Tracey]

On Mon, May 7, 2012 at 5:50 PM, Anssi Kääriäinen <anssi.ka...@thl.fi> wrote:

I would like to just get rid of the sequence resets. Oracle doesn't do
it currently, TestCase doesn't do it, and IMO assuming the IDs are
going to start from 1 is an assumption one should not make.

Objections to just getting rid of the sequence resets altogether (with
the opt-in flag)?

I did not realize Django code was doing something explicit to reset sequences during the database flush between TransactionTestCases, I thought it just happened as a side-effect of the method used to clear the DB. I don't believe tests should be relying on pk values starting with 1, if that's the only reason the sequence resets are there I think we should get rid of them.

Karen

[Łukasz Rekucki]

On 8 May 2012 14:22, Karen Tracey <kmtr...@gmail.com> wrote:
> On Mon, May 7, 2012 at 5:50 PM, Anssi Kääriäinen <anssi.ka...@thl.fi>
> wrote:
>>
>> I would like to just get rid of the sequence resets. Oracle doesn't do
>> it currently, TestCase doesn't do it, and IMO assuming the IDs are
>> going to start from 1 is an assumption one should not make.
>>
>> Objections to just getting rid of the sequence resets altogether (with
>> the opt-in flag)?
>
>
> I did not realize Django code was doing something explicit to reset
> sequences during the database flush between TransactionTestCases, I thought
> it just happened as a side-effect of the method used to clear the DB.

Taking a quick look at the code, it is a side-effect in the sense that
"manage.py flush" on PostgreSQL does that and TransactionTestCase uses
exactly that command.

Also, one might want to fix #10164 [1] first or the "reset_sequence"
flag (which is missleading name for something that does an extra
flush) will be a noop on SQLite.


[1]: https://code.djangoproject.com/ticket/10164

--
Łukasz Rekucki

[Ramiro Morales]

On Mon, May 7, 2012 at 6:50 PM, Anssi Kääriäinen
<anssi.ka...@thl.fi> wrote:
>
> I would like to just get rid of the sequence resets. Oracle doesn't do
> it currently, TestCase doesn't do it, and IMO assuming the IDs are
> going to start from 1 is an assumption one should not make.
>
> Objections to just getting rid of the sequence resets altogether (with
> the opt-in flag)?

Hi all,

Based on Andreas' work and discussion on this thread, I've:

1. If the user specifies the `reset_sequences` flag, what is performed
during test case setup is actually that: A reset of DB sequences.
There is no more flushing of tables.

For this a method was added to DB backends DatabaseOperation classes.

2. Dropped resetting of sequences from the DB flush now performed during
test case teardown code. For this, a stealth option was added to the
`flush` management command.

3. Expanded and re-factored [related] documentation a bit.

1. and 2. mean that now:

* Pre test case execution: There is no DB table flushing nor reset of
sequences (the latter can be asked for the developer by using
reset_sequences.

* Post test case execution: Flushing of DB tables is performed.

https://github.com/ramiro/django/compare/pr45_t18271

So far, I count two favorable from core devs: Karen and Anssi.

Any further feedback about the proposed changes is welcome.
Especially reports of results of people running their own test suites.

I ran the Django test suite w/o against Postgres, MySQL and sqlite3
w/o problems.

Hopefully I will be able to commit something along the lines soon.

Regards,

--
Ramiro Morales

[Anssi Kääriäinen]

On 10 heinä, 00:35, Ramiro Morales <cra...@gmail.com> wrote:
> On Mon, May 7, 2012 at 6:50 PM, Anssi Kääriäinen
>

I don't know if this is the proper forum to give a review... but here
goes anyways :)

I spotted one error:
https://github.com/ramiro/django/compare/pr45_t18271#L7R472 - The
line should say conn.cursor(), not connection.cursor().

And a question:
This change in https://github.com/ramiro/django/compare/pr45_t18271#L9R502
says that changing a TestCase to TransactionTestCase might solve some
errors caused by test ordering. I don't get why this is true.

I ran the full test suite on SQLite, and got this error:
======================================================================
ERROR: test_pass_connection_between_threads
(regressiontests.backends.tests.ThreadTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/akaariai/Programming/django/tests/regressiontests/
backends/tests.py", line 595, in test_pass_connection_between_threads
self.assertTrue(isinstance(exceptions[0], DatabaseError))
IndexError: list index out of range

I don't know if this is related to this patch or not. The error does
not happen when running the backends test alone. This might be related
to test ordering, or just a random test failure (one which I haven't
seen before, though). I have no time to rerun the full test suite just
now.

- Anssi

[Ramiro Morales]

Anssi,

Thanks for the review,

On Tue, Jul 10, 2012 at 2:16 AM, Anssi Kääriäinen
<anssi.ka...@thl.fi> wrote:
>
> I spotted one error:
> https://github.com/ramiro/django/compare/pr45_t18271#L7R472 - The
> line should say conn.cursor(), not connection.cursor().

Fixed in the last iteration of the branch.

>
> I ran the full test suite on SQLite, and got this error:
> ======================================================================
> ERROR: test_pass_connection_between_threads
> (regressiontests.backends.tests.ThreadTests)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/akaariai/Programming/django/tests/regressiontests/
> backends/tests.py", line 595, in test_pass_connection_between_threads
> self.assertTrue(isinstance(exceptions[0], DatabaseError))
> IndexError: list index out of range
>
> I don't know if this is related to this patch or not. The error does
> not happen when running the backends test alone. This might be related
> to test ordering

Exactly, this is what I've found is happening:

The problems shows when you run together (minimal expression):

A. Any test from the servers `regression` tests (that subclass
TransactiontestCase via LiveServerTestCase)
B. The test that is generating the error: A test from the `backends`
regression tests (that subclass TestCase)

e.g.

runtests.py --settings=test_sqlite servers.LiveServerViews.test_404
backends.ThreadTests.test_pass_connection_between_threads

For SQLite, memory-hosted databases, LiveServerTestCase sets main thread's
'default' DB connection's allow_thread_sharing to True.

The `backends` tests:

* Spawns a second helper thread
* It clobbers this thread's connection with the main thread's one.
* Both main and helper threads 'default' connections' allow_`thread_sharing`
values are initialized or assumed be False and then the effects of such
value are then verified.

Currently, tests that subclass TestCase always run first so the order is
always B before A and no problem occurs. Even when the main thread default
connection `allow_thread_sharing` value state is preserved across test cases
after all the LiveserverTestCase have finished.

But when the patch is applied such order isn't guaranteed anymore (all the
tests that inherit from unittest.TestCase are run intermixed w/o a defined
orderiing.)

The test discovery seems to always order them so A runs before B, the main
thread connection allow_thread_sharing=True values survives, causes a
condition that isn't expected by the B test and so the error occurs.

Maybe we can fix the LiveServerTestCase so it doesn't leak the munging of
`allow_thread_sharing` it performs by resetting its value in its
classTearDown() method?.

>
> And a question:
> This change in https://github.com/ramiro/django/compare/pr45_t18271#L9R502
> says that changing a TestCase to TransactionTestCase might solve some
> errors caused by test ordering. I don't get why this is true.

It was text I had moved from a spot below in the same document. It applied to
the changes introduced in 1.1 and is actually inaccurate now. My fault,
I've reworded it.

Regards,

--
Ramiro Morales

[Ramiro Morales]

On Thu, Jul 19, 2012 at 8:32 PM, Ramiro Morales <cra...@gmail.com> wrote:
>> I ran the full test suite on SQLite, and got this error:
>> ======================================================================
>> ERROR: test_pass_connection_between_threads
>> (regressiontests.backends.tests.ThreadTests)
>> ----------------------------------------------------------------------
>> Traceback (most recent call last):
>> File "/home/akaariai/Programming/django/tests/regressiontests/
>> backends/tests.py", line 595, in test_pass_connection_between_threads
>> self.assertTrue(isinstance(exceptions[0], DatabaseError))
>> IndexError: list index out of range
>>
>> I don't know if this is related to this patch or not. The error does
>> not happen when running the backends test alone. This might be related
>> to test ordering
>
> Exactly

> [...]

> Maybe we can fix the LiveServerTestCase so it doesn't leak the munging of
> `allow_thread_sharing` it performs by resetting its value in its
> classTearDown() method?.
>

Julien solved this precisely in that way, the fix got applied in:

https://github.com/django/django/commit/ea667ee3aeed33bce1dd681d9c0ea42f9926db5a

This hopefully completes the changes motivated by the feedback you provided.
I intend to apply this patch in the next days, any further opinions is welcome.

Regards,

--
Ramiro Morales