Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#25176 closed Cleanup/optimization (fixed)

TestCase: an error in setUpTestData snowballs across the entire test suite

Reported by: Adam Johnson Owned by: nobody
Component: Testing framework Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Change History (9)

comment:1 by Adam Johnson, 9 years ago

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

comment:2 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Tim Graham, 9 years ago

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

comment:4 by Adam Johnson, 9 years ago

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.

comment:5 by Aymeric Augustin, 9 years ago

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.

comment:6 by Tim Graham, 9 years ago

Patch needs improvement: set

comment:7 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 0abb069:

Fixed #25176 -- Prevented TestCase.setUpTestData() exception from leaking transaction.

comment:8 by Tim Graham <timograham@…>, 9 years ago

In fd81588:

Refs #25176 -- Fixed typo in tests/test_utils/tests.py

comment:9 by Tim Graham <timograham@…>, 9 years ago

In b46dad1b:

[1.8.x] Fixed #25176 -- Prevented TestCase.setUpTestData() exception from leaking transaction.

Backport of 0abb06930fc0686cb35079934e5bb40df66f5691 from master

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