LiveServerTestCase._tearDownClassInternal() has unneeded hasattr check

[Chris Jerdonek]

I noticed that #30171 ​removed some setup/teardown symmetry in LiveServerTestCase that it seems like would be better to keep.

Specifically, LiveServerTestCase.setUpClass() unconditionally calls inc_thread_sharing() on certain connections:
​https://github.com/django/django/blob/37cc6a9dce3354cd37f23ee972bc25b0e5cebd5c/django/test/testcases.py#L1437-L1452
but in _tearDownClassInternal(), after the above change it does the reverse only conditionally -- based on whether the server_thread attribute is present. In particular, if _create_server_thread() errors out, which is what sets server_thread, then the connections won't be restored to their initial state inside _tearDownClassInternal().

It seems like the restoration should happen unconditionally in _tearDownClassInternal(), which is how it was before #30171.

[Chris Jerdonek]

After looking at this some more, rather than keeping the symmetry, it looks like the first three lines of LiveServerTestCase._tearDownClassInternal can simply be deleted:

@classmethod
def _tearDownClassInternal(cls):
    # There may not be a 'server_thread' attribute if setUpClass() for some
    # reasons has raised an exception.
    if hasattr(cls, 'server_thread'):

This is because it doesn't seem like it's possible for _tearDownClassInternal() to be called without server_thread being set. For example, per the ​Python unittest docs, tearDownClass() isn't called if setUpClass() errors out:

If an exception is raised during a setUpClass then the tests in the class are not run and the tearDownClass is not run.

Also, LiveServerTestCase.setUpClass() only calls _tearDownClassInternal() when cls.server_thread is already set.

[Mariusz Felisiak]

Looks reasonable.

staticfiles_tests.test_liveserver.StaticLiveServerChecks.test_test_test fails with this change, but as far as I'm aware removing cls.server_thread is only a way to avoid _tearDownClassInternal() call, so we can override it instead, e.g.:

diff --git a/tests/staticfiles_tests/test_liveserver.py b/tests/staticfiles_tests/test_liveserver.py
index 820fa5bc89..22c4a645e7 100644
--- a/tests/staticfiles_tests/test_liveserver.py
+++ b/tests/staticfiles_tests/test_liveserver.py
@@ -54,6 +54,10 @@ class StaticLiveServerChecks(LiveServerBase):
         # skip it, as setUpClass doesn't call its parent either
         pass
 
+    @classmethod
+    def _tearDownClassInternal(cls):
+        pass
+
     @classmethod
     def raises_exception(cls):
         try:
@@ -64,9 +68,6 @@ class StaticLiveServerChecks(LiveServerBase):
             # app without having set the required STATIC_URL setting.")
             pass
         finally:
-            # Use del to avoid decrementing the database thread sharing count a
-            # second time.
-            del cls.server_thread
             super().tearDownClass()
 
     def test_test_test(self):

[Chris Jerdonek]

Are you sure that super().tearDownClass() needs to be called in that finally block? Maybe it only needs to be called in the case that setUpClass() raises an exception (because otherwise, tearDownClass() will be called automatically by unittest). Maybe that explains why something is getting called twice when it shouldn't.

[Mariusz Felisiak]

Replying to Chris Jerdonek:

Are you sure that super().tearDownClass() needs to be called in that finally block? Maybe it only needs to be called in the case that setUpClass() raises an exception (because otherwise, tearDownClass() will be called automatically by unittest). Maybe that explains why something is getting called twice when it shouldn't.

StaticLiveServerChecks.tearDownClass() doesn't call super().tearDownClass() so LiveServerTestCase.tearDownClass() is called only once 🤔.

[Chris Jerdonek]

Sorry, I'm not sure what I was thinking when I wrote that.

Looking more carefully, I see that LiveServerTestCase.setUpClass() already does (some of) its own clean-up when cls.server_thread.error is true:
​https://github.com/django/django/blob/63d239db037f02d98b7771c90422840bbb4a319a/django/test/testcases.py#L1543-L1547

class LiveServerTestCase(TransactionTestCase):

    @classmethod
    def setUpClass(cls):
        super().setUpClass()
        ...
        if cls.server_thread.error:
            # Clean up behind ourselves, since tearDownClass won't get called in
            # case of errors.
            cls._tearDownClassInternal()
            raise cls.server_thread.error

I believe that explains why the database thread sharing count was already decremented as this comment in the finally block is referring to:

# Use del to avoid decrementing the database thread sharing count a
# second time.

(In the happy path of the StaticLiveServerChecks test, cls.server_thread.error is true at the end of LiveServerTestCase.setUpClass() because that's the raised ImproperlyConfigured error that the test is looking for.)

A couple things occur to me:

First, it seems like LiveServerTestCase.setUpClass() should actually be calling cls.tearDownClass() if cls.server_thread.error is true, instead of cls._tearDownClassInternal(). The reason is that otherwise, it's not undoing the rest of LiveServerTestCase.setUpClass(). (Might that be causing unwanted side effects on certain LiveServerTestCase test failures today?)

Second, if the change I just mentioned is made, then it looks like StaticLiveServerChecks's. raises_exception() could be changed to call super().tearDownClass() in the finally block only when cls.server_thread.error is false. That would exactly mirror LiveServerTestCase.setUpClass() and would eliminate the need for either hack of (1) calling del cls.server_thread or (2) overriding _tearDownClassInternal() with a no-op.

[Chris Jerdonek]

Looking at the history of why _tearDownClassInternal() is called rather than tearDownClass():
​https://github.com/django/django/commit/73a610d2a81bc3bf2d3834786b2458bc85953ed0
it looks like the safer option might be to move the final two lines of LiveServerTestCase.tearDownClass() into LiveServerTestCase._tearDownClassInternal(), so that LiveServerTestCase .tearDownClass() would become:

@classmethod
def tearDownClass(cls):
    cls._tearDownClassInternal()

[Chris Jerdonek]

I'm working on a PR for this, FYI. I think there might actually be a minor bug in LiveServerTestCase, so I'll be looking into confirming that prior to the clean-up I was proposing above.

[Chris Jerdonek]

I posted here (#32437) the bug I mentioned in my previous comment. I will post a PR for the current issue only after a fix for that issue is merged. The reason is that I'd rather do the clean-up after the bug fix, since the clean-up overlaps the bug fix.

[Chris Jerdonek]

This is now ready for review: ​https://github.com/django/django/pull/13987

[Mariusz Felisiak <felisiak.mariusz@…>]

In fc0069b:

Refs #32417 -- Improved cleaning up and fixed isolation of staticfiles_tests tests.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 600ff26a:

Fixed #32417 -- Removed unneeded hasattr() check in LiveServerTestCase._tearDownClassInternal().