Opened 5 years ago

Closed 4 years ago

#30457 closed New feature (fixed)

on_commit should be triggered in a TestCase

Reported by: Bernhard Mäder Owned by: Adam Johnson
Component: Testing framework Version: dev
Severity: Normal Keywords: on_commit TestCase
Cc: Simon Charette, François Freitag, Adam Johnson, Florian Demmer Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Change History (23)

comment:1 by Simon Charette, 5 years ago

Cc: Simon Charette added
Version: 1.11master

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?

comment:2 by Carlton Gibson, 5 years ago

Component: Testing frameworkDocumentation
Triage Stage: UnreviewedAccepted

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

comment:3 by Carlton Gibson, 5 years ago

...at the least.

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

in reply to:  1 comment:4 by Bernhard Mäder, 5 years ago

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.

comment:5 by Simon Charette, 5 years ago

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.

comment:6 by Adam Johnson, 5 years ago

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.

comment:7 by Swatantra, 5 years ago

Owner: changed from nobody to Swatantra
Status: newassigned

comment:8 by Johannes Maron, 5 years ago

Patch needs improvement: set

comment:9 by Dylan Young, 5 years ago

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__()

comment:10 by Simon Charette, 5 years ago

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.

comment:11 by François Freitag, 5 years ago

Cc: François Freitag added

comment:12 by Adam Johnson, 5 years ago

Cc: Adam Johnson added

comment:13 by Swatantra, 5 years ago

Owner: Swatantra removed
Status: assignednew

comment:14 by Florian Demmer, 5 years ago

Cc: Florian Demmer added

comment:15 by Jannah Mandwee, 5 years ago

Owner: set to Jannah Mandwee
Status: newassigned

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 that go 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?

Last edited 5 years ago by Jannah Mandwee (previous) (diff)

comment:16 by Jannah Mandwee, 5 years ago

Has patch: set

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.

comment:17 by Jannah Mandwee, 5 years ago

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.

comment:18 by Adam Johnson, 4 years ago

Owner: changed from Jannah Mandwee to 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

Last edited 4 years ago by Mariusz Felisiak (previous) (diff)

comment:19 by Adam Johnson, 4 years ago

Patch needs improvement: unset

comment:20 by Mariusz Felisiak, 4 years ago

Component: DocumentationTesting framework

comment:21 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:22 by Mariusz Felisiak, 4 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:23 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In e906ff6:

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

Note: See TracTickets for help on using tickets.
Back to Top