Opened 11 years ago
Last modified 8 months ago
#21540 new Bug
TestCase with multiple assertRaises fails with TransactionManagementError
Reported by: | Kevin Christopher Henry | Owned by: | |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | chris.jerdonek@…, Ülgen Sarıkavak | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (15)
comment:1 by , 11 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 by , 11 years ago
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 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 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.
comment:3 by , 11 years ago
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.
comment:4 by , 11 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
I'm bumping this ticket back to unreviewed, given that you've reframed it as a documentation issue.
comment:5 by , 11 years ago
Discussion here: https://groups.google.com/forum/#!topic/django-developers/26ZCcUPubro.
comment:6 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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()
comment:7 by , 11 years ago
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 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.
comment:8 by , 11 years ago
Cc: | added |
---|
comment:9 by , 11 years ago
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.
comment:10 by , 11 years ago
Component: | Testing framework → Documentation |
---|
comment:11 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I'm going to try to add something here
comment:12 by , 4 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:13 by , 2 years ago
I just checked, this bug is still a bug as of commit d62563cbb194c420f242bfced52b37d6638e67c6
comment:14 by , 2 years ago
Component: | Documentation → Testing framework |
---|---|
Version: | 1.6 → dev |
comment:15 by , 8 months ago
Cc: | added |
---|
The correct code is:
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 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.