[Django] #21540: TestCase with multiple assertRaises fails with TransactionManagementError
Django
-----------------------------------+--------------------
Reporter: marfire | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
This test case:
{{{
class NameModel(models.Model):
first_name = models.CharField(unique=True, max_length=30)
last_name = models.CharField(unique=True, max_length=30)
# passes in 1.5
# passes with TransactionTestCase
class UniqueTestCase(TestCase):
def setUp(self):
NameModel.objects.create(first_name="John", last_name="Doe")
# passes with either, but not both, of these assertRaises()
def test_unique(self):
self.assertRaises(IntegrityError, NameModel.objects.create,
first_name="John", last_name="Hancock")
self.assertRaises(IntegrityError, NameModel.objects.create,
first_name="A", last_name="Doe")
}}}
...fails with:
{{{
======================================================================
ERROR: test_unique (example.tests.UniqueTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"D:\Files\Documents\Programs\Trivia\assert_raises_bug\example\tests.py",
line 16, in test_unique
self.assertRaises(IntegrityError, NameModel.objects.create,
first_name="A", last_name="Doe")
File "C:\Program Other\Python27\Lib\unittest\case.py", line 475, in
assertRaises
callableObj(*args, **kwargs)
File "C:\PythonEnv\Trivia\lib\site-
packages\django\db\models\manager.py", line 157, in create
return self.get_queryset().create(**kwargs)
File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\query.py",
line 319, in create
obj.save(force_insert=True, using=self.db)
File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\base.py",
line 545, in save
force_update=force_update, update_fields=update_fields)
File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\base.py",
line 573, in save_base
updated = self._save_table(raw, cls, force_insert, force_update,
using, update_fields)
File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\base.py",
line 654, in _save_table
result = self._do_insert(cls._base_manager, using, fields, update_pk,
raw)
File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\base.py",
line 687, in _do_insert
using=using, raw=raw)
File "C:\PythonEnv\Trivia\lib\site-
packages\django\db\models\manager.py", line 232, in _insert
return insert_query(self.model, objs, fields, **kwargs)
File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\query.py",
line 1511, in insert_query
return query.get_compiler(using=using).execute_sql(return_id)
File "C:\PythonEnv\Trivia\lib\site-
packages\django\db\models\sql\compiler.py", line 898, in execute_sql
cursor.execute(sql, params)
File "C:\PythonEnv\Trivia\lib\site-packages\django\db\backends\util.py",
line 47, in execute
self.db.validate_no_broken_transaction()
File "C:\PythonEnv\Trivia\lib\site-
packages\django\db\backends\__init__.py", line 365, in
validate_no_broken_transaction
"An error occurred in the current transaction. You can't "
TransactionManagementError: An error occurred in the current transaction.
You can't execute queries until the end of the 'atomic' block.
}}}
I assume the problem here is that the `TestCase` method is being run as a
transaction, but the individual `assertRaises()` are not themselves using
atomic transactions to run the passed-in code.
--
Ticket URL: <https://code.djangoproject.com/ticket/21540>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: closed
Component: Testing framework | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* status: new => closed
* needs_better_patch: => 0
* resolution: => invalid
* needs_tests: => 0
* needs_docs: => 0
Comment:
The correct code is:
{{{
def test_unique(self):
with self.assertRaises(IntegrityError):
# roll back transaction to a clean state before letting the
IntegrityError bubble up
with transaction.atomic():
NameModel.objects.create(first_name="John",
last_name="Hancock")
with self.assertRaises(IntegrityError):
# roll back transaction to a clean state before letting the
IntegrityError bubble up
with transaction.atomic():
NameModel.objects.create(first_name="A", last_name="Doe")
}}}
This is explained in the documentation of the new transaction management
in Django 1.6. You're seeing the expected (if not totally user-friendly)
behavior.
In Django 1.5, this would not blow up in tests, but it would blow up in
actual code. I understand that you find this error annoying, but it least
you get the same behavior in tests and in actual code in Django 1.6.
I've spent lot of time thinking about this behavior and it's a Really Hard
Problem (at least for me). If you have concrete suggestions, let's discuss
on the django-developers mailing list. You may want to check past
discussions first, as well as the videos of [http://myks.org/talks my
talks] at DjangoCon Europe and DjangoCon US 2013 for more background on
the design.
I don't think it's a good idea to wrap the body of an assertRaises block
in a transaction automatically as it may be used for many purposes that do
not involve transactions.
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:1>
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: closed
Component: Testing framework | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by marfire):
To be clear, I'm not criticizing the design of the transaction management
system. I understand why database exceptions need to be caught outside of
an atomic block.
What worries me here is that a user can have (non-transactional) code that
is perfectly correct that nonetheless produces test errors simply because
of the implementation details of `TestCase` (namely that it uses
transactions instead of table truncation). It's not just `assertRaises()`
- any piece of code that, say, catches a database error and then continues
(without using transactions) will result in a test error even though it's
correct.
Of course, if the user understands transaction management they can figure
out this error and work around it in the manner you describe. But
transaction management is an "advanced" feature (per the
[https://docs.djangoproject.com/en/dev/#the-model-layer documentation])
and users shouldn't be forced to learn it to get their tests to properly
validate their working, non-transactional code. Practically speaking, many
won't, leading to spurious and confusing errors like the one above.
The right answer, I think, is that such cases require
`TransactionTestCase`. My proposal would be to update the documentation
about when to use `TransactionTestCase` vs. `TestCase`. Right now it
[https://docs.djangoproject.com/en/dev/topics/testing/overview/#django.test.TransactionTestCase
reads]: "If your test requires testing of such transactional behavior, you
should use a Django `TransactionTestCase`. `TransactionTestCase` and
`TestCase` are identical except for... the ability for test code to test
the effects of commit and rollback.... Always use `TransactionTestCase`
when testing transactional behavior." This is too limited, I think it
needs to be broadened to suggest `TransactionTestCase` for any (non-
transactional) code that catches database errors.
I'm not sure of the best wording for this, but I'd be happy to take a stab
at it if there's agreement that this is a good idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:2>
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: closed
Component: Testing framework | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
The difficulties are:
- `TransactionTestCase` is very inefficient when you have many models
(starting at a few dozens).
- The problem is not limited to database errors, it is known to happen if
a post-save signal handler raises an exception. That makes it hard to be
provide clear instructions.
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:3>
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* status: closed => new
* resolution: invalid =>
Comment:
I'm bumping this ticket back to unreviewed, given that you've reframed it
as a documentation issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:4>
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by marfire):
Discussion here: https://groups.google.com/forum/#!topic/django-
developers/26ZCcUPubro.
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:5>
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Unreviewed => Accepted
Comment:
Accepting on the basis that we should at least document the intended
pattern for this particular use case:
{{{
def test_error():
with self.assertRaises(IntegrityError):
with transaction.atomic():
raise_integrity_error()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:6>
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
I also just ran into this issue and agree improvements to the
documentation would help. A couple suggestions as someone new to Django
and using 1.6:
When I saw this error in my test:
TransactionManagementError: An error occurred in the current
transaction. You can't execute queries until the end of the 'atomic'
block.
I immediately read the Django docs about
[https://docs.djangoproject.com/en/1.6/topics/db/transactions/#module-
django.db.transaction database transactions] and my immediate thoughts
were, "that's strange, I'm not doing any manual transaction management and
I haven't touched the Django default to use autocommit."
My suggestion then would be to say something about `TestCase` vs
`TransactionTestCase` in the main "Database transactions" docs (including
a warning even), and not just include this in the testing docs. The
reason is that while the main docs say Django defaults to autocommit,
"autocommit" doesn't really seem to describe the behavior while testing
using `TestCase`. Hence, the error behavior while testing is unexpected.
I also agree that the current wording in the testing section could be more
explicit. Right now, the focus of the wording is on the ''explicit''
testing of transactions, whereas even just implicitly relying on rollback,
etc. makes you affected.
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:7>
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cjerdonek):
* cc: chris.jerdonek@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:8>
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
I recently opened a new ticket #21836 and submitted a pull request for it.
That pull request is related to this issue because I think it will make
the `TransactionManagementError` described in this issue a //little// less
surprising. This is because the pull request adds a sentence to the main
transaction docs saying that `TestCase` wraps its tests in transactions.
Currently, this fact is mentioned only in the testing docs and not in the
main transaction docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:9>
Django
Reporter: marfire | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* component: Testing framework => Documentation
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:10>
Django
Reporter: marfire | Owner: shaib
Type: Bug | Status: assigned
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* status: new => assigned
* owner: nobody => shaib
Comment:
I'm going to try to add something here
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:11>
Django
Reporter: Kevin Christopher | Owner: (none)
Henry |
Type: Bug | Status: new
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: Shai Berger => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:12>
Django
Reporter: Kevin Christopher | Owner: (none)
Henry |
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sheenarbw):
I just checked, this bug is still a bug as of commit
d62563cbb194c420f242bfced52b37d6638e67c6
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:13>
Django
Reporter: Kevin Christopher | Owner: (none)
Henry |
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sheenarbw):
* version: 1.6 => dev
* component: Documentation => Testing framework
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:14>
Django
Reporter: Kevin Christopher | Owner: (none)
Henry |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21540#comment:15>