Fixed #30457 -- Added TestCase.captureOnCommitCallbacks() #12944
Conversation
adamchainz
force-pushed
the
ticket_30457
branch
2 times, most recently
from
fa08ffd to
98490c3
Compare
|
I don't have the savepoint/rollback code up right now, but I'm curious, does this flow honor if exception handling or custom savepoint/rollback logic is used, the way django (hopefully) does? So, if an on_commit is added during part of a transaction (nested or otherwise), and if the developer either catches an exception in part of the flow, or rollsback to a previous savepoint, will this behave the same way as the connection's on_commit logic does? At this point I'm assuming that django handles it correctly/as expected (e.g. removes the on_commits which are effectively rolled back, but keeps the ones which are still applicable/in-play). |
|
The answers to your questions are all in the documentation: https://docs.djangoproject.com/en/3.0/topics/db/transactions/#performing-actions-after-commit |
|
My question is more related to your implementation. Since you are wrapping on_commit with your inner function, in it you copy the callable and add it to your |
|
Oh right, sorry I misunderstood your question. You're right - this currently captures every callback, even in the case of code that rolls back transactions and therefore throws away their commit hooks. I think it can be improved to make the returned value a proxy to virtually construct the captured commit hooks - looking into it. |
|
I've made changes here and in django-capture-on-commit-callbacks to solve that, by populating the list only as the context manager exits. |
|
I've looked over the pull and it makes sense and I can't see anything wrong. We've installed the associated package and are using it happily in tests. |
adamchainz
force-pushed
the
ticket_30457
branch
2 times, most recently
from
368a619 to
0fe515b
Compare
As suggested by @felixxm : django/django#12944 (comment)
As suggested by @felixxm : django/django#12944 (comment)
There was a problem hiding this comment.
@adamchainz Thanks for updates 👍
I added docstring to TestCase.captureOnCommitCallbacks(), added test_different_using() and test_pre_callback() tests, and small edits to docs.
|
@adamchainz Do you want to take a look at my edits? |
There was a problem hiding this comment.
Your edits LGTM
One minor thought - I think we should push this up into SimpleTestCase. Whilst rare, it's possible to use the DB from within SimpleTestCase subclasses by using databases = {...} so it would be useful to have this helper available there too. What do you think?
|
I would leave it in |
|
Yes that's fair, one could always test the side effects, or copy it into the specific testcase class. |
This backports `capture_on_commit_callbacks` from django/django#12944. We need this when using `transaction.on_commit` in tests, since the callbacks will never fire due to Django always rolling back the transaction. Specifically backported for use in https://github.com/getsentry/sentry/pull/19974/files.
…20056) This backports `capture_on_commit_callbacks` from django/django#12944. We need this when using `transaction.on_commit` in tests, since the callbacks will never fire due to Django always rolling back the transaction. Specifically backported for use in https://github.com/getsentry/sentry/pull/19974/files.
Ticket
I've also released this code as a package: https://github.com/adamchainz/django-capture-on-commit-callbacks