Opened 2 months ago

Closed 2 months ago

#32417 closed Cleanup/optimization (fixed)

LiveServerTestCase._tearDownClassInternal() has unneeded hasattr check

Reported by: Chris Jerdonek Owned by: Chris Jerdonek
Component: Testing framework Version: 2.2
Severity: Normal Keywords:
Cc: Jon Dufresne, Florian Apolloner Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (15)

comment:1 Changed 2 months ago by 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.

comment:2 Changed 2 months ago by Chris Jerdonek

Component: UncategorizedTesting framework
Summary: LiveServerTestCase's tearDownClass() not symmetric with setUpClass()LiveServerTestCase._tearDownClassInternal has unneeded hasattr check
Type: UncategorizedCleanup/optimization

comment:3 Changed 2 months ago by Mariusz Felisiak

Cc: Jon Dufresne added
Summary: LiveServerTestCase._tearDownClassInternal has unneeded hasattr checkLiveServerTestCase._tearDownClassInternal() has unneeded hasattr check
Triage Stage: UnreviewedAccepted

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

comment:4 Changed 2 months ago by 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.

comment:5 in reply to:  4 Changed 2 months ago by 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 🤔.

comment:6 Changed 2 months ago by 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.

Last edited 2 months ago by Chris Jerdonek (previous) (diff)

comment:7 Changed 2 months ago by 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()
Last edited 2 months ago by Chris Jerdonek (previous) (diff)

comment:8 Changed 2 months ago by 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.

comment:9 Changed 2 months ago by 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.

comment:10 Changed 2 months ago by Chris Jerdonek

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:11 Changed 2 months ago by Chris Jerdonek

Has patch: set

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

comment:12 Changed 2 months ago by Mariusz Felisiak

Cc: Florian Apolloner added

comment:13 Changed 2 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:14 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In fc0069b:

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

comment:15 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 600ff26a:

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

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