Fix ordering of finalizers in db fixture

[theY4Kman]

See https://bitbucket.org/hpk42/pytest/src/17a246750c75f5eda01d065b9c72ab9fb0f1b544/_pytest/python.py?at=default#cl-1756

pytest's addfinalizer method appends finalizers to a list, and pops them off the end to call in finish(). These two lines must be switched to prevent _post_teardown accessing the blocking CursorWrapper.

It's a bit ugly in this case, but you might want to use yield_fixtures to make the ordering more explicit:

@pytest.yield_fixture(scope="function")
def django_db(_django_cursor_wrapper):
    if ('transactional_db' not in request.funcargnames and
            'live_server' not in request.funcargnames and
            not is_django_unittest(request.node)):

        from django.test import TestCase

        with _django_cursor_wrapper:
            case = TestCase(methodName='__init__')
            case._pre_setup()
            yield
            case._post_teardown()

    yield

It looks pretty, except for that yield at the end, which is necessary when that if case is False.

[pelme]

Thanks for the report! I think the order should be changed. Does this result in an actual error (i.e is a useful db cursor needed in _post_teardown?), or is it more a matter of being correct? I have never seen any problems with this myself.

Is it possible to create a test case which shows this error?

We support older versions of pytest which does not have yield_fixture, so it has to use addfinalizer.

[theY4Kman]

We were getting

___________________________________ ERROR at teardown of test_products_listed ___________________________________
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/test/testcases.py:796: in _post_teardown
>           self._fixture_teardown()
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/test/testcases.py:889: in _fixture_teardown
>           return super(TestCase, self)._fixture_teardown()
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/test/testcases.py:817: in _fixture_teardown
>                        inhibit_post_syncdb=self.available_apps is not None)
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/core/management/__init__.py:159: in call_command
>       return klass.execute(*args, **defaults)
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/raven/contrib/django/management/__init__.py:37: in new_execute
>           return original_func(self, *args, **kwargs)
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/core/management/base.py:285: in execute
>           output = self.handle(*args, **options)
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/core/management/base.py:415: in handle
>       return self.handle_noargs(**options)
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/core/management/commands/flush.py:52: in handle_noargs
>                            allow_cascade=allow_cascade)
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/core/management/sql.py:113: in sql_flush
>           tables = connection.introspection.django_table_names(only_existing=True)
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/db/backends/__init__.py:1240: in django_table_names
>           existing_tables = self.table_names()
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/db/backends/__init__.py:1210: in table_names
>           cursor = self.connection.cursor()
/home/they4kman/.virtualenvs/store.hivelocity.net/local/lib/python2.7/site-packages/django/db/backends/__init__.py:159: in cursor
>           cursor = util.CursorWrapper(self._cursor(), self)
E           Failed: Database access not allowed, use the "django_db" mark to enable

_post_teardown calls TransactionTestCase._fixture_teardown, which runs the "flush" management command. It's at that point it failed for us. I'll work on a test case tonight.

[theY4Kman]

Alright, it looks like this only occurs when testing with a database that doesn't support transactions. We recently switched from sqlite3 to MySQL and hit upon this error. Using MySQL with the MyISAM engine will cause this error. Provided a test case. Cheers!

[pelme]

I see, there is an open issue about supporting MySQL with MyISAM tables: #8

Then the real fix for the test suite is to add a new environment for MySQL/MyISAM in Travis CI. That would already have caught this error since there are a existing tests for the database setup/teardown.

The reason that this has not been fixed earlier is probably because there are very few who actually uses MyISAM tables. You should probably consider InnoDB using tables instead. Albeit from being very useful in a lot of cases, having transactions in your database will speed up your test suite a lot.

[theY4Kman]

Word, we switched when we ran into this :P I'll pull out the test, then.

[theY4Kman]

Removed it. (I rebased out the commit and force pushed; would you have done it differently?)

[pelme]

I would like to have Travis CI run the test suite on MyISAM tables before merging, otherwise we will never know if we break MyISAM support in the future anyways.

[julen]

I've run into this issue and spent few hours trying to figure out what was going on right after switching my app from Django 1.4 to 1.6. I've been using MyISAM, and all tests run just fine in 1.4. After reading this I've switched my tables to InnoDB to see if it made any difference, but no luck so far.

Applied the patch of this PR and now tests are back on track. @pelme it'd be great if you could merge the fix. Thanks!

[pelme]

@julen So you are saying that this error happens for you with InnoDB, too? Can you please provide an example or test that shows the breakage when using InnoDB?

Regarding MyISAM support: Travis CI does not currently run the test suite on MySQL with MyISAM tables, so it is impossible to know if we break MyISAM compatibility in the future. If anyone is interested in pytest-django having proper support for MyISAM tables in the future - we need the test suite to continuously run the tests agains MyISAM tables.

I will unfortunately not have the time to fix this in the near future, but I'll happily merge the changes instantly if anyone puts in the effort to add a MySQL/MyISAM environment to Travis!

[julen]

The error I get without the patch is exactly the same as @theY4Kman was getting above:

../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/test/testcases.py:796: in _post_teardown
>           self._fixture_teardown()
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/test/testcases.py:889: in _fixture_teardown
>           return super(TestCase, self)._fixture_teardown()
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/test/testcases.py:817: in _fixture_teardown
>                        inhibit_post_syncdb=self.available_apps is not None)
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/core/management/__init__.py:159: in call_command
>       return klass.execute(*args, **defaults)
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/core/management/base.py:285: in execute
>           output = self.handle(*args, **options)
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/core/management/base.py:415: in handle
>       return self.handle_noargs(**options)
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/core/management/commands/flush.py:52: in handle_noargs
>                            allow_cascade=allow_cascade)
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/core/management/sql.py:113: in sql_flush
>           tables = connection.introspection.django_table_names(only_existing=True)
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/db/backends/__init__.py:1240: in django_table_names
>           existing_tables = self.table_names()
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/db/backends/__init__.py:1210: in table_names
>           cursor = self.connection.cursor()
../../../.virtualenvs/pootle-django1.6/lib/python2.7/site-packages/django/db/backends/__init__.py:159: in cursor
>           cursor = util.CursorWrapper(self._cursor(), self)
E           Failed: Database access not allowed, use the "django_db" mark to enable

[pelme]

This is now merged in master. Thanks to both of you for making this happen!