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)
follow-up: 4 comment:1 by , 5 years ago
Cc: | added |
---|---|
Version: | 1.11 → master |
comment:2 by , 5 years ago
Component: | Testing framework → Documentation |
---|---|
Triage Stage: | Unreviewed → Accepted |
Yeah, thanks Simon, good example. Let's accept this for a Documentation patch at the least.
comment:3 by , 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.
comment:4 by , 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 , 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 , 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 , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 5 years ago
Patch needs improvement: | set |
---|
comment:9 by , 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 , 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 , 5 years ago
Cc: | added |
---|
comment:12 by , 5 years ago
Cc: | added |
---|
comment:13 by , 5 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:14 by , 5 years ago
Cc: | added |
---|
comment:15 by , 5 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
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?
comment:16 by , 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 , 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 , 4 years ago
Owner: | changed from | to
---|
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
comment:19 by , 4 years ago
Patch needs improvement: | unset |
---|
comment:20 by , 4 years ago
Component: | Documentation → Testing framework |
---|
comment:21 by , 4 years ago
Patch needs improvement: | set |
---|
comment:22 by , 4 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Hi Bernhard,
on_commit
hooks are not run duringTestCase
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 thuson_commit
hooks are never fired. I've previously discussed changing the behavior to makeon_commit
hook fire on the most innerSAVEPOINT
commit to abstract'sTestCase
wrapping but that would be backward incompatible and wouldn't account for nestedatomic(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 baseTestCase
subclassThis also allows us to assert against the
on_commit
mock'sassert_called_*
methods.Right now the proper way to test for
on_commit
execution is to use aTransactionTestCase
which doesn't performs this transaction wrapping.I guess the documentation of
TestCase
,TransactionTestCase
andon_commit
could be enhanced in this regard and a solution similar toimmediate_on_commit
could be added toTestCase
.Thoughts?