[Django] #30457: on_commit should be triggered in a TestCase
Django
-------------------------------------+-------------------------------------
Reporter: Bernhard | Owner: nobody
Mäder |
Type: New | Status: new
feature |
Component: Testing | Version: 1.11
framework |
Severity: Normal | Keywords: on_commit TestCase
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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:
{{{#!python
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/30457>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Bernhard Mäder | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: on_commit TestCase | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
* version: 1.11 => master
Comment:
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
[https://docs.djangoproject.com/en/2.2/topics/testing/tools/#testcase
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
{{{#!python
@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?
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:1>
Django
Reporter: Bernhard Mäder | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: on_commit TestCase | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* component: Testing framework => Documentation
* stage: Unreviewed => Accepted
Comment:
Yeah, thanks Simon, good example. Let's accept this for a Documentation
patch at the least.
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:2>
Django
Reporter: Bernhard Mäder | Owner: nobody
Type: New feature | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:3>
Django
Reporter: Bernhard Mäder | Owner: nobody
Type: New feature | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:4>
Django
Reporter: Bernhard Mäder | Owner: nobody
Type: New feature | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:5>
Django
Reporter: Bernhard Mäder | Owner: nobody
Type: New feature | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:6>
Django
Reporter: Bernhard Mäder | Owner: Swatantra
Type: New feature | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Swatantra):
* status: new => assigned
* owner: nobody => Swatantra
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:7>
Django
Reporter: Bernhard Mäder | Owner: Swatantra
Type: New feature | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Johannes Hoppe):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:8>
Django
Reporter: Bernhard Mäder | Owner: Swatantra
Type: New feature | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by 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__()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:9>
Django
Reporter: Bernhard Mäder | Owner: Swatantra
Type: New feature | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
{{{#!python
@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
{{{#!python
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:10>
Django
Reporter: Bernhard Mäder | Owner: Swatantra
Type: New feature | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* cc: François Freitag (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:11>
Django
Reporter: Bernhard Mäder | Owner: Swatantra
Type: New feature | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Adam (Chainz) Johnson):
* cc: Adam (Chainz) Johnson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:12>
Django
Reporter: Bernhard Mäder | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Swatantra):
* owner: Swatantra => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:13>
Django
Reporter: Bernhard Mäder | Owner: (none)
Type: New feature | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Florian Demmer):
* cc: Florian Demmer (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:14>
Django
-------------------------------------+-------------------------------------
| Mandwee
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Jannah Mandwee
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:15>
Django
-------------------------------------+-------------------------------------
| Mandwee
Type: New feature | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:16>
Django
-------------------------------------+-------------------------------------
| (Chainz) Johnson
Type: New feature | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Jannah Mandwee => Adam (Chainz) Johnson
Comment:
I've created an alternate PR that adds `TestCase.captureOnCommitCallbacks`
: [PR #12944](https://github.com/django/django/pull/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
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:18>
Django
-------------------------------------+-------------------------------------
| (Chainz) Johnson
Type: New feature | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:19>
Django
-------------------------------------+-------------------------------------
| (Chainz) Johnson
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: on_commit TestCase | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* component: Documentation => Testing framework
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:20>
Django
-------------------------------------+-------------------------------------
| (Chainz) Johnson
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:21>
Django
-------------------------------------+-------------------------------------
| (Chainz) Johnson
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:22>
Django
-------------------------------------+-------------------------------------
| (Chainz) Johnson
Component: Testing framework | Version: master
Keywords: on_commit TestCase | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"e906ff6fca291fc0bfa0d52f05817ee9dae0335d" e906ff6]:
{{{
#!CommitTicketReference repository=""
revision="e906ff6fca291fc0bfa0d52f05817ee9dae0335d"
Fixed #30457 -- Added TestCase.captureOnCommitCallbacks().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30457#comment:23>