Opened 4 years ago
Closed 4 years 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 by , 4 years ago
comment:2 by , 4 years ago
Component: | Uncategorized → Testing framework |
---|---|
Summary: | LiveServerTestCase's tearDownClass() not symmetric with setUpClass() → LiveServerTestCase._tearDownClassInternal has unneeded hasattr check |
Type: | Uncategorized → Cleanup/optimization |
comment:3 by , 4 years ago
Cc: | added |
---|---|
Summary: | LiveServerTestCase._tearDownClassInternal has unneeded hasattr check → LiveServerTestCase._tearDownClassInternal() has unneeded hasattr check |
Triage Stage: | Unreviewed → Accepted |
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):
follow-up: 5 comment:4 by , 4 years ago
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 by , 4 years ago
Replying to Chris Jerdonek:
Are you sure that
super().tearDownClass()
needs to be called in thatfinally
block? Maybe it only needs to be called in the case thatsetUpClass()
raises an exception (because otherwise,tearDownClass()
will be called automatically byunittest
). 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 by , 4 years ago
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.
comment:7 by , 4 years ago
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()
comment:8 by , 4 years ago
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 by , 4 years ago
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 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 4 years ago
Has patch: | set |
---|
This is now ready for review: https://github.com/django/django/pull/13987
comment:12 by , 4 years ago
Cc: | added |
---|
comment:13 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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:This is because it doesn't seem like it's possible for
_tearDownClassInternal()
to be called withoutserver_thread
being set. For example, per the Python unittest docs,tearDownClass()
isn't called ifsetUpClass()
errors out:Also,
LiveServerTestCase.setUpClass()
only calls_tearDownClassInternal()
whencls.server_thread
is already set.