[Django] #32409: TestCase async tests are not transaction-aware
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Type: Bug | Status: new
Component: Testing | Version: 3.1
framework | Keywords: TestCase, async,
Severity: Normal | transaction
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
`SimpleTestCase` wraps each ''async'' test with `async_to_sync` (see
[https://github.com/django/django/commit/17009e910538b9c3709d302f54c2d10eb245cf38
commit]).
When using `TestCase` this results in problems while accessing the
database via `sync_to_async` (or with channels `database_sync_to_async`).
Since under-the-hood `aync_to_sync` runs the async code in an other thread
and savepoints (aka: transactions) are thread-bound what happens is that
those tests do not interact with the actual ("expected") database which is
wrapped in a transaction bound to main thread.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage:
transaction | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Andrew Godwin, Carlton Gibson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
transaction |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Bug => New feature
* stage: Unreviewed => Accepted
Comment:
Hi David. Thanks for the report.
The following diff gives an `OperationalError` ''database table is
locked'', so yes.
{{{
(django) ~/Documents/Django-Stack/django/tests (master)$ GIT_PAGER="" git
diff
diff --git a/tests/async/tests.py b/tests/async/tests.py
index 783fbdd080..076cd9bc1d 100644
--- a/tests/async/tests.py
+++ b/tests/async/tests.py
@@ -2,11 +2,11 @@ import os
import sys
from unittest import mock, skipIf
-from asgiref.sync import async_to_sync
+from asgiref.sync import async_to_sync, sync_to_async
from django.core.cache import DEFAULT_CACHE_ALIAS, caches
from django.core.exceptions import SynchronousOnlyOperation
-from django.test import SimpleTestCase
+from django.test import SimpleTestCase, TestCase
from django.utils.asyncio import async_unsafe
from .models import SimpleModel
@@ -61,3 +61,19 @@ class AsyncUnsafeTest(SimpleTestCase):
self.dangerous_method()
except SynchronousOnlyOperation:
self.fail('SynchronousOnlyOperation should not be raised.')
+
+
+class SyncToAsyncTestCase(TestCase):
+
+ @classmethod
+ def setUpTestData(cls):
+ SimpleModel.objects.create(field=1)
+
+ async def test_using_sync_to_async(self):
+ @sync_to_async
+ def fetch():
+ return list(SimpleModel.objects.all())
+
+ data = await fetch()
+ self.assertEqual(len(data), 1)
+ self.assertEqual(data[0].field, 1)
}}}
I'm inclined to think this a documentation issue — that nested
sync_to_async calls are not yet supported — plus a new feature request to
add that support.
i.e. that the usage (whilst presumably reasonable) is beyond where
Django's async support has yet reached. (I will provisionally accept on
that basis, but happy if we want to adjust that.)
Can I ask what the use-case is? What are you trying to do using this
pattern?
It looks like a tough one to actually address until we can get the async
wrapper layer around the ORM. 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
I've opened a possible [https://github.com/django/django/pull/13965 PR]
just noting the limitation as it is now.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David):
Hi Carlton,
I did not think about nested `sync_to_async` and `async_to_sync` could
cause issue.
My use case is the following: I have a websocket users use to acquire and
release "lock" on a specific database row, the "lock" is also a database
table which connects user and the other one. Database access is required
by websocket since it needs to verify that the requested element was not
already locked by an other user (and this is the failing test).
My backend was build with Django v2.2 and Channels v1 (using also
multiplexing).
I am now moving to Django v3 which has forced me also to update Channels
since otherwise there would be a conflict with `asgiref` dependency.
Since from Channels v1 to v2 (also v3) there are many breaking changes I
have been forced to rewrite my unittests. Reading
[https://docs.djangoproject.com/en/3.1/topics/testing/tools/#async-tests
Django 3.1 supporting async testing] I thought to try it out (since I am
not familiar with pytest fixtures and database presets) and migrate my
existing tests.
I had a similar issue with an other websocket, which was due to
authorization (since I use a permission-check mechanism to accept
websocket connections), which was work-arounded by pre-fetching
permissions and test user in `TestCase`:
{{{
#!python
class ConsumerTestCase(TestCase):
fixtures = ['test_authorization', ]
@classmethod
def setUpTestData(cls):
user = User.objects.get_by_natural_key('other')
assign_perm('app.model', user) # from guardian.shortcuts import
assign_perm
# force perm cache to be evaluated here
# without this even if DB is in sync there could be a miss-read
due to transactions
user.has_perm('app.model')
cls.user = user
async def test_connect(self):
application = AuthMiddlewareStack(WebsocketConsumer.as_asgi())
communicator = self._connect_to_consumer(application)
connected, _subprotocol = await communicator.connect()
self.assertTrue(connected)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David):
Using debugger I noticed this behaviour:
{{{
TestCase
|
| async_to_sync-> Thread (test)
|--------------------|
| |
| | (database_)sync_to_async ->Thread (ORM)
| |--------------------------------|
}}}
Since DB connection (along with savepoint/transaction references) resides
in the main thread it is hidden from re-synced code.
This behaviour could be fine if there is not any open transaction, since
every thread can hit the database independently. However, like in this
case, when the main thread opens a transaction this info is not trasmitted
through async-thread-chain thus resulting in reading a "wrong" state from
the database.
It could be addressed by making `async_to_sync` and `sync_to_async`
transaction-aware. I do not know if this can be achieved without an async-
ready database client.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
> I do not know if this can be achieved without an async-ready database
client.
Yes, That's kind of where I'm at with it. :| — This is a Difficult
Problemâ„¢.
In traditional request-response scenarios we can reasonably assume that
there's a single transaction in a single thread and not worry about what
goes on outside that. Once there are multiple threads in play you start
wanting (that's not the right word perhaps) to think about isolation
levels, how you'll handle conflicts and so on, and what's right in that
space for your application (which may not be general). At the moment we're
constrained by the ORM not having been built with any in mind at all, so
the required safety wheels do impinge.
For now, can you try writing this particular test with pytest-asyncio?
That should skip the outer async_to_sync(), which should/may hopefully
mean that the sync_to_async call is run on the outermost thread. (Without
sitting down with the exact test and the debugger it's hard to say
exactly.)
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David):
I will try to move this test under pytest.
Then I will try to build a sample project with a simplified version of my
actual test for further analysis.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David):
Here is a demo project with the use-case I described before:
https://github.com/sevdog/test-websocker-user-lock.
I have setup both **py-test** (however something is still missing here,
since they work if run one-by-one but fails when run in sequence, I should
learn more about this library) and **django testcase**.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Andrew Godwin):
I'm not sure quite why this is happening - any recent release of `asgiref`
ensures that every sync_to_async call runs in the same thread as we now
default thread-sensitive mode to on; precisely to stop this kind of
behaviour.
Can you verify you're seeing this with the most recent asgiref release?
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David):
Hi Andrew, in both my main project and test-one I use `asgiref==3.3.1`
which, as of now, is the latest available on pypi (Nov 2020).
I will give a try with latest commit from github to see if this issue
persists in the main project.
Thank you.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Andrew Godwin):
No, that's plenty new, the change happened in 3.3.0. In that case, I think
the best course of investigation here is why the outer sync thread with
the transaction in it, and the inner sync thread the ORM runs in, are
different. I can take a look at that in a few weeks, but unfortunately I'm
pretty busy at work right now so I can't do it immediately.
If anyone else is interested in doing that investigation, the thing I
normally do is go edit the Django source code at relevant points and make
it print the current thread ID, then run a test and see what thread IDs
get printed out during it.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David):
I have added thead-id logging to my test app and unittests.
Looking at [https://github.com/sevdog/test-websocker-user-
lock/blob/8a953b19ec3c0725cf336df93b1d5c7644965041/ws_lock/tests/test_with_django.py#L18
connect test] It seems that in some way with django `TestCase` an extra
thread gets spawned.
{{{
test-setup 140537656747840
test 140537592018688
db-connection-2 140537583625984
--- error gets thrown
}}}
While the [https://github.com/sevdog/test-websocker-user-
lock/blob/8a953b19ec3c0725cf336df93b1d5c7644965041/ws_lock/tests/test_with_pytest.py#L9
corresponding test] with pytest does not.
{{{
fixture-sync 140584466261824
fixture-sync 140584466261824
fixture-async 140584466261824
db-connection-2 140584401794816
connect 140584466261824
db-connection-1 140584401794816
test 140584466261824
db-connection-4 140584401794816
disconnect 140584466261824
--- test successfull
}}}
I checked against the test case provided by Carlton and seems that it
passes with `asgiref==3.3.1`. I do not know if `ApplicationCommunicator`
from `asgiref` are bound in some way to this issue.
I will investigate were the source of the problem lies and create an
appropriate test case for it.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David):
Looking deeper at [https://github.com/pytest-dev/pytest-
django/blob/59fdb49fbc380bc09bcac65de8acab767194b553/pytest_django/fixtures.py#L126-L153
pytest] integration I found that in my case it does not behave like
`TestCase` but like `TransactionTestCase`.
When using `TransactionTestCase` all my unit-tests pass.
I have also add the "simple-fetch" test suggested by Carlton to my sample
project tests along with automated testing to better see what is going on
(results can be see [https://github.com/sevdog/test-websocker-user-
lock/actions?query=workflow%3ATests here]).
Usin sqlit3 tests fail due to this error:
`django.db.utils.OperationalError: database table is locked:
ws_lock_grouptypevisibility`. From what I have found this error is due to
a concurrent connection with an uncommitted-transaction.
So the whole problem seems to be bound to ''transactions'' and how they
behave with async code. In a normal/production working flow this ''may''
not be a problem, but while running tests it is.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
I cannot reproduce this issue with Django's `TestCase` or
`TransactionTestCase`. I tried with SQLite and PostgreSQL, and asgiref
3.3.0 and 3.3.1. Puzzled.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => needsinfo
Comment:
Closing as "needsinfo", since we don't have a reproducible scenario.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution: needsinfo
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David):
Replying to [comment:15 Mariusz Felisiak]:
> Closing as "needsinfo", since we don't have a reproducible scenario.
Hi Mariusz, sorry to not have replied earlier but I had provided a
complete test scenario in this repository: https://github.com/sevdog/test-
websocker-user-lock
Replying to [comment:8 David]:
> Here is a demo project with the use-case I described before:
https://github.com/sevdog/test-websocker-user-lock.
> I have setup both **py-test** (however something is still missing here,
since they work if run one-by-one but fails when run in sequence, I should
learn more about this library) and **django testcase**.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: David | Owner: nobody
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: needsinfo =>
Comment:
Sorry for missing a sample project. Let's reopen this ticket for further
investigation. I still cannot reproduce it with a small set of tests
without `channels` etc. (see also similar not-async tickets #32416 and
#29062).
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:17>
Django
-------------------------------------+-------------------------------------
| GuangZe
Type: New feature | Status: assigned
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Jin GuangZe
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:18>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aleksi Hoffman):
I'm faced with a similar {{{OperationalError}}} whilst testing an async
middleware using {{{TestCase}}}.
The db accessing method in the middleware is decorated with {{{
@database_sync_to_async }}} from Channels. Package versions are
{{{django==3.2.8}}} {{{channels==3.0.4}}}, {{{ asgiref==3.4.1 }}} and
{{{pytest==6.2.5}}}.
Decorating the async test case with both {{{@pytest.mark.asyncio}}} and
{{{@async_to_sync}}} seems to make the problem go away.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:19>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Component: Testing framework | Version: 3.1
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Oleg):
I have faced the same issue when writing tests for API implemented using
`strawberry-graphql-django`.
Strawberry converts django calls using `sync_to_async(func,
thread_sensitive=True)`.
Initially, I've converted tests to fully async ones, for example:
{{{#!python
@pytest.mark.django_db
async def test_currencies(setup_for_currency):
schema = gql.Schema(query=Query)
result = await schema.execute(
"""
query {
currency {
currencies {
name
}
}
}
"""
)
}}}
As a result, tests based on `TestCase` don't work, while
`TransactionTestCase` based do work. This happens because fixture
`setup_for_currency` and `TestCase` setup and tear-down are executed
against a different DB connection.
However, if I change upper-level test-case to synchronous one, everything
works!
{{{#!python
@pytest.mark.django_db
def test_currencies(setup_for_currency):
schema = gql.Schema(query=Query)
result = async_to_sync(schema.execute)(
"""
query {
currency {
currencies {
name
}
}
}
"""
)
}}}
Looks like this happens according to `asgiref` documentation:
>If the outermost program is async (i.e. SyncToAsync is outermost), then
>this will be a dedicated single sub-thread that all sync code runs in,
>one after the other. If the outermost program is sync (i.e. AsyncToSync
is
>outermost), this will just be the main thread. This is achieved by idling
>with a CurrentThreadExecutor while AsyncToSync is blocking its sync
parent,
>rather than just blocking.
However, this behavior is still very confusing, I have no idea if this
could be fixed.
Using `TransactionTestCase` everywhere is a no-no for large projects, but
my solution still looks like a hack.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:20>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: TestCase, async, | Triage Stage: Accepted
transaction |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: KangDo96 => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:21>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Severity: Normal | Resolution:
transaction | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => worksforme
Comimg back here after some time and now I have tested that thanks to
`asgiref` and `django` updates this problems seems to have been solved: my
example repo now shows how everithing works fine
[https://github.com/sevdog/test-websocker-user-
lock/actions/runs/9445655867 in latest run].
At the moment I do not have enough time to deep investigate how or when it
was solved, i belive this can be closed now.
--
Ticket URL: <https://code.djangoproject.com/ticket/32409#comment:22>