-
Notifications
You must be signed in to change notification settings - Fork 348
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
Add shared_db_wrapper
for creating long-lived db state
#258
base: main
Are you sure you want to change the base?
Changes from all commits
fc64c57
f40fddf
38cc40c
b67b9ce
753224a
073856d
ca8bc74
dd40cd6
b228363
62fe749
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,9 @@ | |
|
||
from __future__ import with_statement | ||
|
||
from contextlib import contextmanager | ||
import os | ||
import sys | ||
import warnings | ||
|
||
import pytest | ||
|
@@ -13,7 +15,8 @@ | |
from .django_compat import is_django_unittest | ||
from .lazy_django import get_django_version, skip_if_no_django | ||
|
||
__all__ = ['_django_db_setup', 'db', 'transactional_db', 'admin_user', | ||
__all__ = ['_django_db_setup', 'db', 'transactional_db', 'shared_db_wrapper', | ||
'admin_user', | ||
'django_user_model', 'django_username_field', | ||
'client', 'admin_client', 'rf', 'settings', 'live_server', | ||
'_live_server_helper'] | ||
|
@@ -195,6 +198,58 @@ def transactional_db(request, _django_db_setup, _django_cursor_wrapper): | |
return _django_db_fixture_helper(True, request, _django_cursor_wrapper) | ||
|
||
|
||
@pytest.fixture(scope='session') | ||
def shared_db_wrapper(_django_db_setup, _django_cursor_wrapper): | ||
"""Wrapper for common database initialization code. | ||
|
||
This fixture provides a context manager that let's you access the database | ||
from a transaction spanning multiple tests. | ||
""" | ||
from django.db import connection, transaction | ||
|
||
if get_django_version() < (1, 8): | ||
raise Exception('shared_db_wrapper is only supported on Django >= 1.8.') | ||
|
||
class DummyException(Exception): | ||
"""Dummy for use with Atomic.__exit__.""" | ||
|
||
@contextmanager | ||
def wrapper(request): | ||
# We need to take the request | ||
# to bind finalization to the place where this is used | ||
if 'transactional_db' in request.funcargnames: | ||
raise Exception( | ||
'shared_db_wrapper cannot be used with `transactional_db`.') | ||
|
||
with _django_cursor_wrapper: | ||
if not connection.features.supports_transactions: | ||
raise Exception( | ||
"shared_db_wrapper cannot be used when " | ||
"the database doesn't support transactions.") | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to me that the next 25 lines could be written as: try:
_django_cursor_wrapper.enable()
with transaction.atomic():
yield
transaction.set_rollback(True)
finally:
_django_cursor_wrapper.restore() This raises the question of why this fixture needs to care about with transaction.atomic():
yield
transaction.set_rollback(True) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ktosiek What do you think about that? |
||
# Use atomic instead of calling .savepoint* directly. | ||
# This way works for both top-level transactions and "subtransactions". | ||
atomic = transaction.atomic() | ||
|
||
def finalize(): | ||
# dummy exception makes `atomic` rollback the savepoint | ||
with _django_cursor_wrapper: | ||
atomic.__exit__(DummyException, DummyException(), None) | ||
|
||
try: | ||
_django_cursor_wrapper.enable() | ||
atomic.__enter__() | ||
yield | ||
request.addfinalizer(finalize) | ||
except: | ||
atomic.__exit__(*sys.exc_info()) | ||
raise | ||
finally: | ||
_django_cursor_wrapper.restore() | ||
|
||
return wrapper | ||
|
||
|
||
@pytest.fixture() | ||
def client(): | ||
"""A Django test client instance.""" | ||
|
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 API adds some boilerplate. I'd love to write just:
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 would like that too, but there is an issue with this. When you specify a dependency to
some_users
, theshared_db_wrapper
fixture will still be active. This is a problem, because you only want to use global state for a limited amount of data (e.g. creating the users and not modifying them).I think that is why OP has called it a
shared_db_wrapper
and notshared_db
.However, maybe there's a solution around this. Maybe it's possible to somehow create a special "destructor" before entering "local" scope. However this might be complicated given how the current architecture (I have no idea about pytest internals).
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.
But to at least simplify it a bit: Why do we need access to the
request
fixture?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.
We need
request
to register a finalizer that will run when the shared fixture (and not just the wrapper) is getting torn down.Would it be desirable to put all that boilerplate in a decorator that would be used instead of (or maybe before?) pytest.fixture? Something like:
I don't think it's possible to hook into fixture instantiation in py.test, so we can't just use a marker on those fixture and wrap them later.
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 have to admit that the interactions between fixtures of different scopes are totally over my head :-/
The end goal is to call
transaction.atomic
andtransaction.set_rollback
as follows -- but I don't know how to map this to pytest's hooks and to make it work within pytest-django's existing infrastructure.If multiple databases are being used, you need to call
transaction.atomic(using=...)
andtransaction.set_rollback(True, using=...)
for each of them.Ping me if you have questions about Django's transaction management — I wrote it.
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 can't use
with atomic():
because I have to register a callback on request :-/.Thank you for pointing out transaction.set_rollback(True) - this will make my code cleaner (no more dummy exceptions!).
For the
using=...
part I might even be able to use Django's test case enter/exit atomics, or at least get some inspiration from there.