Opened 3 years ago

Last modified 2 years ago

#21540 assigned Bug

TestCase with multiple assertRaises fails with TransactionManagementError

Reported by: Kevin Christopher Henry Owned by: Shai Berger
Component: Documentation Version: 1.6
Severity: Normal Keywords:
Cc: chris.jerdonek@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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\", line 16, in test_unique
    self.assertRaises(IntegrityError, NameModel.objects.create, first_name="A", last_name="Doe")
  File "C:\Program Other\Python27\Lib\unittest\", line 475, in assertRaises
    callableObj(*args, **kwargs)
  File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\", line 157, in create
    return self.get_queryset().create(**kwargs)
  File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\", line 319, in create, using=self.db)
  File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\", line 545, in save
    force_update=force_update, update_fields=update_fields)
  File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\", 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\", 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\", line 687, in _do_insert
    using=using, raw=raw)
  File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\", line 232, in _insert
    return insert_query(self.model, objs, fields, **kwargs)
  File "C:\PythonEnv\Trivia\lib\site-packages\django\db\models\", 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\", line 898, in execute_sql
    cursor.execute(sql, params)
  File "C:\PythonEnv\Trivia\lib\site-packages\django\db\backends\", line 47, in execute
  File "C:\PythonEnv\Trivia\lib\site-packages\django\db\backends\", 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 (11)

comment:1 Changed 3 years ago by Aymeric Augustin

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: invalid
Status: newclosed

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 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.

comment:2 Changed 3 years ago by Kevin Christopher Henry

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 Changed 3 years ago by Aymeric Augustin

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 Changed 3 years ago by Aymeric Augustin

Resolution: invalid
Status: closednew

I'm bumping this ticket back to unreviewed, given that you've reframed it as a documentation issue.

comment:6 Changed 3 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

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():

comment:7 Changed 3 years ago by Chris Jerdonek

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 Changed 3 years ago by Chris Jerdonek

Cc: chris.jerdonek@… added

comment:9 Changed 3 years ago by Chris Jerdonek

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 Changed 3 years ago by Aymeric Augustin

Component: Testing frameworkDocumentation

comment:11 Changed 2 years ago by Shai Berger

Owner: changed from nobody to Shai Berger
Status: newassigned

I'm going to try to add something here

Note: See TracTickets for help on using tickets.
Back to Top