#32417: LiveServerTestCase._tearDownClassInternal() has unneeded hasattr check
--------------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.089cc047a0864ee79bfe3fad210d43b1%40djangoproject.com.
