[Django] #35548: An error in TestCase.setUpTestData() leaks data on databases without transactions

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions
---------------------------------------------+------------------------
Reporter: Tim Graham | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
While working on [https://github.com/mongodb-labs/django-mongodb a backend
for MongoDB], I found that an exception on
[https://github.com/django/django/blob/72b7aecbbfbec0ceb1a829eef82a68d7283df604/tests/expressions/tests.py#L1445
the fourth line] of
`expressions.tests.ExpressionsNumericTests.setUpTestData` left `Number`
rows in the database such that a later test failed like this:

{{{
======================================================================
ERROR: test_F_reuse (expressions.tests.ExpressionsTests.test_F_reuse)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/tim/code/django/tests/expressions/tests.py", line 1217, in
test_F_reuse
self.assertEqual(n_qs.get(), n)
^^^^^^^^^^
File "/home/tim/code/django/django/db/models/query.py", line 652, in get
raise self.model.MultipleObjectsReturned(
expressions.models.Number.MultipleObjectsReturned: get() returned more
than one Number -- it returned more than 20!
}}}

This is the same problem as #25176 but for databases with
`DatabaseFeatures.supports_transactions = False`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35548>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions

-----------------------------------+------------------------------------

Reporter: Tim Graham | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: dev

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-----------------------------------+------------------------------------
Changes (by Simon Charette):

* stage: Unreviewed => Accepted

Comment:

I think the following should do and also avoid a lack of rolling back
transaction when they are supported

{{{#!diff
diff --git a/django/test/testcases.py b/django/test/testcases.py
index f1c6b5ae9c..5cd90a7415 100644
--- a/django/test/testcases.py
+++ b/django/test/testcases.py
@@ -7,7 +7,7 @@
import threading
import unittest
from collections import Counter
-from contextlib import contextmanager
+from contextlib import contextmanager, suppress
from copy import copy, deepcopy
from difflib import get_close_matches
from functools import wraps
@@ -1125,6 +1125,11 @@ def _pre_setup(self):
try:
self._fixture_setup()
except Exception:
+ # Attempt to teardown fixtures on exception during setup as
+ # `_post_teardown` won't be triggered to cleanup state when
an
+ # an exception is surfaced to `SimpleTestCase._pre_setup`.
+ with suppress(Exception):
+ self._fixture_teardown()
if self.available_apps is not None:
apps.unset_available_apps()
setting_changed.send(
}}}

Only lightly tested though.
--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:1>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions
-----------------------------------+------------------------------------

Reporter: Tim Graham | Owner: Rish
Type: Bug | Status: assigned

Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Changes (by Rish):

* owner: nobody => Rish
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:2>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions
-----------------------------------+------------------------------------
Reporter: Tim Graham | Owner: Rish
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Rish):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:3>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions
-----------------------------------+------------------------------------
Reporter: Tim Graham | Owner: Rish
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Changes (by Tim Graham):

* needs_tests: 0 => 1

Comment:

The [https://github.com/django/django/pull/18312 PR] lacks any tests.
Perhaps inspiration could be taken from
0abb06930fc0686cb35079934e5bb40df66f5691.
--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:4>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions
-----------------------------------+------------------------------------

Reporter: Tim Graham | Owner: (none)
Type: Bug | Status: new

Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Tim Graham):

* owner: Rish => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:5>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions

-----------------------------------+--------------------------------------
Reporter: Tim Graham | Owner: JaeHyuckSa
Type: Bug | Status: assigned

Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-----------------------------------+--------------------------------------
Changes (by JaeHyuckSa):

* needs_tests: 1 => 0
* owner: (none) => JaeHyuckSa

* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:6>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions
-----------------------------------+--------------------------------------
Reporter: Tim Graham | Owner: JaeHyuckSa
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Changes (by Hisham Mahmood):

* needs_docs: 0 => 1

Comment:

[https://github.com/django/django/pull/19774 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:7>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions
-----------------------------------+--------------------------------------
Reporter: Tim Graham | Owner: JaeHyuckSa
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Changes (by Tim Graham):

* needs_docs: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:8>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions

-------------------------------------+-------------------------------------

Reporter: Tim Graham | Owner:
| JaeHyuckSa
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by H. White):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:9>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions

-----------------------------------+--------------------------------------

Reporter: Tim Graham | Owner: JaeHyuckSa
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-----------------------------------+--------------------------------------
Changes (by JaeHyuckSa):

* stage: Ready for checkin => Accepted

Comment:

As I also mentioned on the other ticket, please refrain from modifying the
Triage Stage directly.
--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:10>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions
-----------------------------------+--------------------------------------
Reporter: Tim Graham | Owner: JaeHyuckSa
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by blighj):

In their defence, there is guidance that says members can mark tickets
ready for checkin
https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/#ready-for-checkin
>The ticket was reviewed by any member of the community other than the
person who supplied the patch and found to meet all the requirements for a
commit-ready contribution. A merger now needs to give a final review prior
to being committed.
https://forum.djangoproject.com/t/django-ticket-status-change/16239

It does sort of elude that if a member things a patch meets all the
requirements they can mark it ready for checkin
I know it's not the usual practise though.
--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:11>

[Django]

#35548: An error in TestCase.setUpTestData() leaks data on databases without
transactions
-----------------------------------+--------------------------------------
Reporter: Tim Graham | Owner: JaeHyuckSa
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by Jacob Walls):

Right, I like it when people set Ready for Checkin, because it clarifies
that I'm not the blocker for everything. The other mergers and I are only
blockers on the stuff marked RFC. So when someone does that, it floats to
the top of my list as a way of respecting the time the reviewer donated.
--
Ticket URL: <https://code.djangoproject.com/ticket/35548#comment:12>