Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#25176 closed Cleanup/optimization (fixed)

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

Reported by: Adam (Chainz) 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 Changed 5 years ago by Adam (Chainz) Johnson

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 Changed 5 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:3 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Adam (Chainz) Johnson

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

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 Changed 5 years ago by Tim Graham

Patch needs improvement: set

comment:7 Changed 5 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 0abb069:

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

comment:8 Changed 5 years ago by Tim Graham <timograham@…>

In fd81588:

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

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

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