From 3cf80d3fcf7446afdde16a2be515c423f720e54d Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Fri, 23 Nov 2018 21:22:09 -0500 Subject: [PATCH] Fixed #31395 -- Made setUpTestData enforce in-memory data isolation. Since it's introduction in Django 1.8 setUpTestData has been suffering from a documented but confusing caveat due to its sharing of attributes assigned during its execution with all test instances. By keeping track of class attributes assigned during the setUpTestData phase its possible to ensure only deep copies are provided to test instances on attribute retreival and prevent manual setUp gymnastic to work around the previous lack of in-memory data isolation. Thanks Adam Johnson for the extensive review. --- django/test/testcases.py | 61 +++++++++++++++++- docs/internals/deprecation.txt | 4 ++ docs/releases/3.2.txt | 9 ++- docs/topics/testing/tools.txt | 12 ++-- tests/test_utils/models.py | 2 +- tests/test_utils/test_testcase.py | 102 +++++++++++++++++++++++++++++- 6 files changed, 178 insertions(+), 12 deletions(-) diff --git a/django/test/testcases.py b/django/test/testcases.py index 7ebddf80e5..5787dc0115 100644 --- a/django/test/testcases.py +++ b/django/test/testcases.py @@ -5,9 +5,10 @@ import posixpath import sys import threading import unittest +import warnings from collections import Counter from contextlib import contextmanager -from copy import copy +from copy import copy, deepcopy from difflib import get_close_matches from functools import wraps from unittest.suite import _DebugResult @@ -40,6 +41,7 @@ from django.test.utils import ( CaptureQueriesContext, ContextList, compare_xml, modify_settings, override_settings, ) +from django.utils.deprecation import RemovedInDjango41Warning from django.utils.functional import classproperty from django.views.static import serve @@ -1071,6 +1073,59 @@ def connections_support_transactions(aliases=None): return all(conn.features.supports_transactions for conn in conns) +class TestData: + """ + Descriptor to provide TestCase instance isolation for attributes assigned + during the setUpTestData() phase. + + Allow safe alteration of objects assigned in setUpTestData() by test + methods by exposing deep copies instead of the original objects. + + Objects are deep copied using a memo kept on the test case instance in + order to maintain their original relationships. + """ + memo_attr = '_testdata_memo' + + def __init__(self, name, data): + self.name = name + self.data = data + + def get_memo(self, testcase): + try: + memo = getattr(testcase, self.memo_attr) + except AttributeError: + memo = {} + setattr(testcase, self.memo_attr, memo) + return memo + + def __get__(self, instance, owner): + if instance is None: + return self.data + memo = self.get_memo(instance) + try: + data = deepcopy(self.data, memo) + except TypeError: + # RemovedInDjango41Warning. + msg = ( + "Assigning objects which don't support copy.deepcopy() during " + "setUpTestData() is deprecated. Either assign the %s " + "attribute during setUpClass() or setUp(), or add support for " + "deepcopy() to %s.%s.%s." + ) % ( + self.name, + owner.__module__, + owner.__qualname__, + self.name, + ) + warnings.warn(msg, category=RemovedInDjango41Warning, stacklevel=2) + data = self.data + setattr(instance, self.name, data) + return data + + def __repr__(self): + return '' % (self.name, self.data) + + class TestCase(TransactionTestCase): """ Similar to TransactionTestCase, but use `transaction.atomic()` to achieve @@ -1119,12 +1174,16 @@ class TestCase(TransactionTestCase): cls._rollback_atomics(cls.cls_atomics) cls._remove_databases_failures() raise + pre_attrs = cls.__dict__.copy() try: cls.setUpTestData() except Exception: cls._rollback_atomics(cls.cls_atomics) cls._remove_databases_failures() raise + for name, value in cls.__dict__.items(): + if value is not pre_attrs.get(name): + setattr(cls, name, TestData(name, value)) @classmethod def tearDownClass(cls): diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index e48356bed1..d4cc1d2d95 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -15,6 +15,10 @@ about each item can often be found in the release notes of two versions prior. See the :ref:`Django 3.2 release notes ` for more details on these changes. +* Support for assigning objects which don't support creating deep copies with + ``copy.deepcopy()`` to class attributes in ``TestCase.setUpTestData()`` will + be removed. + .. _deprecation-removed-in-4.0: 4.0 diff --git a/docs/releases/3.2.txt b/docs/releases/3.2.txt index 4ba20c4c09..b1a0e19edb 100644 --- a/docs/releases/3.2.txt +++ b/docs/releases/3.2.txt @@ -195,7 +195,10 @@ Templates Tests ~~~~~ -* ... +* Objects assigned to class attributes in :meth:`.TestCase.setUpTestData` are + now isolated for each test method. Such objects are now required to support + creating deep copies with :py:func:`copy.deepcopy`. Assigning objects which + don't support ``deepcopy()`` is deprecated and will be removed in Django 4.1. URLs ~~~~ @@ -255,4 +258,6 @@ Features deprecated in 3.2 Miscellaneous ------------- -* ... +* Assigning objects which don't support creating deep copies with + :py:func:`copy.deepcopy` to class attributes in + :meth:`.TestCase.setUpTestData` is deprecated. diff --git a/docs/topics/testing/tools.txt b/docs/topics/testing/tools.txt index b14a173987..eac4fa3ef7 100644 --- a/docs/topics/testing/tools.txt +++ b/docs/topics/testing/tools.txt @@ -873,11 +873,13 @@ It also provides an additional method: (for instance, MySQL with the MyISAM engine), ``setUpTestData()`` will be called before each test, negating the speed benefits. - Be careful not to modify any objects created in ``setUpTestData()`` in - your test methods. Modifications to in-memory objects from setup work done - at the class level will persist between test methods. If you do need to - modify them, you could reload them in the ``setUp()`` method with - :meth:`~django.db.models.Model.refresh_from_db`, for example. + .. versionchanged:: 3.2 + + Objects assigned to class attributes in ``setUpTestData()`` must + support creating deep copies with :py:func:`copy.deepcopy` in order to + isolate them from alterations performed by each test methods. In + previous versions of Django these objects were reused and changes made + to them were persisted between test methods. .. _live-test-server: diff --git a/tests/test_utils/models.py b/tests/test_utils/models.py index c40801f81b..b0497f12b9 100644 --- a/tests/test_utils/models.py +++ b/tests/test_utils/models.py @@ -12,4 +12,4 @@ class Person(models.Model): class PossessedCar(models.Model): car = models.ForeignKey(Car, models.CASCADE) - belongs_to = models.ForeignKey(Person, models.CASCADE) + belongs_to = models.ForeignKey(Person, models.CASCADE, related_name='possessed_cars') diff --git a/tests/test_utils/test_testcase.py b/tests/test_utils/test_testcase.py index 853aba7c22..5ec71f84d0 100644 --- a/tests/test_utils/test_testcase.py +++ b/tests/test_utils/test_testcase.py @@ -1,7 +1,11 @@ -from django.db import IntegrityError, connections, transaction -from django.test import TestCase, skipUnlessDBFeature +from functools import wraps -from .models import Car, PossessedCar +from django.db import IntegrityError, connections, transaction +from django.test import TestCase, ignore_warnings, skipUnlessDBFeature +from django.test.testcases import TestData +from django.utils.deprecation import RemovedInDjango41Warning + +from .models import Car, Person, PossessedCar class TestTestCase(TestCase): @@ -38,3 +42,95 @@ class TestTestCase(TestCase): ) with self.assertRaisesMessage(AssertionError, message): Car.objects.using('other').get() + + +class NonDeepCopyAble: + def __deepcopy__(self, memo): + raise TypeError + + +def assert_no_queries(test): + @wraps(test) + def inner(self): + with self.assertNumQueries(0): + test(self) + return inner + + +class TestDataTests(TestCase): + # setUpTestData re-assignment are also wrapped in TestData. + jim_douglas = None + + @classmethod + def setUpTestData(cls): + cls.jim_douglas = Person.objects.create(name='Jim Douglas') + cls.car = Car.objects.create(name='1963 Volkswagen Beetle') + cls.herbie = cls.jim_douglas.possessed_cars.create( + car=cls.car, + belongs_to=cls.jim_douglas, + ) + cls.non_deepcopy_able = NonDeepCopyAble() + + @assert_no_queries + def test_class_attribute_equality(self): + """Class level test data is equal to instance level test data.""" + self.assertEqual(self.jim_douglas, self.__class__.jim_douglas) + + @assert_no_queries + def test_class_attribute_identity(self): + """ + Class level test data is not identical to instance level test data. + """ + self.assertIsNot(self.jim_douglas, self.__class__.jim_douglas) + + @assert_no_queries + def test_identity_preservation(self): + """Identity of test data is preserved between accesses.""" + self.assertIs(self.jim_douglas, self.jim_douglas) + + @assert_no_queries + def test_known_related_objects_identity_preservation(self): + """Known related objects identity is preserved.""" + self.assertIs(self.herbie.car, self.car) + self.assertIs(self.herbie.belongs_to, self.jim_douglas) + + @ignore_warnings(category=RemovedInDjango41Warning) + def test_undeepcopyable(self): + self.assertIs(self.non_deepcopy_able, self.__class__.non_deepcopy_able) + + def test_undeepcopyable_warning(self): + msg = ( + "Assigning objects which don't support copy.deepcopy() during " + "setUpTestData() is deprecated. Either assign the " + "non_deepcopy_able attribute during setUpClass() or setUp(), or " + "add support for deepcopy() to " + "test_utils.test_testcase.TestDataTests.non_deepcopy_able." + ) + with self.assertRaisesMessage(RemovedInDjango41Warning, msg): + self.non_deepcopy_able + + def test_repr(self): + self.assertEqual( + repr(TestData('attr', 'value')), + "", + ) + + +class SetupTestDataIsolationTests(TestCase): + """ + In-memory data isolation is respected for model instances assigned to class + attributes during setUpTestData. + """ + @classmethod + def setUpTestData(cls): + cls.car = Car.objects.create(name='Volkswagen Beetle') + + def test_book_name_deutsh(self): + self.assertEqual(self.car.name, 'Volkswagen Beetle') + self.car.name = 'VW sKäfer' + self.car.save() + + def test_book_name_french(self): + self.assertEqual(self.car.name, 'Volkswagen Beetle') + self.car.name = 'Volkswagen Coccinelle' + self.car.save()