Add shared_db_wrapper for creating long-lived db state

[ktosiek]

This is my take on #105 and #243. This pull request adds a new fixture, shared_db_wrapper, that lets other fixtures access the database regardless of scope.

shared_db_wrapper is a contextmanager that creates a new savepoint/transaction, temporarily enables the cursor wrapper, and adds a finalizer through user fixture's request. This design, as opposed to making it a normal fixture, has 2 reasons:

• finalization needs to be tied to user fixture's scope, not shared_db_wrapper's
• I don't want just having some shared state loaded to leave the cursor enabled

Is this approach sensible? If so, I'll be happy to write the documentation and make sure this PR is mergeable.

TODO:

• multiple databases (using=...)
• make sure db runs after shared fixtures
• use transaction.set_rollback(True, using=...) instead of dummy exception

[blueyed]

Looks good to me so far!
Could you please squash it already?

Would this help for keeping data from migrations with transactional_db (#220 (comment))?
I was looking into https://github.com/fastmonkeys/stellar in this regard - which might be used if it becomes easily usable as a library (fastmonkeys/stellar#53).
It basically allows to create a snapshot and rollback to it, which could be used in transactional_db (or a new variant).

@pelme
What do you think?

[ktosiek]

Sure, I'll squash it shortly.

This pretty much bails out in transactional_db, so won't help there. But I think the API is restrictive enough on the user side that we can add some kind of snapshots later.

[landscape-bot]

Repository health decreased by 0.18% when pulling c2fd0cd on ktosiek:shared-db-setup into b9eb210 on pytest-dev:master.

• 1 new problem was found (including 0 errors and 1 code smell).
• No problems were fixed.

[pelme]

This is something I personally have been thinking about and wanting for a long time. The API and implementation looks good and simple to use! Passing request explicitly to tie the scope to the user fixture is the sensible (and possibly only way currently to handle this). This will be a killer feature for pytest-django! :)

Is session/module/class fixtures that is used at the same time handled properly? I see no reason why they shouldn't be, but it would be nice with a test that uses all scopes at the same time.

Transactional db support with stellar or flushes would be a really cool future addition to shared_db_wrapper.

[ktosiek]

I can see how I could safely test mixing class and function scopes, but how would I write a test for using it with session and module scopes?
Would a test that just calls the wrapper multiple times and checks the DB state around those calls good enough?

[pelme]

@ktosiek It is possible to test that with a "pytester" test, that invokes a new pytest session. See for instance

pytest-django/tests/test_unittest.py

Lines 122 to 153 in f1711f0

	def test_unittest(self, django_testdir):
	django_testdir.create_test_module('''
	from unittest import TestCase
	class TestFoo(TestCase):
	@classmethod
	def setUpClass(self):
	print('\\nCALLED: setUpClass')
	def setUp(self):
	print('\\nCALLED: setUp')
	def tearDown(self):
	print('\\nCALLED: tearDown')
	@classmethod
	def tearDownClass(self):
	print('\\nCALLED: tearDownClass')
	def test_pass(self):
	pass
	''')
	result = django_testdir.runpytest_subprocess('-v', '-s')
	result.stdout.fnmatch_lines([
	"CALLED: setUpClass",
	"CALLED: setUp",
	"CALLED: tearDown",
	"PASSED",
	"CALLED: tearDownClass",
	])
	assert result.ret == 0

.

The trick is to use the django_testdir fixture which will create a new Django project for that particular test and then invoke django_testdir.runpytest() to run and collect tests.

[ktosiek]

I'll push a pytester test shortly, but I also have some bad news: it looks like this won't work on Django 1.6 and 1.7, as they always close the DB connection in TransactionTestCase._post_teardown: https://github.com/django/django/blob/stable/1.7.x/django/test/testcases.py#L828

1.8 should be OK, as it only does that if some db backends don't support transactions.

[landscape-bot]

Repository health decreased by 0.14% when pulling b67b9ce on ktosiek:shared-db-setup into d3e03b9 on pytest-dev:master.

• 1 new problem was found (including 0 errors and 1 code smell).
• No problems were fixed.

[pelme]

Hmm, couldn't we monkey patch _post_teardown in those cases and have it not close the connection? It would for sure be nice to support Django 1.6 and 1.7 if possible!

[aaugustin]

Django 1.6 and 1.7 don't receive security updates anymore. I don't think you should refrain from merging this feature just because it doesn't work on these versions. I suggest to skip the tests on Django < 1.8.

Is there anything I can do to help completing this feature?

[pelme]

@aaugustin Agreed, it wouldn't be too bad to not support Django <1.7 with shared_db_wrapper, so let's avoid the complexities that would add.

With your expertise in Django transaction management, if you could have a quick look at the PR that would be very helpful. Also if you could try it out in a project where you need it that would be very helpful too.

There is nothing that blocks this from being merged. If I don't hear anything else I will merge it in a couple of days and push a new release. :)

[aaugustin]

I'll try it in my current project and report back (hopefully today or tomorrow).

[aaugustin]

I tried the patch and unfortunately I couldn't make it work.

I'm getting two kinds of errors:

• some tests fail, apparently because of isolation issues between tests -- which indicates the database state isn't reset between tests like I expect
• if the module where I declare a module-level shared_db_wrapper fixture also contains tests that don't touch the database and aren't marked with @pytest.mark.django_db, these tests fail with "Database access not allowed, use the "django_db" mark to enable it."

[aaugustin]

It seems to me that the next 25 lines could be written as:

        try:
            _django_cursor_wrapper.enable()
            with transaction.atomic():
                yield
                transaction.set_rollback(True)
        finally:
            _django_cursor_wrapper.restore()

This raises the question of why this fixture needs to care about _django_cursor_wrapper; it's unclear to me why this is necessary. If it isn't, the code can be further simplified to:

        with transaction.atomic():
            yield
            transaction.set_rollback(True)

[davidhalter]

@ktosiek What do you think about that?

[aaugustin]

I'm not comfortable with pytest's internal APIs, but the general scheme here should be:

    with transaction.atomic():
        # run shared_db_wrapper fixtures
        for test in in tests:
            with transaction.atomic():
                # run test
                transaction.set_rollback(True)
        transaction.set_rollback(True)

I hope this helps...

[davidhalter]

If there's issues with this, I would be glad to help out. Let me know what's still missing.

[ktosiek]

This still has problems: there's no way that I see to force py.test to only run db after shared state fixtures.
This can cause the db atomic to be started first (it should be last, or at least after all shared fixtures), and then the whole test will get run in the last shared fixture's atomic (and will rollback the last fixture's DB changes).

It looks like this behavior depends on order of fixtures in first test function, maybe we can fix it by hooking into test collection?

[davidhalter]

This still has problems: there's no way that I see to force py.test to only run db after shared state fixtures.

Would it help to use the decorator approach you proposed above? I think this would make sense even just for usability. I don't like how complicated it is, currently.

[ktosiek]

Not really, the decorator would just hide the with shared_db_wrapper(...): part, and won't change how it all works. But maybe a pytest_runtest_setup hook could be used to order test's fixtures by scope.

[davidhalter]

But with a decorator you could control the fixture order, right? You could just place it in the first spot. (Note that I don't have a lot of knowledge about py.test fixtures)

[ktosiek]

Nope, what I need is to affect relative ordering between fixtures on the actual test. It's not about fixture's dependencies. Using a decorator won't help there.

I kind of have an idea how to do that (which is a bit hacky - using pytest_runtest_setup and pre-loading some fixtures with item._request.getargvalue), but I'll have to try it (hopefully later today).

[davidhalter]

Hmm, I see! There's probably solutions for that as well, but they are quite hacky. Wouldn't this work?:

import inspect                                                                  
from textwrap import dedent                                                     
from functools import wraps                                                     


def shared_db_fixture(scope='module'):                                          
    def func_wrapper(func):                                                     
        arg_list = list(inspect.getfullargspec(func))                           
        old_args = inspect.formatargspec(*arg_list)                             
        arg_list[0].insert(0, 'request')                                        
        arg_list[0].insert(1, '_shared_db_wrapper')                             
        new_args = inspect.formatargspec(*arg_list)                             

        code = dedent("""                                                       
        @wraps(func)                                                            
        def wrapper%s:                                                          
            with _shared_db_wrapper(request):                                   
                return func%s                                                   
        """ % (new_args, old_args))                                             

        exec(code, globals(), locals())                                         
        print(code, locals())                                                   
        return locals()['wrapper']  # Don't ask me why it doesn't work directly.

    return func_wrapper                                                         


@shared_db_fixture()                                                            
def test_hello(my_fixture):                                                     
    assert myfixture.foobar                                                     

Clearly this is hacky, but it's actually very clear what it does. Hacking around with pytest_runtest_setup is probably not cleaner (and if so, please show me :)). Also, pytest itself has very hacky origins, since it uses function names as an actual feature (which is strange, when you first start with pytest, but really cool once you get it). This leads to the fact that there's no way of modiying the function input anymore, like args/kwargs in a decorator, except for exec.

Tell me what you think!

PS: This solution has a few issues when it comes to Python 2 support and certain signatures, but they are all solvable.

[ktosiek]

as a nicer looking API? Probably. But it won't help with the fixture instantiation order (you'd need to wrap each test to move db-based fixtures after shared db fixtures)

[davidhalter]

True. I've just realized how complicated this actually is.

[pelme]

/cc @nicoddemus 😅

[davidhalter]

My intermediate fix BTW is to use this:

@pytest.fixture(scope="session", autouse=True)
def setup_django_db_hack(_django_db_setup, request, _django_cursor_wrapper):
    _django_db_fixture_helper(False, request, _django_cursor_wrapper)

This initializes the database in the beginning and with that all fixtures can write to it. What do you guys think about this?

[larsrinn]

Even though this is quite an old PR, I would be very much interested in the functionality it provides (without reading through the whole thread though). Why hasn't it been merged? Just because of the merge conflicts?

[davidhalter]

IMO it's not just a simple problem and this PR doesn't solve everything, really. There are issues in the underlying pytest infrastructure/architecture that make it really hard to do this right.

My personal approach has been altered a bit, I use this for a kind of large Django project:

@pytest.fixture(scope="session", autouse=True)
def actually_setup_db(django_db_setup):
    """
    This is an autouse fixture to guarantee calling django_db_setup. If this is
    not done, Django allows accessing the sqlite.db file and modifying it. This
    is obviously NOT what we want.
    """


@pytest.fixture(scope="session")
def session_db(actually_setup_db, django_db_blocker):
    """
    Unlocks the database session-wide. The default of django is to block the
    database.
    """
    with django_db_blocker.unblock():
        yield

It's not perfect. There are some major issues that you might run into. Don't ask me why I didn't make this an autouse fixture... It's just bug-prone and you have to play with it. The fixtures also might need to be defined in the right places, etc. It's just not what you really want. They need to improve some pytest things first, IMO.

I also think that this issue might be a blocker and is highly relevant: pytest-dev/pytest#2405

[ktosiek]

IIRC this PR was blocked by problems with forcing fixture ordering (see #258 (comment)), I haven't checked if there are any new options for working around them.

[yozlet]

@ktosiek Looks like fixtures are now loaded in scope order, thanks to pytest-dev/pytest#3306 (which went into pytest 3.5.0). Does this help things?

[ktosiek]

@yozlet thank you for pointing this out, it might help. But I won't have time to work on this PR this month.

[pdonorio]

The feature of keeping db data across multiple tests would be very important in some scenarios for our tests, at the very least for performance reasons.

May I help you in any way @ktosiek to get this released?

[bluetech]

FYI, we are having some discussion on alternatives to this PR in #514.