-
Notifications
You must be signed in to change notification settings - Fork 348
Add support for db fixture (test inside transaction) for asyncio tests #1223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a code review
From what I can briefly see; this looks great, i've personally had this issue come up, so happy you've taken this on!
@kingbuzzman CI and myself are now happy with where the branch is at, feel free to have a deeper look now. |
@lode-braced can you please sync with |
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5/8 files reviewed. More to follow.
@@ -53,6 +53,10 @@ coverage = [ | |||
"coverage[toml]", | |||
"coverage-enable-subprocess", | |||
] | |||
async = [ | |||
"asgiref>=3.9.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a django requirement, not ours.
"asgiref>=3.9.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this before i saw, you use it in fixtures.py
now im in two minds... I don't have a strong opinion. I'll leave this up for discussion.
@@ -3,7 +3,7 @@ | |||
from __future__ import annotations | |||
|
|||
import os | |||
from collections.abc import Generator, Iterable, Sequence | |||
from collections.abc import AsyncGenerator, Generator, Iterable, Sequence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move AsyncGenerator
inside if TYPE_CHECKING:
-- i will be updating the linter at some point...
from collections.abc import AsyncGenerator, Generator, Iterable, Sequence | |
from collections.abc import Generator, Iterable, Sequence |
@pytest.mark.parametrize("run_number", [1, 2]) | ||
@pytestmark | ||
@pytest.mark.django_db | ||
async def test_async_db(db, run_number) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
async def test_async_db(db, run_number) -> None: | |
async def test_async_db(run_number) -> None: |
|
||
|
||
@fixturemark | ||
async def db_item(db) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
async def db_item(db) -> Any: | |
async def db_item() -> Any: |
|
||
|
||
@pytest.fixture | ||
def sync_db_item(db) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
def sync_db_item(db) -> Any: | |
def sync_db_item() -> Any: |
|
||
@pytestmark | ||
@pytest.mark.xfail(strict=True, reason="Sync fixture used in async test") | ||
async def test_db_item(db_item: Item, sync_db_item) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async def test_db_item(db_item: Item, sync_db_item) -> None: | |
@pytest.mark.usefixtures("db_item", "sync_db_item") | |
async def test_db_item() -> None: |
|
||
|
||
@pytest.mark.xfail(strict=True, reason="Async fixture used in sync test") | ||
def test_sync_db_item(async_db_item: Item, sync_db_item) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_sync_db_item(async_db_item: Item, sync_db_item) -> None: | |
@pytest.mark.usefixtures("db_item", "sync_db_item") | |
def test_sync_db_item() -> None: |
def _unblocked_sync_only(self, wrapper_self: Any, *args, **kwargs): | ||
__tracebackhide__ = True | ||
if threading.current_thread() != threading.main_thread(): | ||
raise RuntimeError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily think this is bad, but this is definitely new functionality that would be better in another PR that can be studied independently. @bluetech thoughts?
def worker() -> None: | ||
try: | ||
# Any ORM operation that touches the DB will attempt to ensure a connection. | ||
# This should raise from the "sync_only" db blocker in non-main threads. | ||
Item.objects.count() | ||
except BaseException as exc: # noqa: BLE001 - we want to capture exactly what is raised | ||
captured[0] = exc | ||
|
||
t = threading.Thread(target=worker) | ||
t.start() | ||
t.join() | ||
|
||
assert captured[0] is not None, "Expected DB access in worker thread to raise an exception" | ||
assert isinstance(captured[0], RuntimeError) | ||
assert "only allowed in the main thread" in str(captured[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[opinion] This is.. a lot. I think something like this would be clearer.
from concurrent.futures import ThreadPoolExecutor
@pytest.mark.django_db
def test_sync_db_access_in_non_main_thread_is_blocked() -> None:
"""Ensure that sync DB access from a non-main thread is not allowed."""
Item.objects.create(name="spam")
with ThreadPoolExecutor() as executor:
future = executor.submit(Item.objects.count)
with pytest.raises(RuntimeError, match="only allowed in the main thread"):
future.result()
@pytest.mark.django_db(transaction=True)
def test_transactional_db_access_in_non_main_thread_is_allowed() -> None:
"""Ensure that transactional DB access from a non-main thread is allowed."""
Item.objects.create(name="spam")
with ThreadPoolExecutor() as executor:
future = executor.submit(Item.objects.count)
assert future.result() == 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS this should be in the other PR I mentioned above.
In using and debugging pytest-django with async tests & fixtures, I ended up hacking together a way to get the db fixture working by enabling the transaction in the sync to async executor thread in which async orm queries run.
This PR aims to integrate that hack as actual functionality into pytest-django.
The async work is done, I also worked to refine the sync side database access: not allowing db access to threads other than the main thread which is running the test (& transaction). That second part is causing some test failures I've yet been unable to fix on 3.9 & django 4.2, will try to fix or drop it from the PR.
edit: I managed to fix the issue with the test failures: my monkeypatches were dropping the self attribute. Ready for review.
Suggestions & feedback welcome.