[Django] #25176: TestCase: an error in setUpTestData snowballs across the entire test suite
Django
--------------------------------------+--------------------
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
If a setUpTestData function fails, it causes an error that breaks the
current transaction; this is not undone by TestCase, and thus all
following database access in all other tests fails, outputting a lot of
ERRORs on a test run, and making it hard to debug that it was only the
*first* one that has an actual problem. The atomic instances can be exited
cleanly on exception fairly easily inside the TestCase code.
--
Ticket URL: <https://code.djangoproject.com/ticket/25176>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: adamchainz | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by adamchainz):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
https://github.com/django/django/pull/5051
I've struggled to add a test for this. I had the idea for something like
the below, but the problem is that if you try do any checking in the
exception handler in setUpClass you can then trigger tearDownClass even
though it shouldn't be run. I don't think there is a sane way of testing
it, except from creating tests that run a second set of tests where one
has a broken setUpTestData, and then asserting that only the one of the
second set failed.
{{{
diff --git a/tests/test_utils/tests.py b/tests/test_utils/tests.py
index 7056ea3..ede8cca 100644
--- a/tests/test_utils/tests.py
+++ b/tests/test_utils/tests.py
@@ -2,6 +2,7 @@
from __future__ import unicode_literals
import unittest
+import sys
from django.conf.urls import url
from django.contrib.staticfiles.finders import get_finder, get_finders
@@ -744,6 +745,26 @@ class SkippingExtraTests(TestCase):
pass
+class TestBadSetupTestData(TestCase):
+
+ class MyException(Exception):
+ pass
+
+ @classmethod
+ def setUpClass(cls):
+ try:
+ super(TestBadSetupTestData, cls).setUpClass()
+ except cls.MyException:
+ assert not connection.in_atomic_block
+
+ @classmethod
+ def setUpTestData(cls):
+ raise cls.MyException()
+
+ def test_failure_in_setUpTestData_should_exit_cleanly(self):
+ pass
+
+
class AssertRaisesMsgTest(SimpleTestCase):
def test_special_re_chars(self):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25176#comment:1>
Django
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25176#comment:2>
Django
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
For the test, could you store the result of `connection.in_atomic_block`
in a variable and then put the assertion in the test method? I didn't
understand what you meant when you said, "you can then trigger
tearDownClass even though it shouldn't be run".
--
Ticket URL: <https://code.djangoproject.com/ticket/25176#comment:3>
Django
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by adamchainz):
The test method would never be reached - we're triggering an error in
`setUpClass` which means the tests don't get run.
The above test patch catches the breakage and allows the tests to continue
- in the "broken" world. I'm not sure this is legit or even if it works
now it might not be forwards compatible with future edits .
I also experimented with a broken test case with `@expectedFailure`
wrapped around it but that's not sensitive enough since it allows any
exception to be expected. It might be possible to write a custom
`expectedFailure` decorator that checks it was the custom exception raised
during the test case and marks it as a pass/skip/expected failure, but
when I briefly looked it seemed a bit hacky.
--
Ticket URL: <https://code.djangoproject.com/ticket/25176#comment:4>
Django
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by aaugustin):
If there is no reasonable way to write a test, let's ship the fix as is.
That happens occasionally for changes in the testing infrastructure. The
latest iteration of the patch looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/25176#comment:5>
Django
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25176#comment:6>
Django
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"0abb06930fc0686cb35079934e5bb40df66f5691" 0abb069]:
{{{
#!CommitTicketReference repository=""
revision="0abb06930fc0686cb35079934e5bb40df66f5691"
Fixed #25176 -- Prevented TestCase.setUpTestData() exception from leaking
transaction.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25176#comment:7>
Django
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"fd81588bc64583c8ee17f680e621129c9be83c43" fd81588]:
{{{
#!CommitTicketReference repository=""
revision="fd81588bc64583c8ee17f680e621129c9be83c43"
Refs #25176 -- Fixed typo in tests/test_utils/tests.py
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25176#comment:8>
Django
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"b46dad1befb1edd680c71f09df952245b3413f20" b46dad1b]:
{{{
#!CommitTicketReference repository=""
revision="b46dad1befb1edd680c71f09df952245b3413f20"
[1.8.x] Fixed #25176 -- Prevented TestCase.setUpTestData() exception from
leaking transaction.
Backport of 0abb06930fc0686cb35079934e5bb40df66f5691 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25176#comment:9>