[Django] #32417: LiveServerTestCase's tearDownClass() not symmetric with setUpClass()
Django
------------------------------------------+------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
I noticed that #30171
[https://github.com/django/django/commit/37cc6a9dce3354cd37f23ee972bc25b0e5cebd5c
#diff-
7833da5b45a68d00834388d97dd5c4413e3796497c7bc5e0cc2621b08a2d0df1L1481-R1483
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32417>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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:
{{{#!python
@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 [https://docs.python.org/3/library/unittest.html
#setupclass-and-teardownclass 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* type: Uncategorized => Cleanup/optimization
* component: Uncategorized => Testing framework
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Jon Dufresne (added)
* stage: Unreviewed => Accepted
Comment:
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):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:3>
Django
--------------------------------------+------------------------------------
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:4>
Django
--------------------------------------+------------------------------------
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:4 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 🤔.
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:5>
Django
--------------------------------------+------------------------------------
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
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
--------------------------------------+------------------------------------
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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:
{{{#!python
@classmethod
def tearDownClass(cls):
cls._tearDownClassInternal()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:7>
Django
--------------------------------------+------------------------------------
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:8>
Django
--------------------------------------+------------------------------------
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:9>
Django
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* owner: nobody => Chris Jerdonek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* has_patch: 0 => 1
Comment:
This is now ready for review: https://github.com/django/django/pull/13987
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Florian Apolloner (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"fc0069bfa52f9d8bd62aa0f5749905d767ab42e9" fc0069b]:
{{{
#!CommitTicketReference repository=""
revision="fc0069bfa52f9d8bd62aa0f5749905d767ab42e9"
Refs #32417 -- Improved cleaning up and fixed isolation of
staticfiles_tests tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Component: Testing framework | Version: 2.2
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"600ff26a85752071da36e3a94c66dd8a77ee314a" 600ff26a]:
{{{
#!CommitTicketReference repository=""
revision="600ff26a85752071da36e3a94c66dd8a77ee314a"
Fixed #32417 -- Removed unneeded hasattr() check in
LiveServerTestCase._tearDownClassInternal().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32417#comment:15>