Merge lp:~gz/bzr/trivial_discard_log_memento_607413 into lp:bzr

[Martin Packman]

Delete _log_memento after TestCase is done with it so as not to upset the funky weakref cleanup mechanism.

Delete _log_memento after TestCase is done with it so as not to upset the funky weakref cleanup mechanism. The original visible fallout got fixed upstream in the logging module, and with bug 613247 landed the general case of accidentally persisting resources is handled. This branch just makes sure that even with TestCase objects we keep alive on purpose (errors, for instance), the token is thrown away so logging doesn't keep tracking it.

While I was there I updated the docstrings for the TestCase log setup and teardown functions as they no longer deal in tempfiles.

[John A Meinel (jameinel)]

Are we sure this is safe across supported versions? You mention fixed
upstream, but that always concerns me about compatibility.

John
=:->
On Oct 17, 2011 5:38 PM, "Martin Packman" <email address hidden>
wrote:

> Martin Packman has proposed merging
> lp:~gz/bzr/trivial_discard_log_memento_607413 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #607413 in Bazaar: " Exception atexit with selftest and Python 2.7"
> https://bugs.launchpad.net/bzr/+bug/607413
>
> For more details, see:
>
> https://code.launchpad.net/~gz/bzr/trivial_discard_log_memento_607413/+merge/79572
>
> Delete _log_memento after TestCase is done with it so as not to upset the
> funky weakref cleanup mechanism. The original visible fallout got fixed
> upstream in the logging module, and with bug 613247 landed the general case
> of accidentally persisting resources is handled. This branch just makes sure
> that even with TestCase objects we keep alive on purpose (errors, for
> instance), the token is thrown away so logging doesn't keep tracking it.
>
> While I was there I updated the docstrings for the TestCase log setup and
> teardown functions as they no longer deal in tempfiles.
> --
>
> https://code.launchpad.net/~gz/bzr/trivial_discard_log_memento_607413/+merge/79572
> Your team bzr-core is requested to review the proposed merge of
> lp:~gz/bzr/trivial_discard_log_memento_607413 into lp:bzr.
>
> === modified file 'bzrlib/tests/__init__.py'
> --- bzrlib/tests/__init__.py 2011-10-14 01:48:16 +0000
> +++ bzrlib/tests/__init__.py 2011-10-17 15:36:26 +0000
> @@ -1718,10 +1718,7 @@
> return result
>
> def _startLogFile(self):
> - """Send bzr and test log messages to a temporary file.
> -
> - The file is removed as the test is torn down.
> - """
> + """Setup a in-memory target for bzr and testcase log messages"""
> pseudo_log_file = StringIO()
> def _get_log_contents_for_weird_testtools_api():
> return [pseudo_log_file.getvalue().decode(
> @@ -1734,14 +1731,13 @@
> self.addCleanup(self._finishLogFile)
>
> def _finishLogFile(self):
> - """Finished with the log file.
> -
> - Close the file and delete it.
> - """
> + """Flush and dereference the in-memory log for this testcase"""
> if trace._trace_file:
> # flush the log file, to get all content
> trace._trace_file.flush()
> trace.pop_log_file(self._log_memento)
> + # The logging module now tracks references for cleanup so discard
> ours
> + del self._log_memento
>
> def thisFailsStrictLockCheck(self):
> """It is known that this test would fail with -Dstrict_locks.
>
>
>

[Jelmer Vernooij (jelmer)]

review: Approve

[Martin Packman (gz)]

> Are we sure this is safe across supported versions? You mention fixed
> upstream, but that always concerns me about compatibility.

Yes, this is just avoiding problems from a change in 2.7 which caused logging to try flushing things late on in module teardown. Prior to that, trace.pop_log_file did everything needed for cleanup and there was no need to care about references at all.

The original Python change:
<http://bugs.python.org/issue6615>
<http://hg.python.org/cpython/diff/022cd0f3c558/Lib/logging/__init__.py>

We're defended against weird behaviour from this pretty well already, using StringIO rather than real files and cleaning up testcases as we go should be enough, this is just a cherry on top.

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-10-14 01:48:16 +0000
+++ bzrlib/tests/__init__.py	2011-10-17 15:36:26 +0000
@@ -1718,10 +1718,7 @@
         return result
 
     def _startLogFile(self):
-        """Send bzr and test log messages to a temporary file.
-
-        The file is removed as the test is torn down.
-        """
+        """Setup a in-memory target for bzr and testcase log messages"""
         pseudo_log_file = StringIO()
         def _get_log_contents_for_weird_testtools_api():
             return [pseudo_log_file.getvalue().decode(
@@ -1734,14 +1731,13 @@
         self.addCleanup(self._finishLogFile)
 
     def _finishLogFile(self):
-        """Finished with the log file.
-
-        Close the file and delete it.
-        """
+        """Flush and dereference the in-memory log for this testcase"""
         if trace._trace_file:
             # flush the log file, to get all content
             trace._trace_file.flush()
         trace.pop_log_file(self._log_memento)
+        # The logging module now tracks references for cleanup so discard ours
+        del self._log_memento
 
     def thisFailsStrictLockCheck(self):
         """It is known that this test would fail with -Dstrict_locks.