on_commit should be triggered in a TestCase

[Bernhard Mäder]

Testing code which uses transaction.on_commit() currently is inconvenient and (for me) confusing, as on_commit() is never called because of TestCase's outermost transaction.

We use something like this, woven into all those test, to run queued commit hooks on demand:

def run_commit_hooks():
    for db_name in settings.DATABASES.keys():
        connection = transaction.get_connection(using=db_name)
        current_run_on_commit = connection.run_on_commit
        connection.run_on_commit = []
        while current_run_on_commit:
            sids, func = current_run_on_commit.pop(0)
            func()

This works, but is, of course, not optimal.

I wonder if there's a reason for this behaviour, is it intentional? As those hooks are fully done within django, shouldn't it be possible to run them when the second to the last transaction is committed?

[Simon Charette]

Hi Bernhard,

on_commit hooks are not run during TestCase because of the transaction wrapping it performs to ensure data isolation. In short each test is wrapped in a transaction that is rolled back on teardown ​which is documented. That means the connection never actually commits the transaction and thus on_commit hooks are never fired. I've previously discussed changing the behavior to make on_commit hook fire on the most inner SAVEPOINT commit to abstract's TestCase wrapping but that would be backward incompatible and wouldn't account for nested atomic(savepoint=False) usages.

The only viable solution we came up with at $DAYJOB is similar to yours. We added an immediate_on_commit context manager to our base TestCase subclass

@contextmanager
def immediate_on_commit(self, using=None):
    """
    Context manager executing transaction.on_commit() hooks immediately as
    if the connection was in auto-commit mode. This is required when
    using a subclass of django.test.TestCase as all tests are wrapped in
    a transaction that never gets committed.
    """
    immediate_using = DEFAULT_DB_ALIAS if using is None else using
    def on_commit(func, using=None):
        using = DEFAULT_DB_ALIAS if using is None else using
        if using == immediate_using:
            func()
    with mock.patch('django.db.transaction.on_commit', side_effect=on_commit) as patch:
        yield patch

This also allows us to assert against the on_commit mock's assert_called_* methods.

Right now the proper way to test for on_commit execution is to use a TransactionTestCase which doesn't performs this transaction wrapping.

I guess the documentation of TestCase, TransactionTestCase and on_commit could be enhanced in this regard and a solution similar to immediate_on_commit could be added to TestCase.

Thoughts?

[Carlton Gibson]

Yeah, thanks Simon, good example. Let's accept this for a Documentation patch at the least.

[Carlton Gibson]

...at the least.

Just to be clear, it may be that an addition to django.test.utils (or similar) is also in order.

[Bernhard Mäder]

Hey Simon

Thanks for your detailed explanation and the code snippet. I think it's actually an improvement to our solution as it doesn't require us to sprinkle the code with run_commit_hooks(), which may lead to confusing behavior such as hook running too late. So, I'm going to use that, thank you!

I still think the default behaviour of TestCase is cumbersome for whenever one uses on_commit in a codebase. But, looking at the actual implementation, changing this seems like it would need changes in the connections themselves, which probably goes too far.

And, I agree that your snippet would be great to have in the documentation. It would be cool if the behaviour would even be available as a config option (static member flag?) on TestCase.

[Simon Charette]

I still think the default behaviour of TestCase is cumbersome for whenever one uses on_commit in a codebase.

I do agree on that and that's what motivated me to try to get things working automatically but as you've said yourself it's quite hard to emulate. This also contradicts the documentation about how transactional behaviour should be tested using TransactionTestCase.

It would be cool if the behaviour would even be available as a config option (static member flag?) on TestCase.

Right. I guess we could also emit a runtime warning when on_commit is used within a TestCase context that points at either this option or the context decorator itself.

[Adam Johnson]

I've also tested this with unittest.mock.patch on transaction.on_commit, checking it was invoked enough times, then invoking the collected functions in the tests.

A helper in django.test.utils would be cool.

[Dylan Young]

Here's a mixin for your TestCases:

class ImmediateOnCommitMixin(object):
    """
    TODO: Remove when immediate_on_commit function is actually implemented
    Django Ticket #: 30456, Link: https://code.djangoproject.com/ticket/30457#no1
    """
    @classmethod
    def setUpClass(cls):
        super(ImmediateOnCommit, cls).setUpClass()
        def immediate_on_commit(func, using=None):
            func()
        # Context manager executing transaction.on_commit() hooks immediately
        # This is required when using a subclass of django.test.TestCase as all tests are wrapped in
        # a transaction that never gets committed.
        cls.on_commit_mgr = mock.patch('django.db.transaction.on_commit', side_effect=immediate_on_commit)
        cls.on_commit_mgr.__enter__()

    @classmethod
    def tearDownClass(cls):
        super(ImmediateOnCommit, cls).tearDownClass()
        cls.on_commit_mgr.__exit__()

[Simon Charette]

FWIW we switched to an approach that defers hooks execution to the end of the context manager instead of executing immediately by default to properly mimic what an atomic block would do.

@contextmanager
def execute_on_commit(immediately=False, using=None):
    """
    Context manager capturing transaction.on_commit() calls and executing
    them on exit or immediately if specified.

    This is required when using a subclass of django.test.TestCase as all
    tests are wrapped in a transaction that never gets committed.
    """
    for_alias = DEFAULT_DB_ALIAS if using is None else using
    deferred = []

    def side_effect(func, using=None):
        alias = DEFAULT_DB_ALIAS if using is None else using
        if alias != for_alias:
            return
        if immediately:
            return func()
        deferred.append(func)

    with mock.patch('django.db.transaction.on_commit') as patch:
        patch.side_effect = side_effect
        yield patch
    for func in deferred:
        func()

For example this was necessary to test edge cases such as lazy loading failure of deleted objects

parent = Node.objects.create()
node = Node.objects.create(parent=parent)
with self.immediate_on_commit():
    # imagine a pre-delete signal scheduling the following hook.
    transaction.on_commit(lambda: node.parent.something())
    parent.delete()

Would fail in a normal scenario because parent doesn't exist anymore one the transaction commits. This is covered by execute_on_commit.

[Jannah Mandwee]

Hello Django Team,

I am working on my final project for my undergraduate software engineering class (here is the link to the assignment: ​​https://web.eecs.umich.edu/~weimerw/481/hw6.html). I have to contribute to an open-source project and would like to contribute to this ticket. Thank you very much.

EDIT:
It seems like the main way around this problem would be to make changes beyond the functionality of TestCase, but this would cause a lot more problems than solutions. Is there another solution besides updating documentation that I may be missing?

[Jannah Mandwee]

​https://github.com/django/django/pull/12761

My partner and I updated the documentation to guide users to the "use in tests" section of the document. Hopefully this will clear up any future confusion on using on_commit() in tests.

[Jannah Mandwee]

​https://github.com/django/django/pull/11805

We received a response to a pull request that is already open for this issue. I've commented it here for convenience.

[Adam Johnson]

I've created an alternate PR that adds TestCase.captureOnCommitCallbacks : ​PR #12944. This returns a context manager that captures the hooks and optionally calls them as the context manager yields. It also returns them as a list so assertions can be made on the hooks, and they can be selectively executed.

I also put the code in a package tested on Django 2.0+: ​https://github.com/adamchainz/django-capture-on-commit-callbacks

[Mariusz Felisiak <felisiak.mariusz@…>]

In e906ff6:

Fixed #30457 -- Added TestCase.captureOnCommitCallbacks().